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

doesn't match already declared in previous expression variable in new expression #339

Closed
peakle opened this issue Jan 1, 2022 · 3 comments · Fixed by #340
Closed

doesn't match already declared in previous expression variable in new expression #339

peakle opened this issue Jan 1, 2022 · 3 comments · Fixed by #340

Comments

@peakle
Copy link
Contributor

peakle commented Jan 1, 2022

rule doesn't match variable initialized through := declaration in previous expression

rule: https://github.com/delivery-club/delivery-club-rules/blob/simplifyErrCheck/rules.go#L413
testdata: https://github.com/delivery-club/delivery-club-rules/blob/simplifyErrCheck/testdata/src/simplifyErrorCheck/a.go#L13

used ruleguard version 0.3.14

@quasilyte
Copy link
Owner

This is probably because the first match stops the other rules from being applied.
This behavior is intuitive with single statement/expression patterns, but it gets more and more surprising the broader the scope becomes. In your example, a whole function is a "scope", so it short-circuits from the function as soon as the first rule matches.

Maybe it should be fixed for stmtList and exprList nodes, I'm not sure yet.

As a side note, patterns like that (from your referenced example) are quite fragile and should not be relied upon too much. ruleguard works best for the expression level and single statement level. It's also good enough for the X ; Y cases, where one statement is immediately followed by another, like in foo(); bar(). Other (more complicated) patterns are usually a sign that this diagnostic should be implemented manually (for example, in go-critic normal checkers).

@quasilyte
Copy link
Owner

As a side note, will this pattern be good enough in your case?

$err = $f($*args); if err != nil { $*_ }

Without surrounding $*_ and func.

@peakle
Copy link
Contributor Author

peakle commented Jan 2, 2022

As a side note, will this pattern be good enough in your case?

$err = $f($*args); if err != nil { $*_ }

Without surrounding $*_ and func.

yes, i also thougth about this pattern after your comment, also i split rule into two unrelated groups, but problem still reproducable: delivery-club/delivery-club-rules@f77c82c

quasilyte added a commit that referenced this issue Jan 2, 2022
This is probably a temporary and sub-optimal solution, but
it should do for now.

Since matches are much more rare than pattern misses,
we do the check under the `matched` condition.
Extra condition shouldn't make anything measurably slower there.

Fixes #339
quasilyte added a commit that referenced this issue Jan 2, 2022
This is probably a temporary and sub-optimal solution, but
it should do for now.

Since matches are much more rare than pattern misses,
we do the check under the `matched` condition.
Extra condition shouldn't make anything measurably slower there.

Fixes #339
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

Successfully merging a pull request may close this issue.

2 participants