cmd/go/internal/vet: print line numbers appropriately on list errors #36989

nicks · 2020-02-02T22:44:19Z

For reasons that are unclear to me, this commit:
f1d5ce0
introduces a TestPackagesFor function that strips line numbers from error
messages. This commit introduces a new version of that function for 'go vet'
that always keeps the line numbers.

gopherbot · 2020-02-02T23:00:26Z

This PR (HEAD: 0e2e7ab) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-02T23:01:40Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-03T19:30:40Z

Message from Jay Conrod:

Patch Set 1:

Hey Nick, thanks for working on this.

I'd prefer to keep only one copy of TestPackagesFor so that behavior is consistent in 'go vet', 'go test', and 'go list -test'. It's not clear to me the behavior in load.TestPackagesFor is correct, so maybe we can just fix that one by leaving the position on the error.

There is some ambiguity here: for an import error, sometimes we should show the chain of imports, and sometimes we should show the file name and line number. When a package can't be imported (because it's internal, main, has a non-canonical import path, doesn't exist, ...), we currently report the error on the imported package rather than the importer. Perhaps this can be addressed in another issue though.

cc Bryan, Michael because we've been discussing this lately in the context of golang.org/issue/36188.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-04T00:46:10Z

This PR (HEAD: d005ca5) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-04T00:51:06Z

Message from Nick Santos:

Patch Set 1:

Patch Set 1:

Hey Nick, thanks for working on this.

I'd prefer to keep only one copy of TestPackagesFor so that behavior is consistent in 'go vet', 'go test', and 'go list -test'. It's not clear to me the behavior in load.TestPackagesFor is correct, so maybe we can just fix that one by leaving the position on the error.

There is some ambiguity here: for an import error, sometimes we should show the chain of imports, and sometimes we should show the file name and line number. When a package can't be imported (because it's internal, main, has a non-canonical import path, doesn't exist, ...), we currently report the error on the imported package rather than the importer. Perhaps this can be addressed in another issue though.

cc Bryan, Michael because we've been discussing this lately in the context of golang.org/issue/36188.

Hi Jay! Thanks for the detailed comments. I really appreciate it.

I like your idea. I pushed a new patch set. It changes the behavior in load.TestPackagesFor so that it preserves the line numbers, then changes the use in 'test' to strip the line numbers.

