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

Potential false negative, didn't report error result in unexported method that always returned nil. #21

Closed
dmitshur opened this Issue Dec 1, 2017 · 12 comments

Comments

2 participants
@dmitshur
Contributor

dmitshur commented Dec 1, 2017

This issue is inspired by a nice simplification I manually discovered recently, shurcooL/issues@beebdcc.

In it, the method reactions had return signature ([]reactions.Reaction, error), but the only return statement was always returning nil error.

I was going to open an issue in @dominikh's go-tools repo for consideration, but then realized that this should fit right into unparam's scope. From reading the README, I don't have a good answer for why this simplification opportunity wouldn't be reported.

Is this an enhancement opportunity, or am I missing a reason it couldn't/shouldn't have been detected?

I tested with latest unparam on the parent of that commit, and didn't get anything:

$ unparam github.com/shurcooL/issues/githubapi
$ 

With -debug, the output is:

$ unparam -debug
func ghUser
parameter user : *github.com/google/go-github/github.User
  skip - used somewhere in the func body
func ghRepoSpec
parameter repo : github.com/shurcooL/issues.RepoSpec
  skip - used somewhere in the func body
func ghIssueState
parameter state : github.com/shurcooL/githubql.IssueState
  skip - used somewhere in the func body
func init
func (service).reactions
parameter rgs : reactionGroups
  skip - used somewhere in the func body
func internalizeReaction
parameter reaction : github.com/shurcooL/githubql.ReactionContent
  skip - used somewhere in the func body
func (service).markRead
parameter ctx : context.Context
  skip - used somewhere in the func body
parameter repo : github.com/shurcooL/issues.RepoSpec
  skip - used somewhere in the func body
parameter id : uint64
  skip - used somewhere in the func body
func ghEventType
parameter typename : string
  skip - used somewhere in the func body
func externalizeReaction
parameter reaction : github.com/shurcooL/reactions.EmojiID
  skip - used somewhere in the func body
func ghActor
parameter actor : *githubqlActor
  skip - used somewhere in the func body
func ghColor
parameter hex : string
  skip - used somewhere in the func body
@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Dec 4, 2017

Owner

The current reason is because we require at least two return statements to print any "result X is always Y" warning. This is because, sometimes, it's easier to return a constant once than to hard-code it in the caller.

Or, in other words, it often doesn't simplify the code overall to move it from one place to another if it was only being returned once.

However, you could say that nil (or zero values in general) should not have such a restriction, because they would often simplify the overall code, even if they were only being returned once. I need to look into this and see what new warnings are printed for std cmd and my packages.

Thanks for the suggestion!

Owner

mvdan commented Dec 4, 2017

The current reason is because we require at least two return statements to print any "result X is always Y" warning. This is because, sometimes, it's easier to return a constant once than to hard-code it in the caller.

Or, in other words, it often doesn't simplify the code overall to move it from one place to another if it was only being returned once.

However, you could say that nil (or zero values in general) should not have such a restriction, because they would often simplify the overall code, even if they were only being returned once. I need to look into this and see what new warnings are printed for std cmd and my packages.

Thanks for the suggestion!

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

Contributor

I see, that makes sense. I think I missed this in the README:

In the last two cases, a minimum number of calls is required to ensure that the warnings are useful.

However, is that sentence missing a word? "a minimum number of two calls is required..."?

Or is that unrelated to the "at least two return statements" requirement? If so, I think it'd be helpful to document in README alongside with the other conditions.

However, you could say that nil (or zero values in general) should not have such a restriction, because they would often simplify the overall code, even if they were only being returned once.

Indeed, it sounds like this might be worthy of a more specific check/warning, that the returned value is always zero.

Thanks!

Contributor

dmitshur commented Dec 4, 2017

I see, that makes sense. I think I missed this in the README:

In the last two cases, a minimum number of calls is required to ensure that the warnings are useful.

However, is that sentence missing a word? "a minimum number of two calls is required..."?

Or is that unrelated to the "at least two return statements" requirement? If so, I think it'd be helpful to document in README alongside with the other conditions.

However, you could say that nil (or zero values in general) should not have such a restriction, because they would often simplify the overall code, even if they were only being returned once.

Indeed, it sounds like this might be worthy of a more specific check/warning, that the returned value is always zero.

Thanks!

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Dec 17, 2017

Owner

The README is intentionally vague because the heuristics are changing constantly, and because I don't want users to have to understand them to use the tool. For example, one of these requirements was changed in a3ef08f.

The -debug flag could be better at giving human-friendly reasons as to why things were or were not flagged, though. It does not do that yet.

As for the check itself - I had a crack at it last week and had it nearly working, but got stuck on some subtle false positives/negatives. I'll likely give this a second go after the holidays.

Owner

mvdan commented Dec 17, 2017

The README is intentionally vague because the heuristics are changing constantly, and because I don't want users to have to understand them to use the tool. For example, one of these requirements was changed in a3ef08f.

The -debug flag could be better at giving human-friendly reasons as to why things were or were not flagged, though. It does not do that yet.

