x/tools: tests are broken with tip Go

[heschi]

@mdempsky
@golang/release
@stamblerre

It appears that something merged to tip today has broken the gopls tests. I'm bisecting.

[cherrymui]

I took a look but I'm not familiar with those tests and couldn't tell what the problem is. It seems all failures are related to q declared but not used message missing in bad1.go. Is this due to the compiler's diagnostic message changed? Is there a way to reproduce it in a standalone manner with the compiler? Thanks.

[findleyr]

It's likely that this due to a change in go/types that I made in dev.regabi. I can't look for 30 minutes or so, but https://golang.org/cl/289720 comes to mind given @cherrymui's description.

[cherrymui]

BTW, failures looks like https://build.golang.org/log/35d8dfbfdecd12e2a4a09df89b83dd9240dc7c66
e.g.

--- FAIL: TestLSP (28.17s)
    --- FAIL: TestLSP/Modules (9.41s)
        --- FAIL: TestLSP/Modules/Diagnostics (0.44s)
            --- FAIL: TestLSP/Modules/Diagnostics/bad1 (0.00s)
                lsp_test.go:211: diagnostics failed because of different lengths got 15 want 16:
                    expected:
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:7:6-7:13: undeclared name: unknown
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:15:1-15:2: x declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:16:5-16:6: q declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:16:7-16:11: undeclared name: blah
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:17:5-17:6: t declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:17:9-17:13: undeclared name: blob
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:26:5-26:7: ch declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:26:14-26:22: undeclared name: favType1
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:27:5-27:6: m declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:27:11-27:18: undeclared name: keyType
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:28:5-28:8: arr declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:28:11-28:19: undeclared name: favType2
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:29:5-29:8: fn1 declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:29:16-29:25: undeclared name: badResult
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:30:5-30:8: fn2 declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:30:14-30:22: undeclared name: badParam
                    got:
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:7:6-7:13: undeclared name: unknown
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:15:1-15:2: x declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:16:7-16:11: undeclared name: blah
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:17:5-17:6: t declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:17:9-17:13: undeclared name: blob
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:26:5-26:7: ch declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:26:14-26:22: undeclared name: favType1
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:27:5-27:6: m declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:27:11-27:18: undeclared name: keyType
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:28:5-28:8: arr declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:28:11-28:19: undeclared name: favType2
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:29:5-29:8: fn1 declared but not used
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:29:16-29:25: undeclared name: badResult
                      file:///workdir/tmp/TestLSP_Modules066057659/lsp/bad/bad1.go:30:5-30:8: fn2 declared but not used

Otheres seem similar.

[heschi]

afd67f333466fc67cd37433e45ecdb190efc8f51 is the first bad commit
commit afd67f333466fc67cd37433e45ecdb190efc8f51
Author: Rob Findley <rfindley@google.com>
Date:   Thu Feb 4 10:27:41 2021 -0500

    [dev.regabi] go/types: no "declared but not used" errors for invalid var decls
    
    This is a port of CL 274615, adapted to go/types. The only change was in
    the positioning of expected errors in vardecl.src: in go/types they are
    positioned on the identifier.

Seems like this is probably a benign bug either in go/types or the gopls tests. So I'm gonna go eat dinner.

[findleyr]

Thanks Heschi for bisecting. I'm looking at this now.

OK this change was certainly intentional. However, looking at this failure, I think perhaps I didn't spend enough time considering its repercussions. In particular, we seem to still be inconsistent with the compiler. Consider this:
https://play.golang.org/p/3mAfAX8CKZj
(this is a copy of the relevant section of the go/typesx/tools tests, with the expected diagnostics annotated inline)

Two observations:

• the compiler suppresses all unused variable errors within the scope (even that of x), not just those with invalid declarations.
• go/types apparently still doesn't suppress the error for t, even though its declaration is invalid.

So rather than work around this in gopls tests, I think I should revert https://golang.org/cl/289712 until we can consider it further.

CC @griesemer

[gopherbot]

Change https://golang.org/cl/292751 mentions this issue: go/types: revert "no 'declared but not used' errors for invalid var decls"

[mdempsky]

the compiler suppresses all unused variable errors within the scope (even that of x), not just those with invalid declarations.

FWIW, historically the compiler didn't report unused variables until code generation, so they were only reported if the user code successfully typechecked otherwise. I moved that to the frontend recently, but unused variables are still reported only for functions that successfully typecheck, and (IIRC) if we haven't seen any errors previously yet.

I think this might be a situation where it's easier to update cmd/compile to match go/types behavior instead, or at least find some common middleground. The compiler's current behavior here certainly isn't principled enough to merit trying to reproduce it.

[findleyr]

Thanks for the background.

I think this might be a situation where it's easier to update cmd/compile to match go/types behavior instead, or at least find some common middleground.

Perhaps this could be as simple as the compiler filtering out 'soft' errors from types2 in the presence of any 'hard' errors. We've debated the value of having this hardness dimension, but this seems like the perfect use-case for it.

[findleyr]

Err, reopening this until the build dashboard clears up, in case anyone goes looking for an open issue related to the failures.

[findleyr]

Ok, x/tools builds are clean now.