Fortunately, there was a good integration test for the stack-printing behavior (https://github.com/golang/go/blob/master/src/cmd/go/testdata/script/test_import_error_stack.txt), and that helped.

Take a look at this version and let me know what you think.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-04T17:57:07Z

Message from Jay Conrod:

Patch Set 2:

Thanks, I tried this out and thought about it a bit more.

I think the error messages for imports should be the same (or nearly so) for 'go vet' and 'go test'. If a test directly imports a package that can't be imported for some reason, both commands should show a file and line number. If a test transitively imports a package with an error, both commands should show a stack. So it should be possible to fix this in load.TestPackagesFor without any special case in internal/test.

Perhaps the important change is this: in load.TestPackagesFor, if the error is on a package directly imported by the test package, we should show a position. Otherwise (if the error is in a transitively imported package), we should show a stack.

if len(p1.DepsErrors) > 0 {
  perr := p1.DepsErrors[0]
  direct := len(perr.ImportStack) >= 2 && perr.ImportStack[len(perr.ImportStack)-2] == p1.ImportPath
  if !direct {
    perr.Pos = "" // show full import stack
  }
  err = perr
  break
}

For reference, the stack printing behavior was introduced in CL 12176 as a fix for golang.org/issue/9558.

Also, I've added a comment to golang.org/issues/26909 about the ambiguity.

Added matloob to reviewers as well.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-04T22:12:04Z

This PR (HEAD: 3578b2f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-04T22:18:35Z

Message from Nick Santos:

Patch Set 3:

Patch Set 2:

Thanks, I tried this out and thought about it a bit more.

I think the error messages for imports should be the same (or nearly so) for 'go vet' and 'go test'. If a test directly imports a package that can't be imported for some reason, both commands should show a file and line number. If a test transitively imports a package with an error, both commands should show a stack. So it should be possible to fix this in load.TestPackagesFor without any special case in internal/test.

Perhaps the important change is this: in load.TestPackagesFor, if the error is on a package directly imported by the test package, we should show a position. Otherwise (if the error is in a transitively imported package), we should show a stack.
if len(p1.DepsErrors) > 0 {
  perr := p1.DepsErrors[0]
  direct := len(perr.ImportStack) >= 2 && perr.ImportStack[len(perr.ImportStack)-2] == p1.ImportPath
  if !direct {
    perr.Pos = "" // show full import stack
  }
  err = perr
  break
}
For reference, the stack printing behavior was introduced in CL 12176 as a fix for golang.org/issue/9558.

Also, I've added a comment to golang.org/issues/26909 about the ambiguity.

Added matloob to reviewers as well.

I uploaded a new patch set. The ImportStack has a weird format that I don't totally understand - it looks like [ "command-line-arguments", "(test)", "real/pkg/name" ]. I added a test for "direct" errors that I think captures the spirit of your suggestion and passes all the tests.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-05T15:29:55Z

Message from Jay Conrod:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-05T17:03:50Z

This PR (HEAD: c4cdc33) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-05T17:06:51Z

Message from Nick Santos:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-05T19:51:02Z

Message from Michael Matloob:

Patch Set 4:

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-05T20:03:02Z

Message from Jay Conrod:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-05T21:07:34Z

Message from Nick Santos:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-05T21:13:33Z

Message from Bryan C. Mills:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-06T04:05:25Z

This PR (HEAD: 048ad00) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-06T04:10:22Z

Message from Nick Santos:

Patch Set 5:

(1 comment)

Thanks everyone! I pushed up a new patch set with a buttload of new tests that poke at several different combinations of this problem.

The new behavior isn't perfect, but is at least a lot better. In particular, the code in load.PackagesForBuild is short-circuiting a lot of the smart-stack-trace-printing logic that this code is trying to do.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

Beaujr

[deleted]

gopherbot · 2020-02-06T15:09:49Z

Message from Jay Conrod:

Patch Set 5: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-06T15:11:24Z

Message from Gobot Gobot:

Patch Set 5:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=e03ef357

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-06T15:21:41Z

Message from Gobot Gobot:

Patch Set 5: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-10T20:03:25Z

Message from Bryan C. Mills:

Patch Set 5:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-11T04:19:48Z

This PR (HEAD: 5a6a3df) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-13T20:24:36Z

Message from Jay Conrod:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-14T06:25:22Z

This PR (HEAD: ca2984b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-14T06:33:39Z

Message from Nick Santos:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-14T16:36:32Z

Message from Jay Conrod:

Patch Set 8:

(3 comments)

Thanks for fixing this! I have a couple API nits, but this is looking really good.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-14T16:38:04Z

Message from Jay Conrod:

Patch Set 8: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-14T16:38:06Z

Message from Gobot Gobot:

Patch Set 8:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=a26e4df6

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-14T16:43:55Z

Message from Gobot Gobot:

Patch Set 8:

Build is still in progress...
This change failed on linux-amd64-race:
See https://storage.googleapis.com/go-build-log/a26e4df6/linux-amd64-race_cc2aa85d.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-14T16:50:23Z

Message from Gobot Gobot:

Patch Set 8: TryBot-Result-1

3 of 20 TryBots failed:
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/a26e4df6/linux-amd64-race_cc2aa85d.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/a26e4df6/windows-amd64-2016_f5109c69.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/a26e4df6/windows-386-2008_43c89cf8.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

Fixes golang#36173 For reasons that are unclear to me, this commit: golang@f1d5ce0 introduces a TestPackagesFor function that strips line numbers from error messages. This commit introduces a new version of that function for 'go vet' that always keeps the line numbers.

gopherbot · 2020-02-17T03:36:19Z

This PR (HEAD: 81c8ef2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/217106 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-02-17T04:15:38Z

Message from Nick Santos:

Patch Set 9:

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-18T17:22:01Z

Message from Bryan C. Mills:

Patch Set 9:

(8 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-18T18:35:32Z

Message from Nick Santos:

Patch Set 9:

Bryan - would it make more sense for you to take this PR over?

It sounds like you have pretty strong opinions about the internal details. Which is fine! And makes sense! I don't work in the Go codebase full-time, and don't have a good sense of what internal details matter.

But at the same time, it feels like going back and forth on some of these details isn't a particularly effective use of my time or of your yours. No hard feelings on it. I just don't have the context that you do. And it feels like we're spending a lot of time on back-and-forth communicating that context.

What do you think?

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-18T22:18:16Z

Message from Michael Matloob:

Patch Set 9:

Patch Set 9:

Bryan - would it make more sense for you to take this PR over?

It sounds like you have pretty strong opinions about the internal details. Which is fine! And makes sense! I don't work in the Go codebase full-time, and don't have a good sense of what internal details matter.

But at the same time, it feels like going back and forth on some of these details isn't a particularly effective use of my time or of your yours. No hard feelings on it. I just don't have the context that you do. And it feels like we're spending a lot of time on back-and-forth communicating that context.

What do you think?

I'll take this over.

Thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-20T22:33:31Z

Message from Michael Matloob:

Uploaded patch set 10: Run-TryBot+1.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-20T22:33:32Z

Message from Michael Matloob:

Patch Set 10:

(5 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-20T22:33:33Z

Message from Gobot Gobot:

Patch Set 10:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=e4e2d932

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-20T22:33:34Z

Message from Michael Matloob:

Patch Set 10:

(I'm not done addressing all the comments yet. Will send more tomorrow)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-21T20:52:24Z

Message from Bryan C. Mills:

Patch Set 11:

I rebased CL 185345, and now it has some test failures that might be related.

(Specifically, it seems to be emitting smaller import stacks in the noncanonical_import test case.)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-21T22:09:24Z

Message from Michael Matloob:

Uploaded patch set 12: Run-TryBot+1.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-21T22:09:25Z

Message from Michael Matloob:

Patch Set 12:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-21T22:09:26Z

Message from Gobot Gobot:

Patch Set 12:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=4c730a41

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-21T22:18:27Z

Message from Michael Matloob:

Uploaded patch set 14: Run-TryBot+1.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-21T22:18:28Z

Message from Gobot Gobot:

Patch Set 14:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=b1eb63df

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-24T22:09:04Z

Message from Michael Matloob:

Uploaded patch set 16: Run-TryBot+1.

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-24T22:09:05Z

Message from Gobot Gobot:

Patch Set 16:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=bf77eede

Please don’t reply on this GitHub thread. Visit golang.org/cl/217106.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-02-24T22:16:20Z

This PR is being closed because golang.org/cl/217106 has been abandoned.

Replaced with https://go-review.googlesource.com/c/go/+/220718. (The original was a GitHub/Gerrit cl, and because of https://golang.org/issue/24887 , gerritbot would keep fighting us.

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Feb 2, 2020

nicks force-pushed the nicks/vet branch from 0e2e7ab to d005ca5 Compare February 4, 2020 00:44

nicks force-pushed the nicks/vet branch from d005ca5 to 3578b2f Compare February 4, 2020 22:11

nicks force-pushed the nicks/vet branch from 3578b2f to c4cdc33 Compare February 5, 2020 17:03

nicks force-pushed the nicks/vet branch from c4cdc33 to 048ad00 Compare February 6, 2020 04:04

Beaujr suggested changes Feb 6, 2020

View reviewed changes

nicks force-pushed the nicks/vet branch from 048ad00 to 5a6a3df Compare February 11, 2020 04:18

nicks force-pushed the nicks/vet branch from 5a6a3df to 6f31565 Compare February 11, 2020 04:43

nicks force-pushed the nicks/vet branch from 6f31565 to ca2984b Compare February 14, 2020 06:23

nicks force-pushed the nicks/vet branch from ca2984b to 81c8ef2 Compare February 17, 2020 03:35

gopherbot closed this Feb 24, 2020

cmd/go/internal/vet: print line numbers appropriately on list errors #36989

cmd/go/internal/vet: print line numbers appropriately on list errors #36989

Conversation

nicks commented Feb 2, 2020

gopherbot commented Feb 2, 2020

gopherbot commented Feb 2, 2020

gopherbot commented Feb 3, 2020

gopherbot commented Feb 4, 2020

gopherbot commented Feb 4, 2020

gopherbot commented Feb 4, 2020

gopherbot commented Feb 4, 2020

gopherbot commented Feb 4, 2020

gopherbot commented Feb 5, 2020

gopherbot commented Feb 5, 2020

gopherbot commented Feb 5, 2020

gopherbot commented Feb 5, 2020

gopherbot commented Feb 5, 2020

gopherbot commented Feb 5, 2020

gopherbot commented Feb 5, 2020

gopherbot commented Feb 6, 2020

gopherbot commented Feb 6, 2020

Beaujr left a comment • edited Loading

Choose a reason for hiding this comment

gopherbot commented Feb 6, 2020

gopherbot commented Feb 6, 2020

gopherbot commented Feb 6, 2020

gopherbot commented Feb 10, 2020

gopherbot commented Feb 11, 2020

gopherbot commented Feb 13, 2020

gopherbot commented Feb 14, 2020

gopherbot commented Feb 14, 2020

gopherbot commented Feb 14, 2020

gopherbot commented Feb 14, 2020

gopherbot commented Feb 14, 2020

gopherbot commented Feb 14, 2020

gopherbot commented Feb 14, 2020

gopherbot commented Feb 17, 2020

gopherbot commented Feb 17, 2020

gopherbot commented Feb 18, 2020

gopherbot commented Feb 18, 2020

gopherbot commented Feb 18, 2020

gopherbot commented Feb 20, 2020

gopherbot commented Feb 20, 2020

gopherbot commented Feb 20, 2020

gopherbot commented Feb 20, 2020

gopherbot commented Feb 21, 2020

gopherbot commented Feb 21, 2020

gopherbot commented Feb 21, 2020

gopherbot commented Feb 21, 2020

gopherbot commented Feb 21, 2020

gopherbot commented Feb 21, 2020

gopherbot commented Feb 24, 2020

gopherbot commented Feb 24, 2020

gopherbot commented Feb 24, 2020

Beaujr left a comment •

edited

Loading