As for the check itself - I had a crack at it last week and had it nearly working, but got stuck on some subtle false positives/negatives. I'll likely give this a second go after the holidays.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 18, 2017

Contributor

because I don't want users to have to understand them to use the tool.

I personally still think it's helpful to explicitly document the behavior, even if not in the README, because then it makes it possible to tell whether some behavior one observes is a bug (it doesn't match the documented behavior) or a feature request. Having the implementation be the documentation makes that distinction impossible.

Of course, I realize documenting this is hard and might not be worth the extra effort, so I'm fine with whatever approach you choose for your project.

The -debug flag could be better at giving human-friendly reasons as to why things were or were not flagged, though. It does not do that yet.

Perhaps I needed to try that. I don't need it to be very human-friendly, as long as I can understand a reason something is/isn't flagged, it'd be good enough.

As for the check itself - I had a crack at it last week and had it nearly working, but got stuck on some subtle false positives/negatives.

Nice, thanks! I'd be curious what those subtle false positives/negatives were, if you have them handy. Otherwise, np.

Contributor

dmitshur commented Dec 18, 2017

because I don't want users to have to understand them to use the tool.

I personally still think it's helpful to explicitly document the behavior, even if not in the README, because then it makes it possible to tell whether some behavior one observes is a bug (it doesn't match the documented behavior) or a feature request. Having the implementation be the documentation makes that distinction impossible.

Of course, I realize documenting this is hard and might not be worth the extra effort, so I'm fine with whatever approach you choose for your project.

The -debug flag could be better at giving human-friendly reasons as to why things were or were not flagged, though. It does not do that yet.

Perhaps I needed to try that. I don't need it to be very human-friendly, as long as I can understand a reason something is/isn't flagged, it'd be good enough.

As for the check itself - I had a crack at it last week and had it nearly working, but got stuck on some subtle false positives/negatives.

Nice, thanks! I'd be curious what those subtle false positives/negatives were, if you have them handy. Otherwise, np.

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Feb 11, 2018

Owner

Sorry about the late reply. I was purposedly ignoring this project for a while, as I got a bit frustrated the second time I tried to write a fix for this :)

Of course, I realize documenting this is hard and might not be worth the extra effort, so I'm fine with whatever approach you choose for your project.

That's true - I hadn't thought about bug reporting. Although, if I implemented a proper -debug flag, that would also work. The tool would tell you something like did not flag method Foo because it implements an interface. Or, in your case, did not flag method reactions because the two return statement cutoff wasn't reached.

Perhaps I needed to try that. I don't need it to be very human-friendly, as long as I can understand a reason something is/isn't flagged, it'd be good enough.

Well, that's the thing, it does a terrible job at that at the moment. It tells you what it's doing at every step, instead of what overall decision it made and why. It's written forwards when it should be backwards, if that makes sense.

Nice, thanks! I'd be curious what those subtle false positives/negatives were, if you have them handy. Otherwise, np.

I'm having another, calmer look at this issue now. I can't remember al the details from two months ago, but I think that the issue that got me stuck is "forwarding return calls", for a lack of a better term.

For example, say that I make it so that nil return values don't require to be returned twice to trigger the result is always X warning. That's an easy patch.

But then, what about:

func alwaysReturnsNil() (int, error) {
    return someConstant, nil
}

func someFunc() (int, error) {
    if someCond {
        return alwaysReturnsNil()
    }
    [...]
}

Removing the error return from the signature would mean that all of these "forwarding return calls" would have to be rewritten to:

x, _ := alwaysReturnsNil()
return x

Which isn't a bad option, but it's certainly a false positive. We don't want to force the user to make this change, as most people will find it intrusive and a net loss of readability/simplicity.

Some examples from the standard library:

  • src/text/template/parse/lex.go:191 lexer.errorf
  • runtime/pprof/pprof.go:354 printCountCycleProfile
  • math/rand/rand.go:264 read

You get the picture. It was a coincidence that this false positive was hidden, as my code incorrectly skipped all the nil return values when doing this result is always X check.

Owner

mvdan commented Feb 11, 2018

Sorry about the late reply. I was purposedly ignoring this project for a while, as I got a bit frustrated the second time I tried to write a fix for this :)

Of course, I realize documenting this is hard and might not be worth the extra effort, so I'm fine with whatever approach you choose for your project.

That's true - I hadn't thought about bug reporting. Although, if I implemented a proper -debug flag, that would also work. The tool would tell you something like did not flag method Foo because it implements an interface. Or, in your case, did not flag method reactions because the two return statement cutoff wasn't reached.

Perhaps I needed to try that. I don't need it to be very human-friendly, as long as I can understand a reason something is/isn't flagged, it'd be good enough.

Well, that's the thing, it does a terrible job at that at the moment. It tells you what it's doing at every step, instead of what overall decision it made and why. It's written forwards when it should be backwards, if that makes sense.

Nice, thanks! I'd be curious what those subtle false positives/negatives were, if you have them handy. Otherwise, np.

I'm having another, calmer look at this issue now. I can't remember al the details from two months ago, but I think that the issue that got me stuck is "forwarding return calls", for a lack of a better term.

For example, say that I make it so that nil return values don't require to be returned twice to trigger the result is always X warning. That's an easy patch.

But then, what about:

func alwaysReturnsNil() (int, error) {
    return someConstant, nil
}

func someFunc() (int, error) {
    if someCond {
        return alwaysReturnsNil()
    }
    [...]
}

Removing the error return from the signature would mean that all of these "forwarding return calls" would have to be rewritten to:

x, _ := alwaysReturnsNil()
return x

Which isn't a bad option, but it's certainly a false positive. We don't want to force the user to make this change, as most people will find it intrusive and a net loss of readability/simplicity.

Some examples from the standard library:

  • src/text/template/parse/lex.go:191 lexer.errorf
  • runtime/pprof/pprof.go:354 printCountCycleProfile
  • math/rand/rand.go:264 read

You get the picture. It was a coincidence that this false positive was hidden, as my code incorrectly skipped all the nil return values when doing this result is always X check.

mvdan added a commit that referenced this issue Feb 11, 2018

check: fix "result X is always nil" warnings
They weren't working, as we used nil to mark when a return value wasn't
consistent. However, constant.Value represents a nil value as simply an
untyped nil, so the two collided and we discarded those.

Add a level of indirection so that we can tell the two nil cases apart.

Updates #21.
@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Feb 11, 2018

Owner

Also, a big reason why I failed twice at working on this is because I tried to do too much at once. I was trying to fix the nil bug fixed above, as well as your bug report, as well as the "forwarding return call" false positives. I sometimes forget how important it is to separate issues into little pieces and commits.

Owner

mvdan commented Feb 11, 2018

Also, a big reason why I failed twice at working on this is because I tried to do too much at once. I was trying to fix the nil bug fixed above, as well as your bug report, as well as the "forwarding return call" false positives. I sometimes forget how important it is to separate issues into little pieces and commits.

mvdan added a commit that referenced this issue Feb 11, 2018

check: fix "forwarding return calls" false positive
For example, given:

	func alwaysReturnsNil() (int, error) {
		return someConstant, nil
	}

	func someFunc() (int, error) {
		if someCond {
			return alwaysReturnsNil()
		}
		[...]
	}

We may flag alwaysReturnsNil as "result error is always nil". But
applying that change would make little sense, as it would make all of
these forwarding callers more complex.

Teach this specific check to avoid these cases, to not run into false
positives.

Updates #21.

@mvdan mvdan closed this in ed0d5d5 Feb 11, 2018

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Feb 11, 2018

Owner

Thanks again for the bug report - this unearthed a bunch of bugs in unparam, and as a result the tool now spots half a dozen more unused results in the Go repo.

Owner

mvdan commented Feb 11, 2018

Thanks again for the bug report - this unearthed a bunch of bugs in unparam, and as a result the tool now spots half a dozen more unused results in the Go repo.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 12, 2018

Contributor

Thank you for your work and making the tool better; glad my report was helpful!

Contributor

dmitshur commented Feb 12, 2018

Thank you for your work and making the tool better; glad my report was helpful!

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 13, 2018

Contributor

Looking to clarify. This issue was marked as resolved in commit ed0d5d5 with the following message:

check: "result X is always nil" with one return

Before, we required at least two return statements for all return types.
Make a special case out of untyped nil, since it's much less prone to
false positives.

Fixes #21.

From that text, it sounds to me like it should now be reporting the code as reported in the original issue (about reactions always returning nil error). However, testing the latest version of unparam, it still doesn't seem to be.

Is my understanding wrong, or is there a bug in the implementation?

Contributor

dmitshur commented Feb 13, 2018

Looking to clarify. This issue was marked as resolved in commit ed0d5d5 with the following message:

check: "result X is always nil" with one return

Before, we required at least two return statements for all return types.
Make a special case out of untyped nil, since it's much less prone to
false positives.

Fixes #21.

From that text, it sounds to me like it should now be reporting the code as reported in the original issue (about reactions always returning nil error). However, testing the latest version of unparam, it still doesn't seem to be.

Is my understanding wrong, or is there a bug in the implementation?

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Feb 14, 2018

Owner

@shurcooL thanks for following up. I indeed closed this one too fast, as I never verified with the original test case.

Owner

mvdan commented Feb 14, 2018

@shurcooL thanks for following up. I indeed closed this one too fast, as I never verified with the original test case.

@mvdan mvdan reopened this Feb 14, 2018

mvdan added a commit that referenced this issue Feb 15, 2018

check: improve code to find forwarding returns
Fixes three false negatives in std cmd, all of which are good warnings.

Updates #21.

@mvdan mvdan closed this in 727ea8e Feb 15, 2018

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Feb 15, 2018

Owner

Ok, I did check this time. Please have a look at the two commits.

The last commit was a rare fix. So rare, it didn't even surface any new warnings on all of std cmd.

Owner

mvdan commented Feb 15, 2018

Ok, I did check this time. Please have a look at the two commits.

The last commit was a rare fix. So rare, it didn't even surface any new warnings on all of std cmd.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 15, 2018

Contributor

Thanks again; this is great!

Contributor

dmitshur commented Feb 15, 2018

Thanks again; this is great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment