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

Jump to bottom

x/tools: tests are broken with tip Go #44316

Closed
heschi opened this issue Feb 17, 2021 · 12 comments
Closed

x/tools: tests are broken with tip Go #44316

heschi opened this issue Feb 17, 2021 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@heschi
Copy link
Contributor

heschi commented Feb 17, 2021

@mdempsky
@golang/release
@stamblerre

It appears that something merged to tip today has broken the gopls tests. I'm bisecting.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 17, 2021
@gopherbot gopherbot added this to the Unreleased milestone Feb 17, 2021
@cherrymui
Copy link
Member

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.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 17, 2021
@findleyr
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

heschi commented Feb 17, 2021 

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
Copy link
Contributor

findleyr commented Feb 17, 2021 •
edited

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
Copy link

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

@mdempsky
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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

@findleyr findleyr reopened this Feb 17, 2021
@findleyr
Copy link
Contributor

Ok, x/tools builds are clean now.

@mdempsky
Copy link
Member

Thanks for the quick investigation and fix, everyone!

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.

I'd rather not try to do any non-trivial error filtering outside of types2. I think we'll lose too much context to make good decisions that way.

I think the most limiting factor is how tedious it is to update the regress tests. If you come up with a sensible way to decide which errors to report that isn't too painful to update the regress tests, then it should be easy to match typecheck's behavior to that. The main reason I left the unused variable error as kind of weird is because I didn't want to modify a whole bunch of regress test files, knowing that a lot of them were probably also modified on the dev.typeparams branch. That just seemed like it would be a merge nightmare.

@findleyr
Copy link
Contributor

I'd rather not try to do any non-trivial error filtering outside of types2

FWIW, this would be (mostly) trivial filtering. go/types exposes the Error.Soft boolean, so the logic of identifying which errors to filter would be delegated to go/types, except of course for the logic of switching on the presence of any hard errors. But of course, this may have been what you were referring to, and it certainly could be frustrating to have the set of errors checked in the regression tests completely change when adding or removing a hard error.

@golang golang locked and limited conversation to collaborators Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@mdempsky @dmitshur @gopherbot @cherrymui @heschi @findleyr