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

x/tools/go/analysis/passes/nilness: detect nil-checking helpers #41135

Closed
ainar-g opened this issue Aug 30, 2020 · 2 comments
Closed

x/tools/go/analysis/passes/nilness: detect nil-checking helpers #41135

ainar-g opened this issue Aug 30, 2020 · 2 comments
Labels
FeatureRequest 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

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Aug 30, 2020

As of v0.0.0-20200828161849-5deb26317202, the nilness command doesn't seem to take into account nil-checking helpers, which are popular in e.g. small scripts. Consider:

     8	func check(err error) {
     9		if err != nil {
    10			panic(err)
    11		}
    12	}
    13	
    14	func f() (err error) {
    15		err = e1()
    16		if err != nil {
    17			return err
    18		}
    19	
    20		// Oops!  Forgot to replace "_" with "err" after debugging.
    21		_ = e2()
    22		if err != nil {
    23			return err
    24		}
    25	
    26		return nil
    27	}
    28	
    29	func g() {
    30		var err error
    31	
    32		err = e1()
    33		check(err)
    34	
    35		// Oops!  Forgot to replace "_" with "err" after debugging.
    36		_ = e2()
    37		check(err)
    38	}

Full code. Currently, nilness reports line 22, but not line 37.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 30, 2020
@gopherbot gopherbot added this to the Unreleased milestone Aug 30, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Sep 1, 2020

/cc @matloob per owners.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Sep 1, 2020
@matloob
Copy link
Contributor

matloob commented Sep 14, 2020

I myself have used this style when starting code on a new project, but this isn't something that we want our tools to encourage: errors should be handled explicitly instead of using these sorts of helpers, even if it makes code more clunky.

(This is the sort of thing that would have been solved if we went forward with the "try" error handling proposal... alas...)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest 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
Development

No branches or pull requests

4 participants