Go error rules#82
Conversation
mschwager
left a comment
mschwager
left a comment
There was a problem hiding this comment.
Nice work. It looks like LLMs create very verbose Semgrep rules, and probably attempt to brute force various scenarios rather than generalize rules. I have had some success by telling LLMs to review existing rules before creating new ones so it can observe some of our more advanced usage like metavariable-pattern.
Semgrep has a lot of implicit equivalences, which are difficult for an LLM to learn. Perhaps we can encode this knowledge somewhere, but for now it will probably require a bit of human review.
| - pattern-inside: | | ||
| switch { | ||
| case $L1: | ||
| ... | ||
| } | ||
| $NEXT |
| - pattern-inside: | | ||
| if $C1 { | ||
| ... | ||
| } | ||
| $NEXT | ||
| - pattern-inside: | | ||
| if $I1; $C1 { | ||
| ... | ||
| } | ||
| $NEXT | ||
| - pattern-inside: | | ||
| switch $X1 { | ||
| case $L1: | ||
| ... | ||
| } | ||
| $NEXT | ||
| - pattern-inside: | | ||
| switch $I1; $X1 { | ||
| case $L1: | ||
| ... | ||
| } | ||
| $NEXT | ||
| - pattern-inside: | | ||
| switch { | ||
| case $L1: | ||
| ... | ||
| } | ||
| $NEXT | ||
| - pattern-inside: | | ||
| switch $X1 { | ||
| default: | ||
| ... | ||
| } | ||
| $NEXT | ||
| - pattern-inside: | | ||
| switch { | ||
| default: | ||
| ... | ||
| } | ||
| $NEXT |
There was a problem hiding this comment.
I think these 7 pattern-insides can be reduced to 3 that cover the same cases: https://semgrep.dev/playground/s/Ok7qb
Kinda annoying that switch ... { ... } doesn't catch all switch statements (could probably be raised as an upstream issue). And you need to include a case to make a valid pattern, and it also catches just default switch statements! 😕
| - metavariable-regex: | ||
| metavariable: $TERM | ||
| regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\(|runtime\.Goexit\(|continue\b|break\b|goto\b)' |
There was a problem hiding this comment.
I think this can be better expressed using metavariable-pattern: https://semgrep.dev/playground/s/er9dE
| - pattern: | | ||
| if $C2 { | ||
| ... | ||
| } | ||
| $TERM | ||
| - pattern: | | ||
| if $I2; $C2 { | ||
| ... | ||
| } | ||
| $TERM | ||
| - pattern: | | ||
| switch $X2 { | ||
| case $L2: | ||
| ... | ||
| } | ||
| $TERM | ||
| - pattern: | | ||
| switch $I2; $X2 { | ||
| case $L2: | ||
| ... | ||
| } | ||
| $TERM | ||
| - pattern: | | ||
| switch { | ||
| case $L2: | ||
| ... | ||
| } | ||
| $TERM | ||
| - pattern: | | ||
| switch $X2 { | ||
| default: | ||
| ... | ||
| } | ||
| $TERM | ||
| - pattern: | | ||
| switch { | ||
| default: | ||
| ... | ||
| } | ||
| $TERM |
There was a problem hiding this comment.
Same as above, this can be 3 statements instead of 7.
| - metavariable-regex: | ||
| metavariable: $TERM | ||
| regex: '^(return\b|panic\(|log\.Fatal(ln|f)?\(|os\.Exit\(|runtime\.Goexit\(|continue\b|break\b|goto\b)' |
There was a problem hiding this comment.
Same as above, metavariable-pattern.
| - metavariable-pattern: | ||
| metavariable: $RET | ||
| patterns: | ||
| - pattern-either: | ||
| - pattern: return errors.$FN($ERR, ...) | ||
| - pattern: $ERR = errors.$FN($ERR, ...) | ||
| # `... $RET ...` could bind $RET to any stmt-level node | ||
| # (including an enclosing if-stmt containing the wrap). | ||
| # Anchor $RET's text to the return/assignment shape. | ||
| - metavariable-regex: | ||
| metavariable: $RET | ||
| regex: '^(return\b|[a-zA-Z_][a-zA-Z0-9_]*\s*=(?!=))' |
There was a problem hiding this comment.
What is this trying to do? So $RET needs to be one of the pattern-eithers above AND this regex? I'm confused. Could we instead include this constraint in the metavariable-pattern above?
| # $ERR reassigned (or `:=`-shadowed) IMMEDIATELY before the | ||
| # wrap (return-form) inside the else body. Tight-adjacent only. | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| $ERR = ... | ||
| return errors.$FN($ERR, ...) | ||
| } | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| ..., $ERR = ... | ||
| return errors.$FN($ERR, ...) | ||
| } | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| $ERR := ... | ||
| return errors.$FN($ERR, ...) | ||
| } | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| ..., $ERR := ... | ||
| return errors.$FN($ERR, ...) | ||
| } |
There was a problem hiding this comment.
All 4 of these assignment cases should be covered by a single $ERR = ...: https://semgrep.dev/playground/s/Lr7W7
| # Same shape for the assignment-form wrap. | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| $ERR = ... | ||
| $ERR = errors.$FN($ERR, ...) | ||
| } | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| ..., $ERR = ... | ||
| $ERR = errors.$FN($ERR, ...) | ||
| } | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| $ERR := ... | ||
| $ERR = errors.$FN($ERR, ...) | ||
| } | ||
| - pattern-not: | | ||
| if $ERR != nil { | ||
| ... | ||
| } else { | ||
| ... | ||
| ..., $ERR := ... | ||
| $ERR = errors.$FN($ERR, ...) | ||
| } |
| - pattern-not-inside: | | ||
| $ERR = ... | ||
| return errors.$FN($ERR, ...) | ||
| - pattern-not-inside: | | ||
| ..., $ERR = ... | ||
| return errors.$FN($ERR, ...) | ||
| - pattern-not-inside: | | ||
| $ERR := ... | ||
| return errors.$FN($ERR, ...) | ||
| - pattern-not-inside: | | ||
| ..., $ERR := ... | ||
| return errors.$FN($ERR, ...) |
| - pattern-not-inside: | | ||
| $ERR = ... | ||
| $ERR = errors.$FN($ERR, ...) | ||
| - pattern-not-inside: | | ||
| ..., $ERR = ... | ||
| $ERR = errors.$FN($ERR, ...) | ||
| - pattern-not-inside: | | ||
| $ERR := ... | ||
| $ERR = errors.$FN($ERR, ...) | ||
| - pattern-not-inside: | | ||
| ..., $ERR := ... | ||
| $ERR = errors.$FN($ERR, ...) |
2 participants
Adds three rules:
shadowed-err-check—if xErr := f(); err != nildeclares one err-like var but checks a different one (typo / copy-paste).pkg-errors-wrap-nil-err—pkg/errors.Wrap/Wrapf/WithMessage/WithMessagef/WithStackcalled on a provably-nil err; these return nil for nil input, silently swallowing the failure path.http-error-missing-return—http.Error(w, ...)not followed byreturn; handler keeps running after the error response.I used regexes for error detection: typed metavariables don't seem to work for this type.