Giter Site home page Giter Site logo

quasilyte / go-ruleguard Goto Github PK

View Code? Open in Web Editor NEW
771.0 12.0 43.0 1.49 MB

Define and run pattern-based custom linting rules.

Home Page: https://go-ruleguard.github.io/

License: BSD 3-Clause "New" or "Revised" License

Go 97.78% Makefile 0.39% Dockerfile 0.16% Shell 1.66%
go golang gogrep linter static-analysis ruleguard go-analysis analysis dynamic-rules codeql semgrep

go-ruleguard's Introduction

go-ruleguard

Build Status Build Status PkgGoDev

Logo

Overview

analysis-based Go linter that runs dynamically loaded rules.

You write the rules, ruleguard checks whether they are satisfied.

ruleguard has some similarities with GitHub CodeQL, but it's dedicated to Go only.

Features:

  • Custom linting rules without re-compilation and Go plugins
  • Diagnostics are written in a declarative way
  • Quickfix actions support
  • Powerful match filtering features, like expression type pattern matching
  • Not restricted to AST rules; it's possible to write a comment-related rule, for example
  • Rules can be installed and distributed as Go modules
  • Rules can be developed, debugged and profiled using the conventional Go tooling
  • Integrated into golangci-lint

It can also be easily embedded into other static analyzers. go-critic can be used as an example.

Quick start

It's advised that you get a binary from the latest release {linux/amd64, linux/arm64, darwin/amd64, darwin/arm64, windows/amd64, windows/arm64}.

If you want to install the ruleguard from source, it's as simple as:

# Installs a `ruleguard` binary under your `$(go env GOPATH)/bin`
$ go install -v github.com/quasilyte/go-ruleguard/cmd/ruleguard@latest

# Get the DSL package (needed to execute the ruleguard files)
$ go get -v -u github.com/quasilyte/go-ruleguard/dsl@latest

If inside a Go module, the dsl package will be installed for the current module, otherwise it installs the package into the $GOPATH and it will be globally available.

If $GOPATH/bin is under your system $PATH, ruleguard command should be available after that:

$ ruleguard -help
ruleguard: execute dynamic gogrep-based rules

Usage: ruleguard [-flag] [package]

Flags:
  -rules string
    	comma-separated list of ruleguard file paths
  -e string
    	execute a single rule from a given string
  -fix
    	apply all suggested fixes
  -c int
    	display offending line with this many lines of context (default -1)
  -json
    	emit JSON output

Create a test rules.go file:

//go:build ruleguard
// +build ruleguard

package gorules

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

func dupSubExpr(m dsl.Matcher) {
	m.Match(`$x || $x`,
		`$x && $x`,
		`$x | $x`,
		`$x & $x`).
		Where(m["x"].Pure).
		Report(`suspicious identical LHS and RHS`)
}

func boolExprSimplify(m dsl.Matcher) {
	m.Match(`!($x != $y)`).Suggest(`$x == $y`)
	m.Match(`!($x == $y)`).Suggest(`$x != $y`)
}

func exposedMutex(m dsl.Matcher) {
	isExported := func(v dsl.Var) bool {
		return v.Text.Matches(`^\p{Lu}`)
	}

	m.Match(`type $name struct { $*_; sync.Mutex; $*_ }`).
		Where(isExported(m["name"])).
		Report("do not embed sync.Mutex")

	m.Match(`type $name struct { $*_; sync.RWMutex; $*_ }`).
		Where(isExported(m["name"])).
		Report("do not embed sync.RWMutex")
}

Create a test example.go target file:

package main

import "sync"

type EmbedsMutex struct {
	key int
	sync.Mutex
}

func main() {
	var v1, v2 int
	println(!(v1 != v2))
	println(!(v1 == v2))
	if v1 == 0 && v1 == 0 {
		println("hello, world!")
	}
}

Run ruleguard on that target file:

$ ruleguard -rules rules.go -fix example.go
example.go:5:1: exposedMutex: do not embed sync.Mutex (rules.go:24)
example.go:12:10: boolExprSimplify: suggestion: v1 == v2 (rules.go:15)
example.go:13:10: boolExprSimplify: suggestion: v1 != v2 (rules.go:16)
example.go:14:5: dupSubExpr: suspicious identical LHS and RHS (rules.go:7)

Since we ran ruleguard with -fix argument, both suggested changes are applied to example.go.

There is also a -e mode that is useful during the pattern debugging:

$ ruleguard -e 'm.Match(`!($x != $y)`)' example.go
example.go:12:10: !(v1 != v2)

It automatically inserts Report("$$") into the specified pattern.

You can use -debug-group <name> flag to see explanations on why some rules rejected the match (e.g. which Where() condition failed).

The -e generated rule will have e name, so it can be debugged as well.

How does it work?

First, it parses ruleguard files (e.g. rules.go) during the start to load the rule set.

Loaded rules are then used to check the specified targets (Go files, packages).

The rules.go file is written in terms of dsl API. Ruleguard files contain a set of functions that serve as a rule groups. Every such function accepts a single dsl.Matcher argument that is then used to define and configure rules inside the group.

A rule definition always starts with Match(patterns...) method call and ends with Report(message) method call.

There can be additional calls in between these two. For example, a Where(cond) call applies constraints to a match to decide whether it's accepted or rejected. So even if there is a match for a pattern, it won't produce a report message unless it satisfies a Where() condition.

Troubleshooting

For ruleguard to work the dsl package must be available at runtime. If it's not, you are going to see an error like:

$ ruleguard -rules rules.go .
ruleguard: load rules: parse rules file: typechecker error: rules.go:6:8: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")

This is fixed by adding the dsl package to the module:

$ ruleguard-test go get github.com/quasilyte/go-ruleguard/dsl
go: downloading github.com/quasilyte/go-ruleguard v0.3.18
go: downloading github.com/quasilyte/go-ruleguard/dsl v0.3.21
go: added github.com/quasilyte/go-ruleguard/dsl v0.3.21
$ ruleguard-test ruleguard -rules rules.go .
.../test.go:6:5: boolExprSimplify: suggestion: 1 == 0 (rules.go:9)

If you have followed past advise of using a build constraint in the rules.go file, like this:

$ ruleguard-test head -4 rules.go
//go:build ignore
// +build ignore

package gorules

you are going to notice that go.mod file lists the dsl as an indirect dependency:

$ grep dsl go.mod
require github.com/quasilyte/go-ruleguard/dsl v0.3.21 // indirect

If you run go mod tidy now, you are going to notice the dsl package disappears from the go.mod file:

$ go mod tidy
$ grep dsl go.mod
$ 

This is because go mod tidy behaves as if all the build constraints are in effect, with the exception of ignore. This is documented in the go website.

This is fixed by using a different build constraint, like ruleguard or rules.

Documentation

Rule set examples

Note: go-critic and go-perfguard embed the rules using the IR precompilation feature.

Extra references

go-ruleguard's People

Contributors

aleksi avatar alexandear avatar c-pro avatar coderaiser avatar cristaloleg avatar cristiangreco avatar deansheather avatar dependabot[bot] avatar dgryski avatar dstrelau avatar fexolm avatar harshavardhana avatar hnh avatar lamg avatar ldez avatar liggitt avatar lizthegrey avatar mem avatar mlevesquedion avatar orsinium avatar peakle avatar penthaapatel avatar quasilyte avatar sebastien-rosset avatar tb3457 avatar timkral avatar tmzane avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

go-ruleguard's Issues

add "const value relation" filters

To implement https://go-critic.github.io/overview#badcond we need to have a way to evaluate whether a constant captured by a submatch is lesser/greater than the other submatch.

This is a code from go-critic:

	// `x < a && x > b`; Where `a` is less than `b`.
	if c.lessAndGreater(lhs, rhs) {
		c.warnCond(cond, "always false")
		return
	}

We can't describe "Where a is less than b" condition yet.

Make it possible to assign a report related info

Especially useful for Go issue links.

Could be something like:

// Related parses the issue string and associates it with the report.
//
// Known formats:
//	* "#<num>"             => "https://golang.org/issue/<num>"
//	* "<org>/<repo>#<num>" => "https://github.com/<org>/<repo>/issues/<num>"
func (m Matcher) Related(issue string) Matcher {
	return m
}

Quoting nested quotes in DSL parser is broken

I've been trying to port this semgrep rule to ruleguard: dgryski/semgrep-go#1

I've gotten hung up on trying to port over the rules due to the different types of quoting involved. The DSL seems to need extra \ to escape double-quotes, but the those \ are expected to show up in the actual source text.

For example, the match text

m.Match("fmt.Sprintf(`\"%s\" <%s>`, $NAME, $EMAIL)")

Only matches the text

        fmt.Println(fmt.Sprintf(`\"%s\" <%s>`, "name", "[email protected]"))

and doesn't match the text without the slashes escaping the quotes

Decide on the selector expressions behavior

Given the path.Join(...) pattern, there are multiple interpretations that can be made:

  1. path could be a local variable (and Join is either a func-typed property or a method).
  2. path can be a package not from stdlib (e.g. github.com/blahblah/path).
  3. path is a stdlib package. (same as 2.)
  4. path is a type name, Join is a method name, path.Join is a method expression.

Our current behavior is based on what gogrep does. It's 100% syntactical, so it matches all of them.

Here is a proposal of how we can make rules that can match the pattern with a better precision.

func example() {
  // Behaves as before. Matches everything.
  m.Match(`path.Join($*_)`)

  // 1.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Var.Name("path"))

  // 2.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Package.Path("github.com/blahblah/path"))

  // 3.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Package.Path("path"))

  // 4.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].Type.Name("path"))
}

CodeQL doesn't have such a problem because it requires you to specify a type of all query variables in every query. gorules have no variable definitions concept, but we can infer their "node kind" constraint through Where clause.

This proposal introduces new fields of a matcher variable: Var, Package and Type (there can be more later). If Type.Name is used, m["p"] is expected to be a type.

A slightly different approach includes adding all predicates to the fluent.Var, as it's done in reflect package:

func example() {
  // Behaves as before. Matches everything.
  m.Match(`path.Join($*_)`)

  // 1.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].VarName("path"))

  // 2.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].PkgPath("github.com/blahblah/path"))

  // 3.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].PkgPath("path"))

  // 4.
  m.Match(`$p.Join($*_)`).
    Where(m["p"].TypeName("path"))
}

A more radical way is to introduce non-ambigous operators:

  • x.f() - f function call from a package x
  • x->f() - f method call from a variable x (rare case)
  • x::f() - f function call from a type x (very rare case)
func example() {
  // 1.
  m.Match(`path->Join($*_)`)

  // 2 and 3.
  // If custom "path" is needed, Import() can be used.
  m.Match(`path.Join($*_)`)

  // 4.
  m.Match(`path::Join($*_)`)
}

This is harder to implement and makes patterns even more incompatible with Go syntax. gogrep doesn't support such syntax => custom source preprocessing will be required.

Add support for matching imports (by prefix/substring)

This will enable custom and more flexible depguard-like rules.

This is needed, for example, to restrict packages inside pkg/ to import package inside internal/ - like grep -r "\"$(go list -m)/internal" pkg.
Probably this will also need ability to restrict to which packages this should be applied (to apply this rule only for packages in pkg/).

How to Suggest() when $*_ is involved?

When $*_ is used inside a pattern, there is no clear way to suggest a code replacement, since $*_ is unnamed and can't be interpolated into the suggestion.

Maybe there is a named $*_ variant in gogrep that I'm missing?

Add rules validation and simplification suggestions

