[tools] imports: prefer math/rand/v2 over math/rand
Peter Weinberger (Gerrit)
Peter Weinberger has uploaded the change for review![]( "Open in Gerrit")
Commit message
imports: prefer math/rand/v2 over math/rand
When a program refers to 'rand.Rand()' without an import statement,
import math/rand/v2 is the proper answer. (Well, it's the better answer.
The right answer depends on the version of go the user asks for.)
Change diff
diff --git a/gopls/internal/test/integration/misc/imports_test.go b/gopls/internal/test/integration/misc/imports_test.go
index d8f453e..d46b326 100644
--- a/gopls/internal/test/integration/misc/imports_test.go
+++ b/gopls/internal/test/integration/misc/imports_test.go
@@ -81,6 +81,33 @@
})
}
+func TestIssue66407(t *testing.T) {
+ const files = `
+-- go.mod --
+module foo
+go 1.21
+-- a.go --
+package foo
+
+func f(x float64) float64 {
+ return x + rand.Rand()
+}
+`
+ WithOptions(Modes(Default)).
+ Run(t, files, func(t *testing.T, env *Env) {
+ env.OpenFile("a.go")
+ was := env.BufferText("a.go")
+ env.OrganizeImports("a.go")
+ env.SaveBuffer("a.go")
+ is := env.BufferText("a.go")
+ env.Await(Diagnostics(ForFile("a.go")))
+ diff := compare.Text(was, is)
+ if !strings.Contains(diff, "rand/v2") {
+ t.Errorf("expected rand/v2, got %q", diff)
+ }
+ })
+}
+
func TestVim1(t *testing.T) {
const vim1 = `package main
diff --git a/internal/imports/fix.go b/internal/imports/fix.go
index 69e2ad5..44c14db 100644
--- a/internal/imports/fix.go
+++ b/internal/imports/fix.go
@@ -301,6 +301,17 @@
return nil
}
+// if there is a trailing major version, remove it
+func withoutVersion(nm string) string {
+ if v := path.Base(nm); v[0] == 'v' {
+ if _, err := strconv.Atoi(v[1:]); err == nil {
+ xnm := nm[:len(nm)-len(v)-1]
+ return xnm
+ }
+ }
+ return nm
+}
+
// importIdentifier returns the identifier that imp will introduce. It will
// guess if the package name has not been loaded, e.g. because the source
// is not available.
@@ -310,7 +321,7 @@
}
known := p.knownPackages[imp.ImportPath]
if known != nil && known.name != "" {
- return known.name
+ return withoutVersion(known.name)
}
return ImportPathToAssumedName(imp.ImportPath)
}
@@ -1057,6 +1068,17 @@
if err != nil {
return err
}
+ localbase := func(nm string) string {
+ ans := path.Base(nm)
+ if ans[0] == 'v' {
+ if _, err := strconv.Atoi(ans[1:]); err == nil {
+ ix := strings.LastIndex(nm, ans)
+ more := path.Base(nm[:ix])
+ ans = path.Join(more, ans)
+ }
+ }
+ return ans
+ }
add := func(pkg string) {
// Prevent self-imports.
if path.Base(pkg) == pass.f.Name.Name && filepath.Join(goenv["GOROOT"], "src", pkg) == pass.srcDir {
@@ -1065,13 +1087,13 @@
exports := symbolNameSet(stdlib.PackageSymbols[pkg])
pass.addCandidate(
&ImportInfo{ImportPath: pkg},
- &packageInfo{name: path.Base(pkg), exports: exports})
+ &packageInfo{name: localbase(pkg), exports: exports}) // PJW: Base is not right (math/v2)
}
for left := range refs {
if left == "rand" {
- // Make sure we try crypto/rand before math/rand.
+ // Make sure we try crypto/rand before math/rand as both have Int()
add("crypto/rand")
- add("math/rand")
+ add("math/rand/v2")
continue
}
for importPath := range stdlib.PackageSymbols {
Change information
- M gopls/internal/test/integration/misc/imports_test.go
- M internal/imports/fix.go
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. |
Kevin Burke (Gerrit)
Kevin Burke added 1 comment![]( "Open in Gerrit")
Fixes: golang.org/go#66407Should be just golang/go#66407 or "Fixes #66407"
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger added 1 comment![]( "Open in Gerrit")
Should be just golang/go#66407 or "Fixes #66407"
- Kevin Burke
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Kevin Burke
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger added 1 comment![]( "Open in Gerrit")
Peter WeinbergerShould be just golang/go#66407 or "Fixes #66407"
yes
Done
Related details
- Kevin Burke
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")
env.SaveBuffer("a.go")Is this Save necessary? I don't see why it would be.
env.Await(Diagnostics(ForFile("a.go")))Shouldn't the diagnostics go away after a call to OrganizeImports?
I suspect that this should be `env.AfterChange(NoDiagnostics())`, which asserts that once gopls is done diagnosing the OrganizeImports changes, there are no diagnostics for any file.
Right now, it looks like this is a racy assertion that there *are* diagnostics, which only succeeds because gopls hasn't yet processed the imports change.
if v := path.Base(nm); v[0] == 'v' {Nit: better to make this a pure function and not worry about it: `len(v) > 0 && v[0] == 'v'`.
```suggestion
if v := path.Base(nm); len(v) > 0 && v[0] == 'v' {
```
return path.Base(xnm)Why are we returning a base path here? Below, we're returning nm unmodified. Is this function just meant to equate rand/v2 and rand? I.e. strings with exactly one '/'? If so, I don't think it should be necessary -- see below comment in importIdentifier.
return withoutVersion(known.name)packageInfo.name is documented to be the real package name, if known. Therefore, we shouldn't have to call withoutVersion here. Did you observe that this was necessary? If so, there must be a bug elsewhere.
ans := path.Base(nm)
if ans[0] == 'v' {
if _, err := strconv.Atoi(ans[1:]); err == nil {
ix := strings.LastIndex(nm, ans)
more := path.Base(nm[:ix])
ans = path.Join(more, ans)
}
}
return ansUse path.Split, and handle the degenerate case of an empty string.
&packageInfo{name: localbase(pkg), exports: exports}) // PJW: Base is not right (math/v2)Is this true? I thought localbase fixed that.
name: "cryptorand_preferred_easy_impossible",I don't understand this test name. Is it supposed to assert that crypto/rand is preferred? Clearly, it is not! Or is it saying that it would be nice, but impossible, to prefer crypto/rand?
var _ = rand.NewZipfPlease add a test where we prefer the old, deprecated rand (by using rand.Read, which doesn't exist in rand/v2, for example).
Related details
- Kevin Burke
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Kevin Burke
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger added 9 comments![]( "Open in Gerrit")
Is this Save necessary? I don't see why it would be.
Done
Shouldn't the diagnostics go away after a call to OrganizeImports?
I suspect that this should be `env.AfterChange(NoDiagnostics())`, which asserts that once gopls is done diagnosing the OrganizeImports changes, there are no diagnostics for any file.
Right now, it looks like this is a racy assertion that there *are* diagnostics, which only succeeds because gopls hasn't yet processed the imports change.
Well there's a problem. go.mod has go1.21, and rand/v2 is only available for g1.22. But if go.mod is changed to go1.22, then tests fail with
runner.go:208: closing the sandbox: error(s) cleaning sandbox: cleaning modcache: go command failed (stdout: ) (stderr: go: downloading go1.22 (darwin/arm64)
go: download go1.22 for darwin/arm64: toolchain not available
So in this particular test, we expect diagnostics.
I don't see what else to do without A) knowing what version of Go the user's code expects, and B) the dependence of stdlib symbols on go version.
Nit: better to make this a pure function and not worry about it: `len(v) > 0 && v[0] == 'v'`.
```suggestion
if v := path.Base(nm); len(v) > 0 && v[0] == 'v' {
```
path.Base() never returns "", but the change avoids readers having to know that.
if len(v) < len(nm) {Peter WeinbergerUse path.Split?
I do not see where path.Split helps. path.Base removes trailing /s but path.Split does not.
Why are we returning a base path here? Below, we're returning nm unmodified. Is this function just meant to equate rand/v2 and rand? I.e. strings with exactly one '/'? If so, I don't think it should be necessary -- see below comment in importIdentifier.
path.Base does canonicalization, so no one has to try to prove that there aren't repeated slashes in an inconvenient place in nm.
packageInfo.name is documented to be the real package name, if known. Therefore, we shouldn't have to call withoutVersion here. Did you observe that this was necessary? If so, there must be a bug elsewhere.
I don't know what "real package name" means. The test fails without this code. If the name is "math/rand/v2", the code needs to return "math/rand".
ans := path.Base(nm)
if ans[0] == 'v' {
if _, err := strconv.Atoi(ans[1:]); err == nil {
ix := strings.LastIndex(nm, ans)
more := path.Base(nm[:ix])
ans = path.Join(more, ans)
}
}
return ansUse path.Split, and handle the degenerate case of an empty string.
It's better not to have to worry about trailing slashes. mod.go:724 is, I believe, incorrect for that reason.
&packageInfo{name: localbase(pkg), exports: exports}) // PJW: Base is not right (math/v2)Is this true? I thought localbase fixed that.
The comment meant that path.Base() is not correct. Removed.
Please add a test where we prefer the old, deprecated rand (by using rand.Read, which doesn't exist in rand/v2, for example).
Read would give us crypt/rand. But Int63 works. I added such a test in fix_test.go right next to the test for the new behavior.
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 5 comments![]( "Open in Gerrit")
env.Await(Diagnostics(ForFile("a.go")))Peter WeinbergerShouldn't the diagnostics go away after a call to OrganizeImports?
I suspect that this should be `env.AfterChange(NoDiagnostics())`, which asserts that once gopls is done diagnosing the OrganizeImports changes, there are no diagnostics for any file.
Right now, it looks like this is a racy assertion that there *are* diagnostics, which only succeeds because gopls hasn't yet processed the imports change.
Well there's a problem. go.mod has go1.21, and rand/v2 is only available for g1.22. But if go.mod is changed to go1.22, then tests fail with
runner.go:208: closing the sandbox: error(s) cleaning sandbox: cleaning modcache: go command failed (stdout: ) (stderr: go: downloading go1.22 (darwin/arm64)
go: download go1.22 for darwin/arm64: toolchain not available
So in this particular test, we expect diagnostics.I don't see what else to do without A) knowing what version of Go the user's code expects, and B) the dependence of stdlib symbols on go version.
Do you mean that this test fails on the Go 1.21 builders? If that's the case, you can just add a `testenv.NeedsGo1Point(t, 22)` to only run this test on Go 1.22+, which is fine.
In either case, we always try to use env.AfterChange rather than env.Await, since the latter is unbounded. If we know that the expected diagnostics should be present after all pending changes have been diagnosed, AfterChange is the API to use.
if v := path.Base(nm); v[0] == 'v' {Peter WeinbergerNit: better to make this a pure function and not worry about it: `len(v) > 0 && v[0] == 'v'`.
```suggestion
if v := path.Base(nm); len(v) > 0 && v[0] == 'v' {
```
path.Base() never returns "", but the change avoids readers having to know that.
Aha, I thought it would return "" for the empty input. Thanks.
return withoutVersion(known.name)Peter WeinbergerpackageInfo.name is documented to be the real package name, if known. Therefore, we shouldn't have to call withoutVersion here. Did you observe that this was necessary? If so, there must be a bug elsewhere.
I don't know what "real package name" means. The test fails without this code. If the name is "math/rand/v2", the code needs to return "math/rand".
"real package name" is quoted from the doc string for packageInfo. I assumed it meant the package name defined by the `package <name>` clause in each package file. But that appears to be an inaccurate comment. Looking at the code, I see that in addStdLibCandidates, it is set to path.Base.
I think we should fix the code to record the correct package name for known package. Can you see if that is feasible?
ans := path.Base(nm)
if ans[0] == 'v' {
if _, err := strconv.Atoi(ans[1:]); err == nil {
ix := strings.LastIndex(nm, ans)
more := path.Base(nm[:ix])
ans = path.Join(more, ans)
}
}
return ansPeter WeinbergerUse path.Split, and handle the degenerate case of an empty string.
It's better not to have to worry about trailing slashes. mod.go:724 is, I believe, incorrect for that reason.
Ack.
var _ = rand.NewZipfPeter WeinbergerPlease add a test where we prefer the old, deprecated rand (by using rand.Read, which doesn't exist in rand/v2, for example).
Read would give us crypt/rand. But Int63 works. I added such a test in fix_test.go right next to the test for the new behavior.
I think this test wasn't added? I don't see a diff between the patchsets.
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger added 3 comments![]( "Open in Gerrit")
env.Await(Diagnostics(ForFile("a.go")))Peter WeinbergerShouldn't the diagnostics go away after a call to OrganizeImports?
I suspect that this should be `env.AfterChange(NoDiagnostics())`, which asserts that once gopls is done diagnosing the OrganizeImports changes, there are no diagnostics for any file.
Right now, it looks like this is a racy assertion that there *are* diagnostics, which only succeeds because gopls hasn't yet processed the imports change.
Robert FindleyWell there's a problem. go.mod has go1.21, and rand/v2 is only available for g1.22. But if go.mod is changed to go1.22, then tests fail with
runner.go:208: closing the sandbox: error(s) cleaning sandbox: cleaning modcache: go command failed (stdout: ) (stderr: go: downloading go1.22 (darwin/arm64)
go: download go1.22 for darwin/arm64: toolchain not available
So in this particular test, we expect diagnostics.I don't see what else to do without A) knowing what version of Go the user's code expects, and B) the dependence of stdlib symbols on go version.
Do you mean that this test fails on the Go 1.21 builders? If that's the case, you can just add a `testenv.NeedsGo1Point(t, 22)` to only run this test on Go 1.22+, which is fine.
In either case, we always try to use env.AfterChange rather than env.Await, since the latter is unbounded. If we know that the expected diagnostics should be present after all pending changes have been diagnosed, AfterChange is the API to use.
I added some explanatory comments. a.go will have an error because its module is go1.21, which is too early for math/rand/v2, but is will get the 'right' import. (remembering that imports has no way of managing go version, but it would get an error without the import too) b.go will get just mat/rand because it uses a function from that package.
And changed to AfterChange()
return withoutVersion(known.name)Peter WeinbergerpackageInfo.name is documented to be the real package name, if known. Therefore, we shouldn't have to call withoutVersion here. Did you observe that this was necessary? If so, there must be a bug elsewhere.
Robert FindleyI don't know what "real package name" means. The test fails without this code. If the name is "math/rand/v2", the code needs to return "math/rand".
"real package name" is quoted from the doc string for packageInfo. I assumed it meant the package name defined by the `package <name>` clause in each package file. But that appears to be an inaccurate comment. Looking at the code, I see that in addStdLibCandidates, it is set to path.Base.
I think we should fix the code to record the correct package name for known package. Can you see if that is feasible?
I think that should be a separate issue and CL.
var _ = rand.NewZipfPeter WeinbergerPlease add a test where we prefer the old, deprecated rand (by using rand.Read, which doesn't exist in rand/v2, for example).
Robert FindleyRead would give us crypt/rand. But Int63 works. I added such a test in fix_test.go right next to the test for the new behavior.
I think this test wasn't added? I don't see a diff between the patchsets.
- 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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Robert Findley
| Code-Review | +2 |
2 comments
// but we have no way of figuring out what the user is usingI think we should promote this to a TODO: we could certainly capture the toolchain version when initializing the process env, and with all the problems we've had with forward/backward compatibility, it's probably something we want to do. WDYT?
// Make sure we try crypto/rand before math/rand as both have Int()
add("crypto/rand")
// if the user's no later than go1.21, this should be "math/rand"
// but we have no way of figuring out what the user is using
add("math/rand/v2")Based on the comment on line 1098, the order of calls to addCandidate influence the chosen imports. So perhaps add
// ...and try math/rand/v2 before math/rand, for overlapping APIs.
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Peter Weinberger
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger added 2 comments![]( "Open in Gerrit")
// but we have no way of figuring out what the user is usingI think we should promote this to a TODO: we could certainly capture the toolchain version when initializing the process env, and with all the problems we've had with forward/backward compatibility, it's probably something we want to do. WDYT?
That would be a fairly substantial change, but I added the comment.
// Make sure we try crypto/rand before math/rand as both have Int()
add("crypto/rand")
// if the user's no later than go1.21, this should be "math/rand"
// but we have no way of figuring out what the user is using
add("math/rand/v2")Based on the comment on line 1098, the order of calls to addCandidate influence the chosen imports. So perhaps add
// ...and try math/rand/v2 before math/rand, for overlapping APIs.
I changed the comment.
Related details
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: internal/imports/fix.go
Insertions: 3, Deletions: 1.
@@ -1010,8 +1010,10 @@
// already know the view type.
if len(e.Env["GOMOD"]) == 0 && len(e.Env["GOWORK"]) == 0 {
e.resolver = newGopathResolver(e)
+ } else if r, err := newModuleResolver(e, e.ModCache); err != nil {
+ e.resolverErr = err
} else {
- e.resolver, e.resolverErr = newModuleResolver(e, e.ModCache)
+ e.resolver = Resolver(r)
}
}
@@ -1095,10 +1097,12 @@
}
for left := range refs {
if left == "rand" {
- // Make sure we try crypto/rand before math/rand as both have Int()
+ // Make sure we try crypto/rand before any version of math/rand as both have Int()
+ // and our policy is to recommend crypto
add("crypto/rand")
// if the user's no later than go1.21, this should be "math/rand"
// but we have no way of figuring out what the user is using
+ // TODO: investigate using the toolchain version to disambiguate in the stdlib
add("math/rand/v2")
continue
}
```
Change information
imports: prefer math/rand/v2 over math/rand
When a program refers to 'rand.Float64()' without an import statement,
import math/rand/v2 is the better answer.
- M gopls/internal/test/integration/misc/imports_test.go
- M internal/imports/fix.go
- M internal/imports/fix_test.go
Code-Review: +2 by Robert Findley
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |