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.

Sign up for GitHub

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/go/analysis: show invalid usage of errors.As() #61342

Open
guettli opened this issue Jul 12, 2023 · 4 comments
Open

x/tools/go/analysis: show invalid usage of errors.As() #61342

guettli opened this issue Jul 12, 2023 · 4 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. 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

@guettli
Copy link

guettli commented Jul 12, 2023

Is your feature request related to a problem? Please describe.

This fails:

package main

import (
	"errors"
	"fmt"
	"os"
)

type CustomError struct {
	Msg string
}

func (e CustomError) Error() string {
	return e.Msg
}

func main() {
	// You can replace this with your actual function which returns an error
	err := fmt.Errorf("this is a wrapped error: %w", &CustomError{Msg: "custom error"})

	customErr := CustomError{}
	if errors.As(err, &customErr) {
		// Here you can handle your custom error
		fmt.Println("This is a custom error:", customErr.Msg)
	} else {
		// Here you handle all other errors
		fmt.Println(":-(  sad - not exptected")
		fmt.Println(err)
		os.Exit(1)
	}
}

Describe the solution you'd like

I would like to get a warning if I provide a pointer to a struct to errors.As().

Changing the line to this works:

customErr := &CustomError{}

That's easy to get wrong, and it takes some time to find the error.

Describe alternatives you've considered

I considered the alternative: don't make mistakes. But sorry, I am not perfect.
@hyangah
Copy link
Contributor

hyangah commented Jul 12, 2023

There is errorsas analyzer which is enabled by default. But this check doesn't do what you want.

A different way to fix the issue in the example program is

err := fmt.Errorf("... %w", CustomError{Msg: "custom error"})

It's hard to tell if it's a misuse or not without know how err was constructed, so I am afraid if this feature needs more expensive approaches like whole program analysis or dynamic analysis to work. cc @timothy-king @adonovan

@adonovan
Copy link
Member

The mistake, I think, is here:

err := fmt.Errorf("this is a wrapped error: %w", &CustomError{Msg: "custom error"})

CustomError{...} is an error value, and &CustomError{} is a pointer that also happens to implement the error interface but isn't how this type is supposed to be used.

One possibility is to change the printf checker to report %w applied to a value of type *E where E implements error. That would be something we could easily evaluate using the new metrics pipeline. @timothy-king

@guettli
Copy link
Author

guettli commented Jul 13, 2023

...
One possibility is to change the printf checker to report %w applied to a value of type *E where E implements error. That would be something we could easily evaluate using the new metrics pipeline. @timothy-king

Wow, this would be great. If you are new to Go, then a warning in the IDE would really help. Thank you.

@adonovan adonovan transferred this issue from golang/vscode-go Jul 13, 2023
@adonovan adonovan changed the title Feature request: Show invalid usage of errors.As() x/tools/go/analysis: show invalid usage of errors.As() Jul 13, 2023
@adonovan adonovan added the FeatureRequest Issues asking for a new feature that does not need a proposal. label Jul 13, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 13, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jul 13, 2023
@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 14, 2023
@timothy-king
Copy link
Contributor

One possibility is to change the printf checker to report %w applied to a value of type *E where E implements error.

Just to clarify *E is also an error. I am not sure we have a solid basis for saying E is the only error implementation based on the method signature. In the example, var customErr *CustomError would be valid, and print the expected result. So such a vet check will have theoretical false positives. If we go forward with this in vet, we may want to rope in the author of errors.As for their opinion about the validity of such a rule. (Also this will need a proposal IMO.)

That would be something we could easily evaluate using the new metrics pipeline. @timothy-king

Once I am back, I can look at this pretty quickly. It would be good to have data.

bobg added a commit to bobg/mingo that referenced this issue Dec 5, 2023
@bobg
Remove the dependency on embryonic package gocheck in anticipation of…
381d213 
… the new API in golang/go#61342.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. 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
@guettli @timothy-king @hyangah @adonovan @gopherbot @cherrymui