Some AST patterns inside a rule group may be excessive or unreachable (if more generic patterns precede the more specific ones).

We can detect such problems during the rules load time.

It's also possible to give warnings that give hints on how the rule set can be improved (enabled by default in interactive mode but should probably be turned off on CI environments).

breaking api change in ruleguard.go -> Context -> Report ?

See https://github.com/quasilyte/go-ruleguard/pull/64/files#r469483412 for the exact location of the problematic change

Here is how the error materializes itself in a project that uses go-ruleguard (in this case, go-critic):

[16:55:23] :	 [Step 5/6] #21 51.74 /go/pkg/mod/github.com/go-critic/[email protected]/checkers/ruleguard_checker.go:81:3: cannot use func literal (type func(ast.Node, string, *ruleguard.Suggestion)) as type func(ruleguard.GoRuleInfo, ast.Node, string, *ruleguard.Suggestion) in field value
[16:55:23] :	 [Step 5/6] #21 57.86 make: *** [GNUmakefile:183: lint-deps] Error 2

Typical dependency chain is as follows:
-> golangci-lint -> go-critic -> go-ruleguard

if uses go get -u to install packages, the dependency tree gets updated and go-critic can't build so the build breaks.

I think this is would be considered a breaking change by semver rules, but given it's all pre-release, I guess all bets are off. Up to you to decide how to best address this :)

panic in go/types.(*Package).Path(...) with a well-formed source file and rule

Here's a minimized source file:

package store
import (
	"io"
	"io/ioutil"
	"strings"
	"testing"
)
type Store interface {
	Create(key string, expectedChecksum string, r io.ReadCloser) error
}
func content(s string) io.ReadCloser {
	return ioutil.NopCloser(strings.NewReader(s))
}
func TestStore(t *testing.T, s Store, kind string) {
	if err := s.Create("c", "deadbeef", content("3")); err == nil {
		t.Errorf("store %v: nil error from invalid checkum", kind)
	}
}

And here is a check with panic output:

~/go/src/github.com/Quasilyte/go-ruleguard/cmd/ruleguard/store $ ../ruleguard  -e 'm.Match("$t0 == $t1").Where(m["t0"].Type.Is("time.Time")).Report("using == with time.Time")'  .
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12d7f84]

goroutine 257 [running]:
go/types.(*Package).Path(...)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/types/package.go:30
github.com/quasilyte/go-ruleguard/ruleguard/typematch.(*Pattern).matchIdentical(0xc0002f4600, 0xc00042b0e0, 0x1471f20, 0xc00005abe0, 0xc00005abe0)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/typematch/typematch.go:328 +0x824
github.com/quasilyte/go-ruleguard/ruleguard/typematch.(*Pattern).MatchIdentical(0xc0002f4600, 0x1471f20, 0xc00005abe0, 0x0)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/typematch/typematch.go:234 +0x71
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesParser).walkFilter.func6(0x1471f20, 0xc00005abe0, 0xc00042b380, 0x1471f20)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/parser.go:505 +0x46
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).handleMatch(0xc0000105c0, 0x0, 0x0, 0xc0002f4800, 0xc00049cce1, 0x17, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:103 +0x874
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).run.func2.1(0x1471160, 0xc000124a50, 0xc00042b3b0)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:77 +0xab
github.com/quasilyte/go-ruleguard/internal/mvdan.cc/gogrep.(*Pattern).MatchNode(0xc0002f4800, 0x1471160, 0xc000124a50, 0xc0001cef18)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/internal/mvdan.cc/gogrep/kludge.go:41 +0xb4
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).run.func2(0x1471160, 0xc000124a50, 0xc00038f901)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:76 +0x15b
go/ast.inspector.Visit(0xc000044890, 0x1471160, 0xc000124a50, 0x146fb80, 0xc000044890)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:373 +0x3a
go/ast.Walk(0x146fb80, 0xc000044890, 0x1471160, 0xc000124a50)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:52 +0x66
go/ast.Walk(0x146fb80, 0xc000044890, 0x14717a0, 0xc0000111c0)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:230 +0x1c44
go/ast.walkStmtList(0x146fb80, 0xc000044890, 0xc00038f9c0, 0x1, 0x1)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:32 +0x9e
go/ast.Walk(0x146fb80, 0xc000044890, 0x14711a0, 0xc000124ae0)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:224 +0x1adf
go/ast.Walk(0x146fb80, 0xc000044890, 0x1471620, 0xc000124b10)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:344 +0xd98
go/ast.walkDeclList(0x146fb80, 0xc000044890, 0xc000010f80, 0x4, 0x4)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:38 +0x9e
go/ast.Walk(0x146fb80, 0xc000044890, 0x14715a0, 0xc000110980)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:353 +0x264e
go/ast.Inspect(...)
	/Users/dgryski/go/src/go.googlesource.com/go/src/go/ast/walk.go:385
github.com/quasilyte/go-ruleguard/ruleguard.(*rulesRunner).run(0xc0000105c0, 0xc000110980, 0x0, 0x1780108)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/runner.go:72 +0x24f
github.com/quasilyte/go-ruleguard/ruleguard.RunRules(...)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/ruleguard/ruleguard.go:30
github.com/quasilyte/go-ruleguard/analyzer.runAnalyzer(0xc0000c4180, 0xc0000c4180, 0x0, 0x0, 0x0)
	/Users/dgryski/go/src/github.com/quasilyte/go-ruleguard/analyzer/analyzer.go:70 +0x208
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc00010a6e0)
	/Users/dgryski/go/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:690 +0x83a
sync.(*Once).doSlow(0xc00010a6e0, 0xc00031af90)
	/Users/dgryski/go/src/go.googlesource.com/go/src/sync/once.go:66 +0xec
sync.(*Once).Do(...)
	/Users/dgryski/go/src/go.googlesource.com/go/src/sync/once.go:57
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(0xc00010a6e0)
	/Users/dgryski/go/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:579 +0x60
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0xc00010a6e0)
	/Users/dgryski/go/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:567 +0x34
created by golang.org/x/tools/go/analysis/internal/checker.execAll
	/Users/dgryski/go/src/golang.org/x/tools/go/analysis/internal/checker/checker.go:573 +0x119

Rejection-matching

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.

Add sub-match support

Some examples where it can be useful:

// check that some function call is not followed by a return.
m.Match(`foo(err); $x`).
	Where(!m["x"].Matches(`return err`)).
	Report(`foo(err) call not followed by return err`)

// Check that for loop contains expensive() call.
// We can't do `for { $*_; expensive(); $*_ } since it would only
// check that expensive() is called without any extra nesting.
// If call is done inside if statement or inside another block,
// it will not trigger.
m.Match(`for { $x }`).
	Where(m["x"].Matches(`expensive()`).
	Report(`expensive call inside a loop`)

// we want to check that $y contains $x[0] somewhere
// inside it's AST.
m.Match(`$x == nil && $y`).
	Where(m["y"].Matches(`$x[0]`)).
	Report(`suspicious $$; nil check may not be enough to access $x[0], check for len`)

Add suggested fix support (quickfixes)

analysis supports SuggestedFixes and we want to use that.

It's proposed to add Suggest() DSL method in order to support that.

Suggest() accepts a single "suggestion" argument that represents a replacement code pattern. If a rule is matched, that replacement is used in a quickfix suggestion.

If a rule doesn't have Report() we use a string from Suggest() as a base for report message in reporting mode. So there is no need to add a non-informative report calls for rules that are intended to be suggestions-like.

Implementation plan:

  • basic suggestions support
  • add tests that check that quickfixes with -fix actually work
  • update docs to clarify that Suggest can be used instead of Report

Flaky results: Same rule produces different results (Root cause: regexp not getting parsed)

My rules:

func _(m fluent.Matcher) {
	m.Match(`$s != $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t != $s`)
	m.Match(`$s == $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t == $s`)
	m.Match(`$s >= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t <= $s`)
	m.Match(`$s <= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t >= $s`)
	m.Match(`$s > $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t < $s`)
	m.Match(`$s < $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t > $s`)
	m.Match(`nil == $s`).Where(!m["s"].Const).Suggest(`$s == nil`)
	m.Match(`nil != $s`).Where(!m["s"].Const).Suggest(`$s != nil`)
	m.Match(`log.Infof($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Info($s)`)
	m.Match(`log.Warnf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Warn($s)`)
	m.Match(`log.Errorf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Error($s)`)
}

Results:
Screenshot 2020-07-12 at 11 45 05 AM

rulegard: make multiple type predicates possible

Right now every type-based predicate overrides the previous one.
It should be composable.

This should define 2 && connected predicates:

pred1 && pred2

Right now it generates a predicate that only includes pred2.

Match dynamic interfaces with dynamic type

I tried to write a matcher that matches

x == y and x != y

as well as the condition that for those variables exist a method name Equal with the signature
(r T) Equal(other T) bool
where the the type T should be dynamic.

The reason is that I want enforce the usage of this method and maybe even automatically fix this.

We have a lot of types where we need this. So doing one rule for each type is not really feasible.

Investigate https://twitter.com/oldmanuk/status/1297285023129903104

https://gist.github.com/dnwe/872f66df252a7fb8a4405e7b6448b199

diff --git a/ruleguard.rules.go b/ruleguard.rules.go
index 5d4c4fe..8d2f8c1 100644
--- a/ruleguard.rules.go
+++ b/ruleguard.rules.go
@@ -130,6 +130,34 @@ func errnoterror(m fluent.Matcher) {
                Report("err variable not error type")
 }

+// err found but return nil
+func errfoundreturnnil(m fluent.Matcher) {
+       m.Match(
+               "if $*_, err := $x; $err != nil { return nil } else if $_ { $*_ }",
+               "if $*_, err := $x; $err != nil { return nil } else { $*_ }",
+               "if $*_, err := $x; $err != nil { return nil }",
+
+               "if $*_, err = $x; $err != nil { return nil } else if $_ { $*_ }",
+               "if $*_, err = $x; $err != nil { return $*_, nil } else { $*_ }",
+               "if $*_, err = $x; $err != nil { return $*_, nil }",
+
+               "if err := $x; $err != nil { return nil } else if $_ { $*_ }",
+               "if err := $x; $err != nil { return $*_, nil } else { $*_ }",
+               "if err := $x; $err != nil { return $*_, nil }",
+
+               "if err := $x; $err != nil { return $*_, nil } else { $*_ }",
+               "if err := $x; $err != nil { return $*_, nil }",
+
+               "if err = $x; $err != nil { return $*_, nil } else { $*_ }",
+               "if err = $x; $err != nil { return $*_, nil }",
+
+               "if $err != nil { return nil }",
+               "if $err != nil { return $*_, nil }",
+       ).
+               Where(m["err"].Type.Is("error")).
+               Report("err value found but return value was nil")
+}

Add context-related filters?

To implement check like bare-return, we need to know that return is done inside a function that has named return values.

Another example is flag-param where we want to know that a variable used inside if statement condition is a bool-typed function param.

Checking whether a current function/file is test related can also be useful.

Where(m[...].Text != "...") not working as expected (with example)

Executing ruleguard -rules rules.go -fix example.go with the following rules.go and example.go does not work as expected.

rules.go:


package gorules

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

func _(m fluent.Matcher) {
        m.Match(
                `func $_($_ *testing.T) { $p;$*_ }`,
        ).
                Where(m["p"].Text != "t.Parallel()").
                Report(`$f should only match where "t.Parallel()" != "$p", but it matches everything"`)
}

example.go:

package main

import (
        "testing"
)

func TestParallel(t *testing.T) {
        t.Parallel()
}

func TestNotParallel(t *testing.T) {
        t.Fatalf("This test should fail")
}

Expectation: Only the TestNotParallel function would match as it is the only function that has t.Parallel() in the body but both TestNotParallel and TestParallel are matched.

ruleguard was git pulled on Aug 20 2020, ruleguard -V reports the error "ruleguard: unsupported flag value: -V=true"

Figure out how to express "go vet shift" check

https://github.com/golang/tools/blob/master/go/analysis/passes/shift/shift.go

We can get somewhat close with this:

m.Match(`$x >> 8`, `$x >>= 8`).
		Where(!m["x"].Const && m["x"].Type.Size == 1).
		Report(`$x (8 bits) too small for shift of 8`)

We need !m["x"].Const to avoid false positives:
https://github.com/golang/tools/blob/89082a3841783366cb82b3dc4ce9258fb3dad1a9/go/analysis/passes/shift/shift.go#L77-L82

The problem is: we need to enumerate all shift constants that overflow a given size.

This would be a better solution:

m.Match(`$x >> $n`, `$x >>= $n`).
		Where(!m["x"].Const && m["x"].Type.Size == 1 && m["n"].Int() >= 8).
		Report(`$x (8 bits) too small for shift of 8`)

Where Int() tries to evaluate a constant value of the expression and convert it to int.
We still need to repeat that rule for every integer size: 8, 16, 32 and 64, but it's far better than not being able to express such diagnostic at all.

Add -e (eval) flag for simpler rule debugging?

Right now you have to create a rules file, add several lines of boilerplate and only after that you can start experimenting with ruleguard features.

I propose a -e flag that makes this use case easier.

Given this rules.go file:

// +build ignore

package gorules

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

func _(m fluent.Matcher) {
	m.Match(`json.NewDecoder($_).Decode($_)`).Report(`$$`)
}

Could be expressed as:

ruleguard -e 'm.Match(`json.NewDecoder($_).Decode($_)`)'

Notable points:

  • Matcher variable is bound to m.
  • Generated rule group is anonymous.
  • Report("$$") is always appended to the -e argument string.
  • If multiple statements are required, ; can be used, but the executed rule should be the last statement.

More examples:

ruleguard -e 'm.Match(`time.Duration($x)*time.Second`).Where(m["x"].Const)'
ruleguard -e 'm.Match(`($a) || ($b)`)'

Output with patterns isn't expanded when patterns match long (> 60 character) identifiers.

Running ruleguard -rules rules.go -fix example.go outputs "$f should be expanded to the name" but expected the output "ZeroOneTwoThreeFourFiveSixSevenEightNineTenElevenTwelveThirte should be expanded to the name" when executed with the following files.

example.go

// +build ignore

package gorules

func ZeroOneTwoThreeFourFiveSixSevenEightNineTenElevenTwelveThirte() {
}

rules.go

// +build ignore

package gorules

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

func MatchesFuncsWithLongNames(m fluent.Matcher) {
        m.Match(
                `func $f($*_) { $*_ }`,
        ).
                Report(`$f should be expanded to the name`)
}

This appears to be related to the limit placed here: https://github.com/quasilyte/go-ruleguard/blob/master/ruleguard/runner.go#L191

This limit was bumped from 40 to 60 in #63

Since many engineers use long function names for unit tests (BDD) I suggest the limit be increased to at least 120 (or removed altogether). An example of a long unit test function name could be.

func TestRuleguardOutput_CorrectlyExpandsMatchingVariablesWhenMatchingLongIdentifiers(t *testing.T) {

gorules: import: table is overwritten by module name collisions

Right now import table gets overwritten on collisions:

I can do m.Import("foo/pkg") and write rules against that package, but if checked code imports bar/pkg then rules ignore m.Import and are applied against bar/pkg.

example repo

m.Import("github.com/bradfitz/gomemcache/memcache")
m.Match("memcache.New").Report("usage of raw memcached client")

import (
	"github.com/utrack/go-ruleguard-import-collision/code/memcache"
)

func SomeFunc() {
	memcache.New()
}
ruleguard -rules ./gorules/rules.go ./code/...
.../go/src/github.com/utrack/go-ruleguard-import-collision/code/code.go:8:2: usage of raw memcached client

Add a "current package" filter

This can be useful to avoid reporting code that implements a function that we're suggesting.

For example, if we recognize some code and suggest strings.HasPrefix, we don't want to recommend using strings.HasPrefix inside strings.HasPrefix function body.

ruleguard: interpolate submatches using src slices instead of using printer

Example:

_ = fmt.Sprintf("%t", (i+i)&1 == 0) // want `use strconv.FormatBool\(\(i \+ i\)&1 == 0\)`

We get i + i instead of i+i because gofmt formats the expression differently from Go printer.
It would be better to interpolate matches expressions "as is", by using a pos-based original source code slicing.

Support named types from stdlib in filter expressions

Right now it's impossible to write Type.Is("io.Reader") or any other type filter that uses named type from a stdlib.

In the future, we want to support all types, both coming from GOROOT and user-defined types, but supporting stdlib types seems like a good first step into that direction.

ruleguard: consider || operator support in Where cond

Needed for context-keys-type checker implementation.

m.Match(`context.WithValue($_, $k, $_)`).
	Where(m["k"].Type.Is("bool") ||
		m["k"].Type.Is("int") ||
		m["k"].Type.Is("int8") ||
		m["k"].Type.Is("int16") ||
		m["k"].Type.Is("int32") ||
		m["k"].Type.Is("int64") ||
		m["k"].Type.Is("uint") ||
		m["k"].Type.Is("uint8") ||
		m["k"].Type.Is("uint16") ||
		m["k"].Type.Is("uint32") ||
		m["k"].Type.Is("uint64") ||
		m["k"].Type.Is("uintptr") ||
		m["k"].Type.Is("float32") ||
		m["k"].Type.Is("float64") ||
		m["k"].Type.Is("complex64") ||
		m["k"].Type.Is("complex128") ||
		m["k"].Type.Is("string")).
	Report(`should not use basic type as key in context.WithValue`)

Or maybe we need to add "basic" type predicate for now.

Quickfixes may result in "imported but not used" errors

Imagine that we're replacing an fmt.Sprint(x) call with s.String(). If that was the only fmt package usage inside that file, we'll get an error afterward.

It would be nice if quickfix could handle that and remove unused imports.

ruleguard/typematch: support func type patterns

In addition to basic per-member types, we may want the $*_ to match zero or more members.

func ($*_) $*_ // any kind of function
func ($_) // function of 1 input and 0 outputs
func () $_ // function of 0 inputs and 1 output
func ($*_, int, $*_) int // function that accepts at least 1 int and returns int

Improve godoc when fluent.Var.Type matches function call

In the fluent.Var.Type godoc, I suggest clarifying how the variable Type is matched against various kinds of AST nodes.

The AST node has an ObjKind which for example could be a ObjKind.Var (variable) or ObjKind.Fun (function). There are many examples showing how the Where clause is processed when the node value is matched against a ObjKind.Var. It's not so clear when the node is ObjKind.Fun, i.e. when the node is a CallExpr. When the function returns a single value, it would be tempting to think the type is matched against the return value, but it's probably not how it works.

Example:

func _(m fluent.Matcher) {
  m.Match(`$x.Walk($y)`).Where(m["x"].Addressable && m["x"].Type.Is("*Person")).Report(...)
}
type Person struct {}
func (p *Person) Walk() {
}

func main() {
   p *Person := ...
   // Invoke Walk(), Type of p is *Person
   p.Walk()

   // Invoke Walk() again, suppose there is a getPerson() function that returns a *Person
   getPerson().Walk()
}

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.