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.

Sign up for GitHub

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

Jump to bottom

More rules for strings module functions #96

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

More rules for strings module functions #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d5fe1b
migrate tests
orsinium Oct 23, 2020
77e183b
migrate rules
orsinium Oct 23, 2020
75c27a0
provide rules for bytes.Count
orsinium Oct 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions analyzer/analyzer_test.go
View file
Open in desktop
Expand Up @@ -32,3 +32,14 @@ func TestAnalyzer(t *testing.T) {
})
}
}

func TestAnalyzer_Rules_LocalConfig(t *testing.T) {
test := "rules"
filename := "../rules.go"
testdata := analysistest.TestData()
err := analyzer.Analyzer.Flags.Set("rules", filename)
if err != nil {
t.Fatalf("set rules flag: %v", err)
}
analysistest.Run(t, testdata, analyzer.Analyzer, test)
}
75 changes: 75 additions & 0 deletions analyzer/testdata/src/rules/main.go
View file
Open in desktop
@@ -0,0 +1,75 @@
// nolint
// This package tests rules specified in `rules.go` of ruleguard itself.
package main

import (
"bytes"
"fmt"
"os"
"strings"
)

const () // want `\Qempty const() block`
var () // want `\Qempty var() block`
type () // want `\Qempty type() block`

func badFmt() {
fmt.Fprint(os.Stdout, "hello") // want `\Qsuggestion: fmt.Print("hello")`
fmt.Fprintln(os.Stdout, "hello") // want `\Qsuggestion: fmt.Println("hello")`
fmt.Fprintf(os.Stdout, "%d", 123) // want `\Qsuggestion: fmt.Printf("%d", 123)`
}

func badString() {
s1 := "oh"
s2 := "hi"
s3 := "mark"

strings.Replace(s1, s2, s3, -4) // want `\Qsuggestion: strings.ReplaceAll(s1, s2, s3)`
strings.Replace(s1, s2, s3, -1) // want `\Qsuggestion: strings.ReplaceAll(s1, s2, s3)`
strings.Replace(s1, s2, s3, 0) // want `\Qsuggestion: strings.ReplaceAll(s1, s2, s3)`
strings.Replace(s1, s2, s3, 1)
strings.Replace(s1, s2, s3, 2)

strings.SplitN(s1, s2, -13) // want `\Qsuggestion: strings.Split(s1, s2)`
strings.SplitN(s1, s2, -1) // want `\Qsuggestion: strings.Split(s1, s2)`
strings.SplitN(s1, s2, 0) // want `\Qsuggestion: strings.Split(s1, s2)`
strings.SplitN(s1, s2, 1) // want `\Qsuggestion: strings.SplitN(s1, s2, 2)`
strings.SplitN(s1, s2, 2)

strings.SplitAfterN(s1, s2, -13) // want `\Qsuggestion: strings.SplitAfter(s1, s2)`
strings.SplitAfterN(s1, s2, -1) // want `\Qsuggestion: strings.SplitAfter(s1, s2)`
strings.SplitAfterN(s1, s2, 0) // want `\Qsuggestion: strings.SplitAfter(s1, s2)`
strings.SplitAfterN(s1, s2, 1) // want `\Qsuggestion: strings.SplitAfterN(s1, s2, 2)`
strings.SplitAfterN(s1, s2, 2)

p := fmt.Println
p(strings.Count(s1, s2) == 0) // want `\Qsuggestion: !strings.Contains(s1, s2)`
p(strings.Count(s1, s2) >= 0) // want `statement always true`
p(strings.Count(s1, s2) > 0) // want `\Qsuggestion: strings.Contains(s1, s2)`
p(strings.Count(s1, s2) >= 1) // want `\Qsuggestion: strings.Contains(s1, s2)`
p(strings.Count(s1, s2) > 1)
}

func badBytes() {
s1 := []byte("oh")
s2 := []byte("hi")
s3 := []byte("mark")

bytes.Replace(s1, s2, s3, -4) // want `\Qsuggestion: bytes.ReplaceAll(s1, s2, s3)`
bytes.Replace(s1, s2, s3, -1) // want `\Qsuggestion: bytes.ReplaceAll(s1, s2, s3)`
bytes.Replace(s1, s2, s3, 0) // want `\Qsuggestion: bytes.ReplaceAll(s1, s2, s3)`
bytes.Replace(s1, s2, s3, 1)
bytes.Replace(s1, s2, s3, 2)

bytes.SplitN(s1, s2, -13) // want `\Qsuggestion: bytes.Split(s1, s2)`
bytes.SplitN(s1, s2, -1) // want `\Qsuggestion: bytes.Split(s1, s2)`
bytes.SplitN(s1, s2, 0) // want `\Qsuggestion: bytes.Split(s1, s2)`
bytes.SplitN(s1, s2, 1) // want `\Qsuggestion: bytes.SplitN(s1, s2, 2)`
bytes.SplitN(s1, s2, 2)

bytes.SplitAfterN(s1, s2, -13) // want `\Qsuggestion: bytes.SplitAfter(s1, s2)`
bytes.SplitAfterN(s1, s2, -1) // want `\Qsuggestion: bytes.SplitAfter(s1, s2)`
bytes.SplitAfterN(s1, s2, 0) // want `\Qsuggestion: bytes.SplitAfter(s1, s2)`
bytes.SplitAfterN(s1, s2, 1) // want `\Qsuggestion: bytes.SplitAfterN(s1, s2, 2)`
bytes.SplitAfterN(s1, s2, 2)
}
56 changes: 43 additions & 13 deletions rules.go
View file
Open in desktop
Expand Up @@ -2,7 +2,9 @@

package gorules

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

// This is an example rule file for ruleguard.
//
Expand Down Expand Up @@ -48,6 +50,15 @@ func _(m fluent.Matcher) {
Suggest(`strings.Contains($s1, $s2)`)
m.Match(`strings.Count($s1, $s2) == 0`).
Suggest(`!strings.Contains($s1, $s2)`)
m.Match(`strings.Count($s1, $s2) >= 0`).
Report(`statement always true`)
m.Match(`bytes.Count($s1, $s2) > 0`,
`bytes.Count($s1, $s2) >= 1`).
Suggest(`bytes.Contains($s1, $s2)`)
m.Match(`bytes.Count($s1, $s2) == 0`).
Suggest(`!bytes.Contains($s1, $s2)`)
m.Match(`bytes.Count($s1, $s2) >= 0`).
Report(`statement always true`)

m.Match(`sort.Slice($s, func($i, $j int) bool { return $s[$i] < $s[$j] })`).
Where(m["s"].Type.Is(`[]string`)).
Expand Down Expand Up @@ -173,15 +184,11 @@ func useMathBits(m fluent.Matcher) {
}

func gocriticWrapperFunc(m fluent.Matcher) {
m.Match(`strings.SplitN($s, $sep, -1)`).Suggest(`strings.Split($s, $sep)`)
m.Match(`strings.Replace($s, $old, $new, -1)`).Suggest(`strings.ReplaceAll($s, $old, $new)`)
m.Match(`strings.TrimFunc($s, unicode.IsSpace)`).Suggest(`strings.TrimSpace($s)`)
m.Match(`strings.Map(unicode.ToUpper, $s)`).Suggest(`strings.ToUpper($s)`)
m.Match(`strings.Map(unicode.ToLower, $s)`).Suggest(`strings.ToLower($s)`)
m.Match(`strings.Map(unicode.ToTitle, $s)`).Suggest(`strings.ToTitle($s)`)

m.Match(`bytes.SplitN($s, $sep, -1)`).Suggest(`bytes.Split($s, $sep)`)
m.Match(`bytes.Replace($s, $old, $new, -1)`).Suggest(`bytes.ReplaceAll($s, $old, $new)`)
m.Match(`bytes.TrimFunc($s, unicode.IsSpace)`).Suggest(`bytes.TrimSpace($s)`)
m.Match(`bytes.Map(unicode.ToUpper, $s)`).Suggest(`bytes.ToUpper($s)`)
m.Match(`bytes.Map(unicode.ToLower, $s)`).Suggest(`bytes.ToLower($s)`)
Expand Down Expand Up @@ -225,15 +232,38 @@ func gocriticArgOrder(m fluent.Matcher) {
Suggest(`strings.Contains($s2, $s1)`)
}

func badSplitNValue(m fluent.Matcher) {
// Interpret any non-positive match count as "match all" and replace it by a better function.
// The special case is zero. Zero does nothing, so it's a bug, so we replace it too.
// Everything else is for consistency.
m.Match(`strings.Replace($s, $d, $w, $n)`).
Where(m["n"].Const && m["n"].Text.Matches(`(\-[0-9]+|0)`)).
Copy link
Owner

@quasilyte quasilyte Oct 30, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like if we had a way to const-fold expressions into int we could just check it like:

m["n"].ConstValue.Int() <= 0

It's not hard to implement, actually.
What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

- Where(m["n"].Const && m["n"].Text.Matches(`(\-[0-9]+|0)`))
+ Where(m["n"].Value.Int() <= 0)

Suggest(`strings.ReplaceAll($s, $d, $w)`)
m.Match(`bytes.Replace($s, $d, $w, $n)`).
Where(m["n"].Const && m["n"].Text.Matches(`(\-[0-9]+|0)`)).
Suggest(`bytes.ReplaceAll($s, $d, $w)`)
m.Match(`strings.SplitN($s, $p, $n)`).
Where(m["n"].Const && m["n"].Text.Matches(`(\-[0-9]+|0)`)).
Suggest(`strings.Split($s, $p)`)
m.Match(`bytes.SplitN($s, $p, $n)`).
Where(m["n"].Const && m["n"].Text.Matches(`(\-[0-9]+|0)`)).
Suggest(`bytes.Split($s, $p)`)
m.Match(`strings.SplitAfterN($s, $p, $n)`).
Where(m["n"].Const && m["n"].Text.Matches(`(\-[0-9]+|0)`)).
Suggest(`strings.SplitAfter($s, $p)`)
m.Match(`bytes.SplitAfterN($s, $p, $n)`).
Where(m["n"].Const && m["n"].Text.Matches(`(\-[0-9]+|0)`)).
Suggest(`bytes.SplitAfter($s, $p)`)

// The last argument of `SplitN` indicates parts count, not splits count
m.Match(`strings.SplitN($s, $p, 1)`).Suggest(`strings.SplitN($s, $p, 2)`)
m.Match(`bytes.SplitN($s, $p, 1)`).Suggest(`bytes.SplitN($s, $p, 2)`)
m.Match(`strings.SplitAfterN($s, $p, 1)`).Suggest(`strings.SplitAfterN($s, $p, 2)`)
m.Match(`bytes.SplitAfterN($s, $p, 1)`).Suggest(`bytes.SplitAfterN($s, $p, 2)`)
}

func gocriticBadCall(m fluent.Matcher) {
m.Match(`strings.Replace($_, $_, $_, 0)`,
`bytes.Replace($_, $_, $_, 0)`,
`strings.SplitN($_, $_, 0)`,
`bytes.SplitN($_, $_, 0)`).
Report(`n=0 argument does nothing, maybe n=-1 is indended?`)

m.Match(`append($_)`).
Report(`append called with 1 argument does nothing`)
m.Match(`append($_)`).Report(`append called with 1 argument does nothing`)
}

func gocriticDupArg(m fluent.Matcher) {
Expand Down