[tools] go/analysis/passes/stdversion: suppress synctest warning
Madeline Kalil (Gerrit)
Madeline Kalil has uploaded the change for review![]( "Open in Gerrit")
Commit message
go/analysis/passes/stdversion: suppress synctest warning
WIP
Change diff
diff --git a/go/analysis/passes/stdversion/stdversion.go b/go/analysis/passes/stdversion/stdversion.go
index 3147219..418b917 100644
--- a/go/analysis/passes/stdversion/stdversion.go
+++ b/go/analysis/passes/stdversion/stdversion.go
@@ -99,6 +99,16 @@
if obj, ok := pass.TypesInfo.Uses[n]; ok && obj.Pkg() != nil {
disallowed := disallowedSymbols(obj.Pkg(), fileVersion)
if minVersion, ok := disallowed[origin(obj)]; ok {
+ if obj.Pkg().Path() == "testing/synctest" && fileVersion == "go1.24" {
+ // Special case: some files that use synctest have
+ // complex build tags that ensure that the synctest
+ // package is available but do not necessarily imply
+ // that the go version is go1.25. We suppress the
+ // error to avoid false positives in this case by
+ // relaxing the check to go1.24.
+ // See golang/go#75367.
+ break
+ }
noun := "module"
if fileVersion != pkgVersion {
noun = "file"
Change information
- M go/analysis/passes/stdversion/stdversion.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. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
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. |
Alan Donovan (Gerrit)
Alan Donovan added 2 comments![]( "Open in Gerrit")
if obj.Pkg().Path() == "testing/synctest" && fileVersion == "go1.24" {I think this want to be implemented in TooNewStdSymbols so that Completion also benefits from it.
-min_go_command=go1.24You may need -max_go_command=go1.25 as IIRC the synctest GOEXPERIMENT is no more, since it is in the mainline.
(Unfortunately I couldn't reproduce the issue we saw on your machine whereby the go command only returns ReleaseTags up to go1.23.)
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
if obj.Pkg().Path() == "testing/synctest" && fileVersion == "go1.24" {I think this want to be implemented in TooNewStdSymbols so that Completion also benefits from it.
Done, although now we get completions from synctest in files with version >= go1.24, is that the behavior we want? Suppressing a potentially false positive error makes sense to me, but suggesting a package that might not actually be available seems less desirable.
You may need -max_go_command=go1.25 as IIRC the synctest GOEXPERIMENT is no more, since it is in the mainline.
(Unfortunately I couldn't reproduce the issue we saw on your machine whereby the go command only returns ReleaseTags up to go1.23.)
Thanks for trying to reproduce the issue. Unfortunately it's preventing me from running this test locally - the marker test framework just skips it.
Related details
- Alan Donovan
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
- Alan Donovan
- 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")
| Commit-Queue | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 3 comments![]( "Open in Gerrit")
if obj.Pkg().Path() == "testing/synctest" && fileVersion == "go1.24" {Madeline KalilI think this want to be implemented in TooNewStdSymbols so that Completion also benefits from it.
Done, although now we get completions from synctest in files with version >= go1.24, is that the behavior we want? Suppressing a potentially false positive error makes sense to me, but suggesting a package that might not actually be available seems less desirable.
See my comment at toonew.go:27.
if pkg.Path() == "testing/synctest" && versions.AtLeast("go1.24", version) {These operands are the wrong way around.
Tip: In general, a function `op(x, y)` representing an asymmetric binary relation such as less, before, after, has prefix, contains, etc, should be read as `x op y`. This is the same parameter order as a method such as `x.contains(y)`.
if pkg.Path() == "testing/synctest" && versions.AtLeast("go1.24", version) {I expect there will be more items like this in future, so let's add a general introduction, and then specific detail. For example:
```
// Some symbols are accessible before their release but
// only with specific build tags unknown to us here.
// Avoid false positives in such cases.
if pkg.Path() == "testing/synctest" && versions.AtLeast(version, "go1.24") {
continue // requires go1.24 && goexperiment.synctest || go1.25
}
```
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 3 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
3 comments
if obj.Pkg().Path() == "testing/synctest" && fileVersion == "go1.24" {Madeline KalilI think this want to be implemented in TooNewStdSymbols so that Completion also benefits from it.
Alan DonovanDone, although now we get completions from synctest in files with version >= go1.24, is that the behavior we want? Suppressing a potentially false positive error makes sense to me, but suggesting a package that might not actually be available seems less desirable.
See my comment at toonew.go:27.
We are now getting synctest completions for files with go1.24 that don't have the special build tag set -- is that okay?
if pkg.Path() == "testing/synctest" && versions.AtLeast("go1.24", version) {These operands are the wrong way around.
Tip: In general, a function `op(x, y)` representing an asymmetric binary relation such as less, before, after, has prefix, contains, etc, should be read as `x op y`. This is the same parameter order as a method such as `x.contains(y)`.
Done
if pkg.Path() == "testing/synctest" && versions.AtLeast("go1.24", version) {I expect there will be more items like this in future, so let's add a general introduction, and then specific detail. For example:
```
// Some symbols are accessible before their release but
// only with specific build tags unknown to us here.
// Avoid false positives in such cases.
if pkg.Path() == "testing/synctest" && versions.AtLeast(version, "go1.24") {
continue // requires go1.24 && goexperiment.synctest || go1.25
}
```
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 2 comments![]( "Open in Gerrit")
if obj.Pkg().Path() == "testing/synctest" && fileVersion == "go1.24" {Madeline KalilI think this want to be implemented in TooNewStdSymbols so that Completion also benefits from it.
Alan DonovanDone, although now we get completions from synctest in files with version >= go1.24, is that the behavior we want? Suppressing a potentially false positive error makes sense to me, but suggesting a package that might not actually be available seems less desirable.
Madeline KalilSee my comment at toonew.go:27.
We are now getting synctest completions for files with go1.24 that don't have the special build tag set -- is that okay?
Ah, sorry, I had thought the operand-order issue was the cause, but I now see that the problem is intrinsic to using (effectively) a boolean result from TooNewStdSymbols: there's no way to express "don't know" without hurting either an optimistic client (such as completion) or a pessimistic one such as this analyzer.
I think this logic does ultimately belong in TooNew; the question is how. One possibility is for TooNew to return some kind of tri-state value so that callers can get one of three answers: ok, not ok, or don't know. That requires changing the map type rather awkwardly. Another approach is to pass an additional boolean to TooNew that determines what to do in the unknown cases. But since we'll want to cherrypick this change to the release branch, for now I think we should minimize the delta by doing the check in the caller (as in PS1) and leaving a TODO to push it down into TooNew at a later date.
Comment that the synctest experiment was removed in go1.26, hence the need for a maximum version too.
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 2 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
2 comments
if obj.Pkg().Path() == "testing/synctest" && fileVersion == "go1.24" {Madeline KalilI think this want to be implemented in TooNewStdSymbols so that Completion also benefits from it.
Alan DonovanDone, although now we get completions from synctest in files with version >= go1.24, is that the behavior we want? Suppressing a potentially false positive error makes sense to me, but suggesting a package that might not actually be available seems less desirable.
Madeline KalilSee my comment at toonew.go:27.
Alan DonovanWe are now getting synctest completions for files with go1.24 that don't have the special build tag set -- is that okay?
Ah, sorry, I had thought the operand-order issue was the cause, but I now see that the problem is intrinsic to using (effectively) a boolean result from TooNewStdSymbols: there's no way to express "don't know" without hurting either an optimistic client (such as completion) or a pessimistic one such as this analyzer.
I think this logic does ultimately belong in TooNew; the question is how. One possibility is for TooNew to return some kind of tri-state value so that callers can get one of three answers: ok, not ok, or don't know. That requires changing the map type rather awkwardly. Another approach is to pass an additional boolean to TooNew that determines what to do in the unknown cases. But since we'll want to cherrypick this change to the release branch, for now I think we should minimize the delta by doing the check in the caller (as in PS1) and leaving a TODO to push it down into TooNew at a later date.
Thanks, makes sense. We no longer have a completion suggestion for the symbols in synctest, although it still suggests the synctest package itself so this must be a different aspect of the completion logic.
Comment that the synctest experiment was removed in go1.26, hence the need for a maximum version too.
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
// Set max to go1.25 since the synctest experiment was removed in go1.26.I'm pretty sure this comment will terminate flag parsing. It's unfortunate that the marker test doesn't report this, but I suspect it's why tests are failing on 1.26.
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. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
// Set max to go1.25 since the synctest experiment was removed in go1.26.I'm pretty sure this comment will terminate flag parsing. It's unfortunate that the marker test doesn't report this, but I suspect it's why tests are failing on 1.26.
If you replace this FlagSet.Parse call in marker_test.go:
```
if err := test.flagSet().Parse(test.flags); err != nil {
```
by this:
```
// Parse flags after loading files, as they may have been set by the "flags" file.
set := test.flagSet()
if err := set.Parse(test.flags); err != nil {
return nil, fmt.Errorf("parsing flags: %v", err)
}
if set.NArg() > 0 {
return nil, fmt.Errorf("surplus arguments after flags: %q", set.Args())
}
```
then we won't fall into this trap again.
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 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
// Set max to go1.25 since the synctest experiment was removed in go1.26.Alan DonovanI'm pretty sure this comment will terminate flag parsing. It's unfortunate that the marker test doesn't report this, but I suspect it's why tests are failing on 1.26.
If you replace this FlagSet.Parse call in marker_test.go:
```
if err := test.flagSet().Parse(test.flags); err != nil {
```
by this:
```
// Parse flags after loading files, as they may have been set by the "flags" file.
set := test.flagSet()
if err := set.Parse(test.flags); err != nil {
return nil, fmt.Errorf("parsing flags: %v", err)
}
if set.NArg() > 0 {
return nil, fmt.Errorf("surplus arguments after flags: %q", set.Args())
}
```
then we won't fall into this trap again.
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
1 comment
noun := "module"Let's use the comment form I suggested earlier, since it explains both the pattern and the specifics:
```
// Some symbols are accessible before their release but
// only with specific build tags unknown to us here.
// Avoid false positives in such cases.
if obj.Pkg().Path() == "testing/synctest" && versions.AtLeast(fileVersion, "go1.24") {
break // requires go1.24 && goexperiment.synctest || go1.25
}
```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 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Let's use the comment form I suggested earlier, since it explains both the pattern and the specifics:
```
// Some symbols are accessible before their release but
// only with specific build tags unknown to us here.
// Avoid false positives in such cases.
if obj.Pkg().Path() == "testing/synctest" && versions.AtLeast(fileVersion, "go1.24") {
break // requires go1.24 && goexperiment.synctest || go1.25
}
```
Done
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. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
1 comment
The test failure is spurious; you can bypass the trybot.
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 TryBot-Bypass+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 submitted the change![]( "Open in Gerrit")
Change information
go/analysis/passes/stdversion: suppress synctest warning
Some files that use synctest have complex build tags
that ensure that the synctest package is available
but do not necessarily imply that the go version is
go 1.25. We suppress the error to avoid false
positives in this case by relaxing the check
to go1.24.
Fixes golang/go#75367
- M go/analysis/passes/stdversion/stdversion.go
- M gopls/internal/test/marker/marker_test.go
- A gopls/internal/test/marker/testdata/diagnostics/stdversion_synctest.txt
- M internal/mcp/conformance_test.go
Code-Review: +2 by Alan Donovan
TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI, TryBot-Bypass+1 by Madeline Kalil
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |