gopherbot
commented
1 month ago Found new dashboard test flakes for:
#!watchflakes
default <- pkg == "golang.org/x/tools/go/analysis/unitchecker" && test == "TestVetStdlib"Open gopherbot opened 1 month ago
gopherbot
commented
1 month ago Found new dashboard test flakes for:
#!watchflakes
default <- pkg == "golang.org/x/tools/go/analysis/unitchecker" && test == "TestVetStdlib"Found new dashboard test flakes for:
#!watchflakes
default <- pkg == "golang.org/x/tools/go/analysis/unitchecker" && test == "TestVetStdlib"Change https://go.dev/cl/603515 mentions this issue: os: change t.Errorf call to t.Error in unit test
gopherbot
commented
1 month ago Found new dashboard test flakes for:
#!watchflakes
default <- pkg == "golang.org/x/tools/go/analysis/unitchecker" && test == "TestVetStdlib"Found new dashboard test flakes for:
#!watchflakes
default <- pkg == "golang.org/x/tools/go/analysis/unitchecker" && test == "TestVetStdlib"Found new dashboard test flakes for:
#!watchflakes
default <- pkg == "golang.org/x/tools/go/analysis/unitchecker" && test == "TestVetStdlib"Found new dashboard test flakes for:
#!watchflakes
default <- pkg == "golang.org/x/tools/go/analysis/unitchecker" && test == "TestVetStdlib"
adonovan
commented
1 month ago The original failure ("non-constant format string") was fixed by https://go.dev/cl/603515 (thanks @timothy-king), but I'm seeing a second failure with goroot and x/tools both at tip:
xtools$ go test ./go/analysis/unitchecker
--- FAIL: TestExampleSeparateAnalysis (1.78s)
...
separate_test.go:228: Got:
Want: /main/main.go:6:2: [printf] separate/lib.MyPrintf format %s has arg 123 of wrong type int
FAIL
FAIL golang.org/x/tools/go/analysis/unitchecker 11.954s
FAILIn other words, the separate-analysis test is failing to produce the expected printf diagnostics. Will investigate...
adonovan
commented
1 month ago GOWORK=off is an effective workaround, and this explains why the problem never showed up on the builders, even though it affects my and @findleyr's development environments, and is fully deterministic:
--- FAIL: TestExampleSeparateAnalysis (1.06s)
...
separate_test.go:228: Got:
Want: /main/main.go:6:2: [printf] separate/lib.MyPrintf format %s has arg 123 of wrong type int
FAIL
FAIL golang.org/x/tools/go/analysis/unitchecker 6.576s
FAIL
xtools$ GOWORK=off go test -count=1 ./go/analysis/unitchecker/
ok golang.org/x/tools/go/analysis/unitchecker 6.136sStill investigating...
adonovan
commented
1 month ago Setting GOWORK=off within the test has no effect, so it's the effect of the go.work file on the build of the test executable that matters. Bisecting reveals that it's the go 1.23rc2 (or rc1) directive in the go.work file that has the effect: even when using an rc2 toolchain, this directive causes the test to fail. That means the language-version tags are being affected. So, something in the test depends on the go1.23 language tag. Knowing a little about the test, the only thing I can think of is GODEBUG=gotypesalias. This was a lucky guess: setting this explicitly to 0 causes the test to pass.
So, the presence of a go.work file on a workstation (but not the builder) causes the tests to use go1.23 semantics, including language-version build tags, which may change the default value of a GODEBUG. This is working as intended. But gotypesalias=1 appears to have a bug that somehow affects this test. This bug may be a possible go1.23 release-blocker.
xtools$ GOWORK=off GODEBUG=gotypesalias=1 go test -count=1 -v ./go/analysis/unitchecker/ -run=Sep
=== RUN TestExampleSeparateAnalysis
separate_test.go:228: Got:
Want: /main/main.go:6:2: [printf] separate/lib.MyPrintf format %s has arg 123 of wrong type int
--- FAIL: TestExampleSeparateAnalysis (1.35s)
FAIL
FAIL golang.org/x/tools/go/analysis/unitchecker 1.730s
FAIL
xtools$ GOWORK=off GODEBUG=gotypesalias=0 go test -count=1 -v ./go/analysis/unitchecker/ -run=Sep
--- PASS: TestExampleSeparateAnalysis (1.10s)
PASS
ok golang.org/x/tools/go/analysis/unitchecker 1.502sThanks @matloob and @samthanawalla.
adonovan
commented
1 month ago Another lucky guess: this patch to go/types is an effective workaround. (The only alias I could see in the source was any in the Printf signature.)
{
universeAnyNoAlias = NewTypeName(nopos, nil, "any", &Interface{complete: true, tset: &topTypeSet})
universeAnyNoAlias.setColor(black)
// ensure that the any TypeName reports a consistent Parent, after
// hijacking Universe.Lookup with gotypesalias=0.
universeAnyNoAlias.setParent(Universe)
// It shouldn't matter which representation of any is actually inserted
// into the Universe, but we lean toward the future and insert the Alias
// representation.
universeAnyAlias = NewTypeName(nopos, nil, "any", nil)
universeAnyAlias.setColor(black)
_ = NewAlias(universeAnyAlias, universeAnyNoAlias.Type().Underlying()) // Link TypeName and Alias
- def(universeAnyAlias)
+ def(universeAnyNoAlias)
}The problem is that the printf analyzer assumes that the type of the final args ...interface{} parameter of every printf wrapper is represented as *types.Interface, which is true when the syntax is an explicit interface{} type literal, or when
we referenced the old value of types.Universe.Lookup("any"), but not when any is represented as a materialized alias.
So this is the fix:
--- a/go/analysis/passes/printf/printf.go
+++ b/go/analysis/passes/printf/printf.go
@@ -160,9 +160,8 @@ func maybePrintfWrapper(info *types.Info, decl ast.Decl) *printfWrapper {
nparams := params.Len() // variadic => nonzero
args := params.At(nparams - 1)
- iface, ok := args.Type().(*types.Slice).Elem().(*types.Interface)
- if !ok || !iface.Empty() {
- return nil // final (args) param is not ...interface{}
+ if !types.Identical(args.Type().(*types.Slice).Elem(), types.Universe.Lookup("any").Type()) {
+ return nil // final param (args) is not "...interface{}"
}
This somehow slipped through my audit for places that do types.Type type assertions/switches that might depend on materialized aliases.
gopherbot
commented
1 month ago Change https://go.dev/cl/603938 mentions this issue: go/analysis/passes/printf: add missing Unalias call
gopherbot
commented
2 weeks ago Change https://go.dev/cl/609435 mentions this issue: [gopls-release-branch.0.16] go/analysis/passes/printf: add missing Unalias call
Issue created automatically to collect these failures.
Example (log):
— watchflakes