Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/mod/modfile: IsDirectoryPath produces a false negative on some short clean directory paths #60572

Closed
dmitshur opened this issue Jun 2, 2023 · 4 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jun 2, 2023

In #51448 (comment), Bryan wrote:

The argument . should trigger modfile.IsDirectoryPath here:
https://cs.opensource.google/go/go/+/ce427cf96128b545ae473983bafb6a0b80ecaa08:src/cmd/go/internal/modload/modfile.go;l=805-807

But it doesn't: this looks like a bug in IsDirectoryPath, which does not expect to ever need to refer to a module in the same directory (compare #34417).

For Go 1.19, we should probably fix up modfile.IsDirectoryPath. For 1.18, perhaps we should change the condition to if path == "." || modfile.IsDirectoryPath(path).

The Go 1.18 condition change happened in CL 389298. This is the tracking issue for the bug in modfile.IsDirectoryPath that we can still fix in Go 1.22 or later. (Or even in Go 1.21 if this bug fix is considered okay in the current pre-RC1 phase of the release freeze.)

I'll send a CL.

CC @bcmills, @matloob.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 2, 2023
@dmitshur dmitshur self-assigned this Jun 2, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jun 2, 2023
@gopherbot
Copy link

Change https://go.dev/cl/500335 mentions this issue: modfile: improve directory path detection and error text consistency

@gopherbot
Copy link

Change https://go.dev/cl/500355 mentions this issue: cmd: update vendored golang.org/x/mod

@gopherbot
Copy link

Change https://go.dev/cl/501016 mentions this issue: gopls: remove trailing slash from replace directory path

@dmitshur
Copy link
Contributor Author

dmitshur commented Jun 5, 2023

Writing down the compatibility notes for this bug fix.

There are known callers of IsDirectoryPath in x/mod itself and in cmd/go in the main repo, which is what I considered in scope below. For other possible callers, this is fixing a small edge case in the intended behavior (and documenting the actual behavior more accurately than before).

Compatibility

Fixing this bug doesn't change the meaning of any existing go.mod or go.work files. It makes it possible to explicitly start using relative paths as directory paths that were previously rejected, both in replace directives (in go.mod and go.work files) and in use directives (in go.work files). Doing that can only work in a Go toolchain that has the bug fix. So modules that wish to keep supporting older Go versions would need to wait before they start using the new directory paths. (CI testing with supported toolchains, e.g., see this change failing on Go 1.20.x and 1.19.x, is one of the ways users can know to avoid unintentionally breaking compatibility.)

The go mod commands don't remove trailing slashes or otherwise format directory paths: doing go mod edit -replace=example.com/module=[directory path] or go mod tidy keeps the directory path as is. The directory path today can stay arbitrary un-clean (e.g., ".././../target/dir"). The new shorter directory paths would need to be explicitly requested before showing up in a module.

The go work commands do try to do some lightweight formatting of incoming paths (via ToDirectoryPath) but only when it's not already a directory path or "." (the special case fixed only for go work in #51448). Since this fix is not decreasing the number of paths considered as directory paths, it continues to be possible to use directory paths that work today across all Go versions.

There's no effect on dependency modules because replace directives in go.mod files are ignored when a module is not main module. So even if a new version of a dependency module chooses to start using new directory paths sooner, all existing Go toolchains will not be affected when using that module as a dependency. (This is similar to how currently checking in a replace directive containing a Windows-style \ path separator makes that module not work on non-Windows systems when it's the main module, but it doesn't stop non-Windows systems using it as a dependency.)

So to me this seems like a backward compatible minor bug fix that doesn't need special treatment (i.e., teaching all supported Go versions to understand but not yet permit the new directory paths) to be rolled out.

gopherbot pushed a commit to golang/mod that referenced this issue Oct 25, 2023
An error text suggests a directory path needs to start with ./ or ../
if it's a relative path, but in reality relative paths with .\ and ..\
prefix (such as those that are used on Windows) are also accepted.

Furthermore, a relative path like ./ or ../ is fine, as are ./. and
../., but the cleaner and shorter equivalent relative paths . and ..
are reported as if they're not directory paths (even though a module
path cannot consist of nothing but dots).

Fix those inconsistencies and make IsDirectoryPath report true on "."
and ".." paths as expected, and make its documentation clear that
a path like "sub/dir", despite being a relative path, is interpreted
as a module path.

For golang/go#60572.

Change-Id: I8fa4a2c66bc83a1ccafc453b96f3bb33dc222cd1
Reviewed-on: https://go-review.googlesource.com/c/mod/+/500335
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@dmitshur dmitshur modified the milestones: Unreleased, Go1.22 Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

2 participants