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

cmd/go: regression in behavior of DepsErrors #40544

Closed
dominikh opened this issue Aug 3, 2020 · 3 comments
Closed

cmd/go: regression in behavior of DepsErrors #40544

dominikh opened this issue Aug 3, 2020 · 3 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@dominikh
Copy link
Member

dominikh commented Aug 3, 2020

The behavior of the DepsErrors field in the output of go list -json -e has changed between Go 1.14 and tip.

In GOPATH-mode, have two packages sandbox/foo and sandbox/bar, with sandbox/foo/foo.go containing

package pkg

import "sandbox/bar"

and sandbox/bar/bar.go containing

ackage bar

Running go list -e -json . in sandbox/foo with Go 1.14.4 would produce the following DepsErrors:

	"DepsErrors": [
		{
			"ImportStack": [
				"sandbox/foo",
				"sandbox/bar"
			],
			"Pos": "foo.go:3:8",
			"Err": "\n../bar/bar.go:1:1: expected 'package', found ackage"
		}
	],

the ImportStack would clearly indicate that this issue occurs in the sandbox/bar package.

However, running the same command with tip (f923374) produces the following output:

	"DepsErrors": [
		{
			"ImportStack": [
				"sandbox/foo"
			],
			"Pos": "foo.go:3:8",
			"Err": "expected 'package', found ackage"
		}
	],

the ImportStack is truncated, and it is impossible to tell which package the error is coming from. This was caused by 641918e (/cc @matloob).

Interestingly, between 1.14 and 641918e, 74d6de0 improved on the error, by using a more accurate value for Pos:

	"DepsErrors": [
		{
			"ImportStack": [
				"sandbox/foo",
				"sandbox/bar"
			],
			"Pos": "../bar/bar.go:1:1",
			"Err": "expected 'package', found ackage"
		}
	],
@dominikh dominikh added this to the Go1.15 milestone Aug 3, 2020
@bcmills
Copy link
Contributor

bcmills commented Aug 3, 2020

Just to set expectations, this may or may not make go1.15rc2. (I agree that it's a regression, but it doesn't seem severe enough to hold up the release if everything else is ready.)

CC @matloob @jayconrod

@bcmills bcmills added GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 3, 2020
@gopherbot
Copy link

Change https://golang.org/cl/246717 mentions this issue: cmd/go: fix error stacks when there are scanner errors

@gopherbot
Copy link

Change https://golang.org/cl/246758 mentions this issue: go/packages: expand cases when filenames are parsed from errors

gopherbot pushed a commit to golang/tools that referenced this issue Aug 5, 2020
Only add files in errors if either the error's import stack is empty,
or the import stack's top package is the same as the package itself,
so we have more confidence that the error applies to the package.

This is bound to be flaky, but shouldn't be worse than the current
state. (And it unblocks a cl from going into the RC...).

Updates golang/go#40544

Change-Id: Ie21a8abec7150800d3d34b94a7ec90fd40d93fe9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246758
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@golang golang locked and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants