quasilyte / go-ruleguard

Define and run pattern-based custom linting rules.
https://go-ruleguard.github.io/
BSD 3-Clause "New" or "Revised" License
794 stars 42 forks source link

Rejection-matching #79

Open seebs opened 4 years ago

seebs commented 4 years ago

Consider the popular errcheck analysis tool, which verifies that things which return error don't have that return ignored.

I can't figure out a way to express this in go-ruleguard. It seems like it could be done if it were possible to do inverted matches of some kind, so you could match a pattern, then un-match a longer pattern (like $x = ... in front of a thing). And while errcheck already exists, I want to do it to a code base that has another type that has similar properties.

quasilyte commented 4 years ago

You may write a rule that catches blank identifier assignments.

image

package gorules

import "github.com/quasilyte/go-ruleguard/dsl/fluent"

func foo(m fluent.Matcher) {
    m.Import("github.com/a/foo")

    m.Match(`_ = $x`).
        Where(m["x"].Type.Is("foo.MyType")).
        Report(`Don't ignore a value of type foo.MyType`)
}

If I understand you correctly, this can be close to what you would like to achieve.

$x is any expression of the required type. It can be a function call or some variable. If you only need to match function calls, it's also possible, but it may require some extra efforts.

seebs commented 4 years ago

Unfortunately, I think that's sort of the opposite of what I want.

What I want is a rule that matches "function returning T", but doesn't match "_ = [function returning T]". And this could sort of be done in ruleguard if there were a way to specify "...but doesn't match" rules.

I threw together a working-enough standalone example: github.com/molecula/noticeme. This is slightly more general, and just detects any unconsumed values of specified types.

quasilyte commented 4 years ago

Instead of concentrating on this new feature, it's easier to discuss your particular case in more details.

There are at least 2 main ways to ignore/not use the function result value:

  1. Use a function call as a statement. E.g. f()
  2. Assign a function result to _. E.g. _ = f()

For (2) I'm not sure why you say that the rule above is the opposite of what you want. It matches exactly that.

If you want to detect unused result that was not assigned to anything (1), then you probably want to check whether a function call expression is ast.ExprStmt. This can be done without rejection matching.

seebs commented 4 years ago

(2) is idiomatic for "i am aware of this return value and i'm discarding it intentionally". Thus, when using something like errcheck, people will write _ = fnReturningErr(...) as a way to indicate to the linter that they were aware of this, but accidental oversights get caught.

And yes, it turns out that "check whether a thing is an ExprStmt" is fairly close to the missing piece, but I think there's a couple of other exceptions I hadn't thought of in some contexts; for instance, if you also care about channel values (which I probably do sometimes), a case <-ch discards the value but isn't an ExprStmt. It turns out to be hard to express "in a context where the value isn't being used".

quasilyte commented 4 years ago

I think it's not impossible to match case clauses that do a receive without assignment. gogrep seems to have problems with incomplete syntax like case <-$x, but it should be possible to add some kludges here and there to make it work as intended.

But if you want a more thorough analysis of value usages, you probably want some form of SSA or taint analysis.