[tools] gpls/internal/golang: check conditions for package move
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
Madeline KalilHi! Do you want me to review this? It's marked as WIP but I'm happy to take an initial pass.
Yes, it would be great if you can do an initial pass of review. Thanks!
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 7 comments![]( "Open in Gerrit")
Nice. Took a quick pass through this and it generally LGTM, but we should try to clarify as much as possible while we're looking at this code.
Will probably have more comments after the initial comments are addressed.
var isPkgPath bool```
var (
isPkgPath bool
err error
)
```
It may be convenient to use the err in scope, but that is action at a distance that should be avoided. If your only goal is to inspect the local error value, use a local variable.
func renamePackageName(ctx context.Context, s *cache.Snapshot, pkg *cache.Package, f file.Handle, newName PackageName, newPath PackagePath) (map[protocol.DocumentURI][]diff.Edit, error) {Call this just 'movePackage' or 'moveAndRenamePackage', since it's doing more than just renaming?
oldBase := f.URI().DirPath()Shouldn't this be 'oldDir'?
A basename is the last component, not the dir, so this is confusing. (I know it's preexisting, but we should clarify while we're looking at it).
// package moveBoth of these are package moves though.
func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPkgPath PackagePath) (map[protocol.DocumentURI][]diff.Edit, error) {Now I'm confused as to what to call this, and why it would differ from 'movePackage' above. Maybe we should unify them? WDYT?
func isValidPackagePath(curPkg *cache.Package, f file.Handle, newName string) (isPkgPath bool, err error) {It was a little unclear to me how to use this from reading its usage above.
How about refactoring this slightly, to return a pkgPath and name, and then you don't need getNewPkgDir, which is called in multiple places.
Or am I missing something?
```
// checkPackageRename returns the effective new package directory and name
// resulting from renaming the current package to newName, which may be an
// identifier or a package path.
//
// An error is returned if the renaming is invalid.
func checkPackageRename(curPkg *cache.Package, f file.Handle, newName) (dir string, name PkgName, err error)
```
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 6 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
6 comments
```
var (
isPkgPath bool
err error
)
```It may be convenient to use the err in scope, but that is action at a distance that should be avoided. If your only goal is to inspect the local error value, use a local variable.
Done
func renamePackageName(ctx context.Context, s *cache.Snapshot, pkg *cache.Package, f file.Handle, newName PackageName, newPath PackagePath) (map[protocol.DocumentURI][]diff.Edit, error) {Call this just 'movePackage' or 'moveAndRenamePackage', since it's doing more than just renaming?
Went with moveAndRenamePackage
Shouldn't this be 'oldDir'?
A basename is the last component, not the dir, so this is confusing. (I know it's preexisting, but we should clarify while we're looking at it).
Good catch, thanks.
// package moveBoth of these are package moves though.
But in the second case, the package directory stays the same. We pass PackagePath("") for the newPath if we determine that just the package name is changing, not the package directory. I'll update the comment
EDIT: Nevermind - this part got removed now that checkPackageName returns the effective new package directory and we always pass a value for newPath
func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPkgPath PackagePath) (map[protocol.DocumentURI][]diff.Edit, error) {Now I'm confused as to what to call this, and why it would differ from 'movePackage' above. Maybe we should unify them? WDYT?
Yeah, the names are rather confusing.
moveAndRenamePackage seems like a good outer function name.
renamePackage updates package and import declarations in all affected packages, so maybe we should call it updateImportsAndPackageDecls or just updatePackageDecls?
moveAndRenamePackage itself just updates replace directives in go.mod files, which I'll extract to its own function.
func isValidPackagePath(curPkg *cache.Package, f file.Handle, newName string) (isPkgPath bool, err error) {It was a little unclear to me how to use this from reading its usage above.
How about refactoring this slightly, to return a pkgPath and name, and then you don't need getNewPkgDir, which is called in multiple places.
Or am I missing something?
```
// checkPackageRename returns the effective new package directory and name
// resulting from renaming the current package to newName, which may be an
// identifier or a package path.
//
// An error is returned if the renaming is invalid.
func checkPackageRename(curPkg *cache.Package, f file.Handle, newName) (dir string, name PkgName, err error)
```
Sounds good, I agree about de-duplicating the work in getNewPkgDir. We still need to know if the user entered a string that would constitute a package move because if package move is disabled we shouldn't allow this
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 9 comments![]( "Open in Gerrit")
if isPkgPath && !snapshot.Options().PackageMove {
return nil, fmt.Errorf("a full package path is specified but internal setting 'packageMove' is not enabled")
}If err is non-nil, do we need to handle isPkgPath? Generally speaking, when a non-nil error is returned, other results are undefined.
editMap, err = moveAndRenamePackage(ctx, snapshot, f, PackageName(newPkgName), PackagePath(newName), newPkgDir)We don't actually need to call 'moveAndRenamePackage' twice here: the only difference is the package path argument. If you refactor into one call, then the surrounding logic can be pushed down into that function (which I suggest renaming back to just 'renamePackage'), simplifying the call site here.
} else {I got rather confused by the multiple nested 'elses' here.
We already factor out 'renameOrdinary', perhaps we should factor out the above clause into 'renamePackage', now that it's gotten more complicated.
I understand the factoring is getting very confusing, which multiple things that look like 'rename package'. Let me go through and see if I can suggest further improvements.
func moveAndRenamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPath PackagePath, newPkgDir string) (map[protocol.DocumentURI][]diff.Edit, error) {Let's just call this 'renamePackage', to be the analog of 'renameOrdinary', and push down the logic from the call site here.
Sorry if this is conflicting with earlier advice--the pre-existing factoring of this control flow was already confusing, and I'm slowly starting to understand it again. Happy to sit down and pair on this if it would help.
// identifier or a package path. It also returns a bool that is true when
// the newName is a package path.I think it may be clearer to pass in the snapshot here (or snapshot.Options()) and avoid returning isPkgPath, so that the call site doesn't need to know about whether or not package path renaming is supported.
isPkgPath = fs.ValidPath(newName) && !validIdentIf you flip the ordering of these two operands, you can avoid the call to ValidPath when the name is an identifier.
} else if validIdent {
// Not an attempted move. Check if a directory already exists at that
// path, in which case we should not allow renaming.
newPkgDir = filepath.Join(filepath.Dir(f.URI().DirPath()), newName)
newPkgName = PackageName(newName)
_, err := os.Stat(newPkgDir)
if err == nil {
// Directory already exists, return an error.
return "", "", false, fmt.Errorf("invalid package identifier: %q already exists", newName)
}...or better, just handle the validIdent early, before you even compute isPkgPath, with an early return.
The nested control flow increases cyclomatic complexity, and can make the code harder to read. By handling the isValidIdent early, you eliminate the relationship between isValidIdent and isPkgPath, and reduce nesting.
} else {
return "", "", false, fmt.Errorf("invalid package path or identifier %q", newName)
}Similar to other suggestion, this also can be handled with an early return, to reduce nesting.
https://go.dev/wiki/CodeReviewComments#indent-error-flow
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 12 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
The test failures look related.
Yeah, windows :( trying to figure it out
Also still need to add tests for import cycles and nested directories that don't exist
if isPkgPath && !snapshot.Options().PackageMove {
return nil, fmt.Errorf("a full package path is specified but internal setting 'packageMove' is not enabled")
}If err is non-nil, do we need to handle isPkgPath? Generally speaking, when a non-nil error is returned, other results are undefined.
Refactored this part
editMap, err = moveAndRenamePackage(ctx, snapshot, f, PackageName(newPkgName), PackagePath(newName), newPkgDir)We don't actually need to call 'moveAndRenamePackage' twice here: the only difference is the package path argument. If you refactor into one call, then the surrounding logic can be pushed down into that function (which I suggest renaming back to just 'renamePackage'), simplifying the call site here.
I think this works well with having checkRenamePackage return newPkgDir, newPkgName, newPkgPath and err, so the caller doesn't have to worry about the settings
I got rather confused by the multiple nested 'elses' here.
We already factor out 'renameOrdinary', perhaps we should factor out the above clause into 'renamePackage', now that it's gotten more complicated.
I understand the factoring is getting very confusing, which multiple things that look like 'rename package'. Let me go through and see if I can suggest further improvements.
Acknowledged
func moveAndRenamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPath PackagePath, newPkgDir string) (map[protocol.DocumentURI][]diff.Edit, error) {Let's just call this 'renamePackage', to be the analog of 'renameOrdinary', and push down the logic from the call site here.
Sorry if this is conflicting with earlier advice--the pre-existing factoring of this control flow was already confusing, and I'm slowly starting to understand it again. Happy to sit down and pair on this if it would help.
Done
// package moveMadeline KalilBoth of these are package moves though.
But in the second case, the package directory stays the same. We pass PackagePath("") for the newPath if we determine that just the package name is changing, not the package directory. I'll update the comment
EDIT: Nevermind - this part got removed now that checkPackageName returns the effective new package directory and we always pass a value for newPath
Acknowledged
func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPkgPath PackagePath) (map[protocol.DocumentURI][]diff.Edit, error) {Madeline KalilNow I'm confused as to what to call this, and why it would differ from 'movePackage' above. Maybe we should unify them? WDYT?
Yeah, the names are rather confusing.
moveAndRenamePackage seems like a good outer function name.
renamePackage updates package and import declarations in all affected packages, so maybe we should call it updateImportsAndPackageDecls or just updatePackageDecls?
moveAndRenamePackage itself just updates replace directives in go.mod files, which I'll extract to its own function.
Acknowledged
// identifier or a package path. It also returns a bool that is true when
// the newName is a package path.I think it may be clearer to pass in the snapshot here (or snapshot.Options()) and avoid returning isPkgPath, so that the call site doesn't need to know about whether or not package path renaming is supported.
Done
func isValidPackagePath(curPkg *cache.Package, f file.Handle, newName string) (isPkgPath bool, err error) {It was a little unclear to me how to use this from reading its usage above.
How about refactoring this slightly, to return a pkgPath and name, and then you don't need getNewPkgDir, which is called in multiple places.
Or am I missing something?
```
// checkPackageRename returns the effective new package directory and name
// resulting from renaming the current package to newName, which may be an
// identifier or a package path.
//
// An error is returned if the renaming is invalid.
func checkPackageRename(curPkg *cache.Package, f file.Handle, newName) (dir string, name PkgName, err error)
```
Sounds good, I agree about de-duplicating the work in getNewPkgDir. We still need to know if the user entered a string that would constitute a package move because if package move is disabled we shouldn't allow this
Done
If you flip the ordering of these two operands, you can avoid the call to ValidPath when the name is an identifier.
Done
} else if validIdent {
// Not an attempted move. Check if a directory already exists at that
// path, in which case we should not allow renaming.
newPkgDir = filepath.Join(filepath.Dir(f.URI().DirPath()), newName)
newPkgName = PackageName(newName)
_, err := os.Stat(newPkgDir)
if err == nil {
// Directory already exists, return an error.
return "", "", false, fmt.Errorf("invalid package identifier: %q already exists", newName)
}...or better, just handle the validIdent early, before you even compute isPkgPath, with an early return.
The nested control flow increases cyclomatic complexity, and can make the code harder to read. By handling the isValidIdent early, you eliminate the relationship between isValidIdent and isPkgPath, and reduce nesting.
Thanks, it's cleaner with the early returns
} else {
return "", "", false, fmt.Errorf("invalid package path or identifier %q", newName)
}Similar to other suggestion, this also can be handled with an early return, to reduce nesting.
https://go.dev/wiki/CodeReviewComments#indent-error-flow
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Madeline Kalil
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Madeline Kalil
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 12 comments![]( "Open in Gerrit")
var (
newPkgName PackageName
newPkgPath PackagePath
err error
)
if newPkgDir, newPkgName, newPkgPath, err = checkPackageRename(snapshot.Options(), pkg, f, newName); err != nil {Looks like these can now be declared with a short variable declaration.
newPkgDir, newPkgName, newPkgPath, err := ...
if err != nil {
...
}
if inPackageName {Can you remind me why we can't just do this in 'renamePackage'? Is it because we don't have the ability to specify a rename document change?
I think that's it. In that case, leave an explanatory comment? The factoring sticks out as not-quite-right, and while I don't think we should work on improving it now, let's at least record why it is the way it is.
oldDir := f.URI().DirPath()Nit: call this 'oldPkgDir' for symmetry, or just inline below.
changes = append(changes,
protocol.DocumentChangeRename(
protocol.URIFromPath(oldDir),
protocol.URIFromPath(newPkgDir)))So, this actually looks wrong.
What if we're renaming a/b, and there's an a/b/c. Doesn't this implicitly move the 'c' directory as well?
What if we're renaming a/b to a, and there are no `go` files in `a`. Can't we support that, by just moving the files?
I think we need to...
1. Check that there are no go files in newPkgDir (or newPkgDir is missing).
2. Create newPkgDir if it doesn't exist.
3. Move each file from oldPkgDir.
4. Delete oldPkgDir if it's now empty.
// renamePackage renames package declarations, imports, and go.mod files.Add:
```
//
// f is the file originating the rename, and therefore f.URI().Dir() is the
// package directory. newName, newPath, and newDir describe the renaming.
```
func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPath PackagePath, newPkgDir string) (map[protocol.DocumentURI][]diff.Edit, error) {We don't say 'newPkgPath', so this should just be 'newDir'. The pkg is implied.
renamingEdits, err := updatePackageDeclsAndImports(ctx, s, f, newName, newPath)Nit: 'renamingEdits' is unnecessarily long, since the function name is 'renamePackage'. How about just 'edits'?
renamingEdits, err = updateModFiles(ctx, s, f.URI().DirPath(), newPkgDir, renamingEdits)No need to return renamingEdits here.
How about just passing in the edits map to both updatePackageDeclsAndImports, and to updateModFiles?
moveToInternal := snapshot.Options().PackageMove && (strings.Contains(string(newName), "internal") || strings.Contains(string(newPath), "internal"))This doesn't look quite right.
1. validating whether a renaming is valid shouldn't depend on the PackageMove option: at this point, we've already handled PackageMove, and whether the move is valid is independent of how we got to it.
2. The condition on importers should be a function of dep path and import path. We shouldn't allow the move if the new import is invalid. See `metadata.IsValidImport`.
func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Handle, newName string) (newPkgDir string, newPkgName PackageName, newPkgPath PackagePath, err error) {We can only perform the package rename if curPkg holds ALL Go files in its directory. If there are any other Go files present, the rename may result in a broken build.
We can do better by trying to find builds that cover all go files, but for now let's just be conservative.
// Directory already exists, return an error."Directory or file already exists at this path; return an error."?
if !strings.Contains(newName, string(os.PathSeparator)) {I don't understand this: doesn't PrepareRename prompt with the package path?
If newName is not an identifier, is it a directory or package path? I thought the latter.
I think the logic here needs to check:
1. That modPath+"/" is a prefix of the new path.
2. That filepath.Join(modDir, filepath.FromSlash(strings.TrimPrefix(newName, modPath+"/"))) is a valid path.
3. That the new path creates a new package.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 13 comments![]( "Open in Gerrit")
Madeline KalilThe test failures look related.
Yeah, windows :( trying to figure it out
Also still need to add tests for import cycles and nested directories that don't exist
Turns out you don't need to explicitly create directories that don't exist, you can just use RenameFile in a DocumentChange and it will create the directories. Still need to add a test for an import cycle
var (
newPkgName PackageName
newPkgPath PackagePath
err error
)
if newPkgDir, newPkgName, newPkgPath, err = checkPackageRename(snapshot.Options(), pkg, f, newName); err != nil {Looks like these can now be declared with a short variable declaration.
newPkgDir, newPkgName, newPkgPath, err := ...
if err != nil {
...
}
I think we still need them declared like this since we use newPkgDir at the end of this function to move the files to the new package directory, and only if inPackageName.
Can you remind me why we can't just do this in 'renamePackage'? Is it because we don't have the ability to specify a rename document change?
I think that's it. In that case, leave an explanatory comment? The factoring sticks out as not-quite-right, and while I don't think we should work on improving it now, let's at least record why it is the way it is.
I agree it's a little clunky - renamePackage calculates only text edits in individual files, which then need to be sorted (L481) and converted to doc changes, and then finally we add the DocumentChangeRename edit. Added a comment.
Nit: call this 'oldPkgDir' for symmetry, or just inline below.
Done
changes = append(changes,
protocol.DocumentChangeRename(
protocol.URIFromPath(oldDir),
protocol.URIFromPath(newPkgDir)))So, this actually looks wrong.
What if we're renaming a/b, and there's an a/b/c. Doesn't this implicitly move the 'c' directory as well?
What if we're renaming a/b to a, and there are no `go` files in `a`. Can't we support that, by just moving the files?
I think we need to...
1. Check that there are no go files in newPkgDir (or newPkgDir is missing).
2. Create newPkgDir if it doesn't exist.
3. Move each file from oldPkgDir.
4. Delete oldPkgDir if it's now empty.
Yeah thinking about it more, it doesn't make sense that nested packages would be also moved. But there is a test TestRenamePackage_NestedModule that asserts that renaming package "foo" in foo/ to "foox" will result in renaming the file "foo/bar/bar.go" which is in package "bar" to "foox/bar/bar.go". Should we change this test expectation?
Also, not sure about the best way to implement (4) so I'll ask you tomorrow.
// renamePackage renames package declarations, imports, and go.mod files.Add:
```
//
// f is the file originating the rename, and therefore f.URI().Dir() is the
// package directory. newName, newPath, and newDir describe the renaming.
```
Done
func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPath PackagePath, newPkgDir string) (map[protocol.DocumentURI][]diff.Edit, error) {We don't say 'newPkgPath', so this should just be 'newDir'. The pkg is implied.
Done
renamingEdits, err := updatePackageDeclsAndImports(ctx, s, f, newName, newPath)Nit: 'renamingEdits' is unnecessarily long, since the function name is 'renamePackage'. How about just 'edits'?
Done
renamingEdits, err = updateModFiles(ctx, s, f.URI().DirPath(), newPkgDir, renamingEdits)No need to return renamingEdits here.
How about just passing in the edits map to both updatePackageDeclsAndImports, and to updateModFiles?
Done
moveToInternal := snapshot.Options().PackageMove && (strings.Contains(string(newName), "internal") || strings.Contains(string(newPath), "internal"))This doesn't look quite right.
1. validating whether a renaming is valid shouldn't depend on the PackageMove option: at this point, we've already handled PackageMove, and whether the move is valid is independent of how we got to it.
2. The condition on importers should be a function of dep path and import path. We shouldn't allow the move if the new import is invalid. See `metadata.IsValidImport`.
metadata.IsValidImport has several TODOs saying that the functionality is incorrect, so maybe I should fix that in a separate CL.
I've handled the case where we are moving from a non-internal to an internal package so that we detect illegal internal imports.
func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Handle, newName string) (newPkgDir string, newPkgName PackageName, newPkgPath PackagePath, err error) {We can only perform the package rename if curPkg holds ALL Go files in its directory. If there are any other Go files present, the rename may result in a broken build.
We can do better by trying to find builds that cover all go files, but for now let's just be conservative.
Sorry I'm not sure if I understand - are you describing a scenario where we have files for package a located in "root/a" and files for package a also located in "root/b"?
"Directory or file already exists at this path; return an error."?
Right thanks, it could conflict with an existing file too
if !strings.Contains(newName, string(os.PathSeparator)) {I don't understand this: doesn't PrepareRename prompt with the package path?
If newName is not an identifier, is it a directory or package path? I thought the latter.
I think the logic here needs to check:
1. That modPath+"/" is a prefix of the new path.
2. That filepath.Join(modDir, filepath.FromSlash(strings.TrimPrefix(newName, modPath+"/"))) is a valid path.
3. That the new path creates a new package.
I should have used "/" instead of os.PathSeparator since forward slash is always used in package paths. But anyways, this check is redundant. It was meant to catch cases like "$$$" which are not valid package identifiers but do pass the fs.ValidPath check. This will actually be rejected when we check case (2) a few lines below.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 2 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
Madeline KalilThe test failures look related.
Madeline KalilYeah, windows :( trying to figure it out
Also still need to add tests for import cycles and nested directories that don't exist
Turns out you don't need to explicitly create directories that don't exist, you can just use RenameFile in a DocumentChange and it will create the directories. Still need to add a test for an import cycle
Actually, I don't believe it is possible to create an import cycle via a package move alone because we don't allow merging packages.
if !strings.Contains(newName, string(os.PathSeparator)) {Madeline KalilI don't understand this: doesn't PrepareRename prompt with the package path?
If newName is not an identifier, is it a directory or package path? I thought the latter.
I think the logic here needs to check:
1. That modPath+"/" is a prefix of the new path.
2. That filepath.Join(modDir, filepath.FromSlash(strings.TrimPrefix(newName, modPath+"/"))) is a valid path.
3. That the new path creates a new package.
I should have used "/" instead of os.PathSeparator since forward slash is always used in package paths. But anyways, this check is redundant. It was meant to catch cases like "$$$" which are not valid package identifiers but do pass the fs.ValidPath check. This will actually be rejected when we check case (2) a few lines below.
Done
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- Madeline Kalil
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 1 comment![]( "Open in Gerrit")
changes = append(changes,
protocol.DocumentChangeRename(
protocol.URIFromPath(oldDir),
protocol.URIFromPath(newPkgDir)))Madeline KalilSo, this actually looks wrong.
What if we're renaming a/b, and there's an a/b/c. Doesn't this implicitly move the 'c' directory as well?
What if we're renaming a/b to a, and there are no `go` files in `a`. Can't we support that, by just moving the files?
I think we need to...
1. Check that there are no go files in newPkgDir (or newPkgDir is missing).
2. Create newPkgDir if it doesn't exist.
3. Move each file from oldPkgDir.
4. Delete oldPkgDir if it's now empty.
Yeah thinking about it more, it doesn't make sense that nested packages would be also moved. But there is a test TestRenamePackage_NestedModule that asserts that renaming package "foo" in foo/ to "foox" will result in renaming the file "foo/bar/bar.go" which is in package "bar" to "foox/bar/bar.go". Should we change this test expectation?
Also, not sure about the best way to implement (4) so I'll ask you tomorrow.
For (4), looks like we can use a DeleteFile operation to remove the old directory, because it will only delete the directory if it is empty (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#deleteFileOptions). I also updated the marker test's apply edit functionality to match what LSP says.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 4 comments![]( "Open in Gerrit")
var (
newPkgName PackageName
newPkgPath PackagePath
err error
)
if newPkgDir, newPkgName, newPkgPath, err = checkPackageRename(snapshot.Options(), pkg, f, newName); err != nil {Madeline KalilLooks like these can now be declared with a short variable declaration.
newPkgDir, newPkgName, newPkgPath, err := ...
if err != nil {
...
}
I think we still need them declared like this since we use newPkgDir at the end of this function to move the files to the new package directory, and only if inPackageName.
Oh! Yep, I just missed that (I can't count...)
changes = append(changes,
protocol.DocumentChangeRename(
protocol.URIFromPath(oldDir),
protocol.URIFromPath(newPkgDir)))Madeline KalilSo, this actually looks wrong.
What if we're renaming a/b, and there's an a/b/c. Doesn't this implicitly move the 'c' directory as well?
What if we're renaming a/b to a, and there are no `go` files in `a`. Can't we support that, by just moving the files?
I think we need to...
1. Check that there are no go files in newPkgDir (or newPkgDir is missing).
2. Create newPkgDir if it doesn't exist.
3. Move each file from oldPkgDir.
4. Delete oldPkgDir if it's now empty.
Madeline KalilYeah thinking about it more, it doesn't make sense that nested packages would be also moved. But there is a test TestRenamePackage_NestedModule that asserts that renaming package "foo" in foo/ to "foox" will result in renaming the file "foo/bar/bar.go" which is in package "bar" to "foox/bar/bar.go". Should we change this test expectation?
Also, not sure about the best way to implement (4) so I'll ask you tomorrow.
For (4), looks like we can use a DeleteFile operation to remove the old directory, because it will only delete the directory if it is empty (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#deleteFileOptions). I also updated the marker test's apply edit functionality to match what LSP says.
Sorry for the churn: chatted about this with Alan and he argued that *either one* of these behaviors is viable: you may want to move subpackages, you may not.
A great opportunity for dialog support. In the meantime, want to paraneterize this with a (const) bool?
func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Handle, newName string) (newPkgDir string, newPkgName PackageName, newPkgPath PackagePath, err error) {Madeline KalilWe can only perform the package rename if curPkg holds ALL Go files in its directory. If there are any other Go files present, the rename may result in a broken build.
We can do better by trying to find builds that cover all go files, but for now let's just be conservative.
Sorry I'm not sure if I understand - are you describing a scenario where we have files for package a located in "root/a" and files for package a also located in "root/b"?
No, what I mean is that there may be a foo_windows.go in the directory, but that file is not present in the curPkg set of compiled go files (because our build is, say, linux). Therefore, the calculations of callers will exclude the windows file, and we'll get a broken build on windows after moving files.
// Directory is not empty. If "Recursive" is true, we delete
// every file in the folder. Otherwise, we continue without
// deleting the directory.Nice! I didn't even know about this option.
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 2 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
2 comments
changes = append(changes,
protocol.DocumentChangeRename(
protocol.URIFromPath(oldDir),
protocol.URIFromPath(newPkgDir)))Madeline KalilSo, this actually looks wrong.
What if we're renaming a/b, and there's an a/b/c. Doesn't this implicitly move the 'c' directory as well?
What if we're renaming a/b to a, and there are no `go` files in `a`. Can't we support that, by just moving the files?
I think we need to...
1. Check that there are no go files in newPkgDir (or newPkgDir is missing).
2. Create newPkgDir if it doesn't exist.
3. Move each file from oldPkgDir.
4. Delete oldPkgDir if it's now empty.
Madeline KalilYeah thinking about it more, it doesn't make sense that nested packages would be also moved. But there is a test TestRenamePackage_NestedModule that asserts that renaming package "foo" in foo/ to "foox" will result in renaming the file "foo/bar/bar.go" which is in package "bar" to "foox/bar/bar.go". Should we change this test expectation?
Also, not sure about the best way to implement (4) so I'll ask you tomorrow.
Robert FindleyFor (4), looks like we can use a DeleteFile operation to remove the old directory, because it will only delete the directory if it is empty (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#deleteFileOptions). I also updated the marker test's apply edit functionality to match what LSP says.
Sorry for the churn: chatted about this with Alan and he argued that *either one* of these behaviors is viable: you may want to move subpackages, you may not.
A great opportunity for dialog support. In the meantime, want to paraneterize this with a (const) bool?
Done
moveToInternal := snapshot.Options().PackageMove && (strings.Contains(string(newName), "internal") || strings.Contains(string(newPath), "internal"))Madeline KalilThis doesn't look quite right.
1. validating whether a renaming is valid shouldn't depend on the PackageMove option: at this point, we've already handled PackageMove, and whether the move is valid is independent of how we got to it.
2. The condition on importers should be a function of dep path and import path. We shouldn't allow the move if the new import is invalid. See `metadata.IsValidImport`.
metadata.IsValidImport has several TODOs saying that the functionality is incorrect, so maybe I should fix that in a separate CL.
I've handled the case where we are moving from a non-internal to an internal package so that we detect illegal internal imports.
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Handle, newName string) (newPkgDir string, newPkgName PackageName, newPkgPath PackagePath, err error) {Madeline KalilWe can only perform the package rename if curPkg holds ALL Go files in its directory. If there are any other Go files present, the rename may result in a broken build.
We can do better by trying to find builds that cover all go files, but for now let's just be conservative.
Robert FindleySorry I'm not sure if I understand - are you describing a scenario where we have files for package a located in "root/a" and files for package a also located in "root/b"?
No, what I mean is that there may be a foo_windows.go in the directory, but that file is not present in the curPkg set of compiled go files (because our build is, say, linux). Therefore, the calculations of callers will exclude the windows file, and we'll get a broken build on windows after moving files.
Got it, thanks. Done
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 10 comments![]( "Open in Gerrit")
if !(strings.HasPrefix(newPkgDir, oldPkgDir)) {Unnecessary parens.
if !(strings.HasPrefix(newPkgDir, oldPkgDir)) {Here also, don't use HasPrefix to check lexical containment of directories.
You may want to use the gopls/internal/util/pathutil.InDir helper.
if !(string(mp.PkgPath)+"/" == string(oldPkgPath)+"/") {`if mp.PkgPath != oldPkgPath`
But also: what about test variants? (the x_test package)? Do we have a test for that?
if !(strings.Contains(string(rdep.PkgPath), "internal")) && strings.Contains(string(newPath), "internal") {
// We do not allow moving packages across the module boundary, so mp.Module.Dir is the module directory
// for the new package path.
if mp.Module.Dir != rdep.Module.Dir {
return fmt.Errorf("invalid: package move would result in illegal internal import")
}
}Can you explain this condition? I don't think this is accurate to the rules for internal imports, and internal doesn't really have anything to do with module dirs.
Also, strings.Contains is not correct: "myinternal" contains the string "internal", but is not an internal directory.
a/b/c can import from a/internal or b/internal, but not a/d/internal.
https://go.dev/doc/go1.4#internalpackages
We discuss metadata.IsValidImport: that function has a couple bugs, but we should still use it (and then fix it in a follow-up). The bugs are historical oversights; I noted them as we refactored.
return f.URI().DirPath(), PackageName(path.Base(newName)), curPkg.Metadata().PkgPath, nilThis isn't correct in the case where curPkg.Metadata().Name does not match the directory base name. Just return curPkg.Metadata().Name.
And I hope that this doesn't result in any edits! Is that guaranteed to be the case?
newPkgDir = filepath.Join(filepath.Dir(f.URI().DirPath()), newName)I recommend factoring out `oldDir`.
newPkgName = PackageName(newName)Nit, move this below the Stat, closer to its usage.
isValidPkgPath := fs.ValidPath(newName)I don't think this is quite right. fs.ValidPath is about file paths. Should you want until below, when you construct the newPkgDir, and pass that to ValidPath?
if !(strings.HasPrefix(newName, curModPath)) {1. Unnecessary parens.
2. This isn't right: need to check newName+"/" and curModPath+"/" to avoid the case of curModPath = module.com/mymodule and newName = module.com/mymodulewithnoslash
newPkgDir = filepath.Join(modDir, newPathAfterMod)This looks wrong. You're joining a directory and an import path segment. This logic should probably operate entirely on dirs.
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 10 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
10 comments
if !(strings.HasPrefix(newPkgDir, oldPkgDir)) {Madeline KalilUnnecessary parens.
Done
if !(strings.HasPrefix(newPkgDir, oldPkgDir)) {Here also, don't use HasPrefix to check lexical containment of directories.
You may want to use the gopls/internal/util/pathutil.InDir helper.
Done
if !(string(mp.PkgPath)+"/" == string(oldPkgPath)+"/") {`if mp.PkgPath != oldPkgPath`
But also: what about test variants? (the x_test package)? Do we have a test for that?
The renaming of package clauses in the test variants is handles above these lines around L1113. The renaming of the files/directories themselves is handled in the outer Rename function as we pass the test package back and move those files individually (L545). There's a test in packagedecl and the TestRenamePackage_Tests and TestRenamePackage_TestVariant in rename_test.go to verify the behavior.
if !(strings.Contains(string(rdep.PkgPath), "internal")) && strings.Contains(string(newPath), "internal") {
// We do not allow moving packages across the module boundary, so mp.Module.Dir is the module directory
// for the new package path.
if mp.Module.Dir != rdep.Module.Dir {
return fmt.Errorf("invalid: package move would result in illegal internal import")
}
}Can you explain this condition? I don't think this is accurate to the rules for internal imports, and internal doesn't really have anything to do with module dirs.
Also, strings.Contains is not correct: "myinternal" contains the string "internal", but is not an internal directory.a/b/c can import from a/internal or b/internal, but not a/d/internal.
https://go.dev/doc/go1.4#internalpackagesWe discuss metadata.IsValidImport: that function has a couple bugs, but we should still use it (and then fix it in a follow-up). The bugs are historical oversights; I noted them as we refactored.
Oops yeah, makes sense that we need to check for "/internal/" not just "internal".
My understanding is that if you have some package in a and some package in b/internal, a can only import b/internal if they're in the same module. metadata.IsValidImport verifies the same thing by checking the package path though, so this check isn't necessary anymore
return f.URI().DirPath(), PackageName(path.Base(newName)), curPkg.Metadata().PkgPath, nilThis isn't correct in the case where curPkg.Metadata().Name does not match the directory base name. Just return curPkg.Metadata().Name.
And I hope that this doesn't result in any edits! Is that guaranteed to be the case?
Yep I have a test case in packagedecl.txt for when the specified package path is the same.
newPkgDir = filepath.Join(filepath.Dir(f.URI().DirPath()), newName)I recommend factoring out `oldDir`.
Done
newPkgName = PackageName(newName)Nit, move this below the Stat, closer to its usage.
Done
isValidPkgPath := fs.ValidPath(newName)I don't think this is quite right. fs.ValidPath is about file paths. Should you want until below, when you construct the newPkgDir, and pass that to ValidPath?
This is mainly checking if the path is a valid utf8 string, but it does make more sense to check the directory (file path) although we have to remove the slash prefix before passing it to fs.ValidPath
if !(strings.HasPrefix(newName, curModPath)) {1. Unnecessary parens.
2. This isn't right: need to check newName+"/" and curModPath+"/" to avoid the case of curModPath = module.com/mymodule and newName = module.com/mymodulewithnoslash
Thanks! Added a test as well
newPkgDir = filepath.Join(modDir, newPathAfterMod)This looks wrong. You're joining a directory and an import path segment. This logic should probably operate entirely on dirs.
Sorry I'm not sure how else to piece together the new package directory.
Since the module is not allowed to change, the new package directory should be the module directory joined with the part of the package path that comes after the module name.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
func isAllSameBuild(pkg *cache.Package, dir string) (bool, error) {I don't think this is correct, working on it
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
func isAllSameBuild(pkg *cache.Package, dir string) (bool, error) {I don't think this is correct, working on it
Will implement this in a follow up
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |