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

Improve error reporting when config is missing #275

Open
mlevesquedion opened this issue Feb 1, 2021 · 2 comments
Open

Improve error reporting when config is missing #275

mlevesquedion opened this issue Feb 1, 2021 · 2 comments

Comments

@mlevesquedion
Copy link
Contributor

mlevesquedion commented Feb 1, 2021

Currently, when the analyzer is used through go vet, if no config flag is provided or the provided path is invalid, the analyzer produces this error message:

levee: failed prerequisites: fieldtags, source

This message does not provide any hints as to its cause, which will likely leave users scratching their head. (I know I've scratched my own head more than once when dealing with this issue.)

Additional context

If the config can't be read, the fieldtags and source analyzers will fail:

// fieldtags/analyzer.go
conf, err := config.ReadConfig()
if err != nil {
	return nil, err
}

Then, in the analysis package, running levee fails because its dependencies failed:

// Report an error if any dependency failed.
var failed []string
for _, dep := range act.deps {
	if dep.err != nil {
		failed = append(failed, dep.String())
	}
}
if failed != nil {
	sort.Strings(failed)
	act.err = fmt.Errorf("failed prerequisites: %s", strings.Join(failed, ", "))
	return
}

Note that when the analyzer is run directly (i.e. not through go vet), the error message is much better:

error reading analysis config: open config.yaml: no such file or directory

This message may still be a bit confusing to users who did not specify a config at all. The open config.yaml part is due to the fact that the config flag defaults to config.yaml.

@PurelyApplied
Copy link
Collaborator

PurelyApplied commented Feb 4, 2021

[Just spreading knowledge from some brief fix attempts.]

I was poking at this issue and thought the most obvious solution would be to add a config.ReadConfig() check in the cmd/levee/ before the analyzer even got going. Unfortunately, that also produces very strange results when used with go vet -vettool=....

Failing fast would be preferable to failing once per package / analyzer, but if we're not able to do that before the analyzer gets rolling, we could perhaps read config in a base "analyzer" and percolate errors up that way. I'd kind of hate how that would look, though.

[edit:] Actually, I just stubbed that shim analyzer approach out and it's not any better. We still end up spending all the cycles build buildssa, and since the error would still be an wrapped in an upstream analyzer anyway, it doesn't report any cleaner. It of course just adds one more to the meaningless set of levee: failed prerequisites: config, fieldtags, source. So that's a wash too. Probably should've seen that coming.

@mlevesquedion
Copy link
Contributor Author

Thank you for doing this investigation! I'm surprised that the shim analyzer approach didn't work, but it makes sense in retrospect.

I think one way to at least show the error message would be to print it to the terminal in the code that reads the config, but then there would be duplicate messages when the analyzer is run by itself. That may be a reasonable tradeoff, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants