From cbe45ffc797a63287b888eeb88b33abbdb84888a Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 8 Jun 2018 16:06:29 +0200 Subject: [PATCH 1/6] Adds rule superfluous-else (an extension of indent-error-flow) (#13) * Adds rule superfluous-else (an extension of indent-error-flow) * Fix superfluous-else rule struct namming. * Adds superfuous-else rule to the rules table --- README.md | 1 + fixtures/superfluous-else.go | 73 +++++++++++++++++++++++++++++++ rule/superfluous-else.go | 81 +++++++++++++++++++++++++++++++++++ test/superfluous-else_test.go | 12 ++++++ 4 files changed, 167 insertions(+) create mode 100644 fixtures/superfluous-else.go create mode 100644 rule/superfluous-else.go create mode 100644 test/superfluous-else_test.go diff --git a/README.md b/README.md index 3a73c6f..a4d6514 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | `cyclomatic` | int | Sets restriction for maximum Cyclomatic complexity. | no | no | | `max-public-structs` | int | The maximum number of public structs in a file. | no | no | | `file-header` | string | Header which each file should have. | no | no | +| `superfluous-else` | n/a | Prevents redundant else statements (extends `indent-error-flow`) | no | no | ## Available Formatters diff --git a/fixtures/superfluous-else.go b/fixtures/superfluous-else.go new file mode 100644 index 0000000..436fa76 --- /dev/null +++ b/fixtures/superfluous-else.go @@ -0,0 +1,73 @@ +// Test of return+else warning. + +// Package pkg ... +package pkg + +import ( + "fmt" + "log" +) + +func h(f func() bool) string { + for { + if ok := f(); ok { + a := 1 + continue + } else { // MATCH /if block ends with a continue statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)/ + return "it's NOT okay!" + } + } +} + +func i(f func() bool) string { + for { + if f() { + a := 1 + continue + } else { // MATCH /if block ends with a continue statement, so drop this else and outdent its block/ + log.Printf("non-positive") + } + } + + return "ok" +} + +func j(f func() bool) string { + for { + if f() { + break + } else { // MATCH /if block ends with a break statement, so drop this else and outdent its block/ + log.Printf("non-positive") + } + } + + return "ok" +} + +func k() { + var a = 10 + /* do loop execution */ +LOOP: + for a < 20 { + if a == 15 { + a = a + 1 + goto LOOP + } else { // MATCH /if block ends with a goto statement, so drop this else and outdent its block/ + fmt.Printf("value of a: %d\n", a) + a++ + } + } +} + +func j(f func() bool) string { + for { + if f() { + a := 1 + fallthrough + } else { + log.Printf("non-positive") + } + } + + return "ok" +} diff --git a/rule/superfluous-else.go b/rule/superfluous-else.go new file mode 100644 index 0000000..a46ddd4 --- /dev/null +++ b/rule/superfluous-else.go @@ -0,0 +1,81 @@ +package rule + +import ( + "go/ast" + "go/token" + + "github.com/mgechev/revive/lint" +) + +// SuperfluousElseRule lints given else constructs. +type SuperfluousElseRule struct{} + +// Apply applies the rule to given file. +func (r *SuperfluousElseRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintSuperfluousElse{make(map[*ast.IfStmt]bool), onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *SuperfluousElseRule) Name() string { + return "superfluous-else" +} + +type lintSuperfluousElse struct { + ignore map[*ast.IfStmt]bool + onFailure func(lint.Failure) +} + +func (w lintSuperfluousElse) Visit(node ast.Node) ast.Visitor { + ifStmt, ok := node.(*ast.IfStmt) + if !ok || ifStmt.Else == nil { + return w + } + if w.ignore[ifStmt] { + return w + } + if elseif, ok := ifStmt.Else.(*ast.IfStmt); ok { + w.ignore[elseif] = true + return w + } + if _, ok := ifStmt.Else.(*ast.BlockStmt); !ok { + // only care about elses without conditions + return w + } + if len(ifStmt.Body.List) == 0 { + return w + } + shortDecl := false // does the if statement have a ":=" initialization statement? + if ifStmt.Init != nil { + if as, ok := ifStmt.Init.(*ast.AssignStmt); ok && as.Tok == token.DEFINE { + shortDecl = true + } + } + extra := "" + if shortDecl { + extra = " (move short variable declaration to its own line if necessary)" + } + + lastStmt := ifStmt.Body.List[len(ifStmt.Body.List)-1] + if stmt, ok := lastStmt.(*ast.BranchStmt); ok { + token := stmt.Tok.String() + if token != "fallthrough" { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: ifStmt.Else, + Category: "indent", + URL: "#indent-error-flow", + Failure: "if block ends with a " + token + " statement, so drop this else and outdent its block" + extra, + }) + } + } + + return w +} diff --git a/test/superfluous-else_test.go b/test/superfluous-else_test.go new file mode 100644 index 0000000..ab68b30 --- /dev/null +++ b/test/superfluous-else_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// TestSuperfluousElse rule. +func TestSuperfluousElse(t *testing.T) { + testRule(t, "superfluous-else", &rule.SuperfluousElseRule{}) +} From 05db4673dbd5e1b60d267bbc194778acfcbf640b Mon Sep 17 00:00:00 2001 From: mgechev Date: Fri, 8 Jun 2018 10:24:28 -0700 Subject: [PATCH 2/6] Add list of contributors --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index a4d6514..d569300 100644 --- a/README.md +++ b/README.md @@ -273,6 +273,12 @@ sys 0m17.192s Currently, type checking is enabled by default. If you want to run the linter without type checking, remove all typed rules from the configuration file. +## Contributors + +[mgechev](https://github.com/mgechev) |[paul-at-start](https://github.com/paul-at-start) |[chavacava](https://github.com/chavacava) | +:---: |:---: |:---: | +[mgechev](https://github.com/mgechev) |[paul-at-start](https://github.com/paul-at-start) |[chavacava](https://github.com/chavacava) | + ## License MIT From dc6767b811977c473d44d6b50264e6d1dba438db Mon Sep 17 00:00:00 2001 From: mgechev Date: Fri, 8 Jun 2018 10:28:17 -0700 Subject: [PATCH 3/6] Add toc --- README.md | 83 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index d569300..c316788 100644 --- a/README.md +++ b/README.md @@ -12,28 +12,57 @@ Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in r Here's how `revive` is different from `golint`: -* Allows us to enable or disable rules using a configuration file. -* Allows us to configure the linting rules with a TOML file. -* 2x faster running the same rules as golint. -* Provides functionality for disabling a specific rule or the entire linter for a file or a range of lines. - * `golint` allows this only for generated files. -* Optional type checking. Most rules in golint do not require type checking. If you disable them in the config file, revive will run over 6x faster than golint. -* Provides multiple formatters which let us customize the output. -* Allows us to customize the return code for the entire linter or based on the failure of only some rules. -* _Everyone can extend it easily with custom rules or formatters._ -* `Revive` provides more rules compared to `golint`. +- Allows us to enable or disable rules using a configuration file. +- Allows us to configure the linting rules with a TOML file. +- 2x faster running the same rules as golint. +- Provides functionality for disabling a specific rule or the entire linter for a file or a range of lines. + - `golint` allows this only for generated files. +- Optional type checking. Most rules in golint do not require type checking. If you disable them in the config file, revive will run over 6x faster than golint. +- Provides multiple formatters which let us customize the output. +- Allows us to customize the return code for the entire linter or based on the failure of only some rules. +- _Everyone can extend it easily with custom rules or formatters._ +- `Revive` provides more rules compared to `golint`.

+ + +- [revive](#revive) + - [Usage](#usage) + - [Text Editors](#text-editors) + - [Installation](#installation) + - [Command Line Flags](#command-line-flags) + - [Sample Invocations](#sample-invocations) + - [Comment Annotations](#comment-annotations) + - [Configuration](#configuration) + - [Default Configuration](#default-configuration) + - [Recommended Configuration](#recommended-configuration) + - [Available Rules](#available-rules) + - [Available Formatters](#available-formatters) + - [Friendly](#friendly) + - [Stylish](#stylish) + - [Default](#default) + - [Extensibility](#extensibility) + - [Custom Rule](#custom-rule) + - [Example](#example) + - [Custom Formatter](#custom-formatter) + - [Speed Comparison](#speed-comparison) + - [golint](#golint) + - [revive](#revive-1) + - [Contributors](#contributors) + - [License](#license) + + + ## Usage Since the default behavior of `revive` is compatible with `golint`, without providing any additional flags, the only difference you'd notice is faster execution. ### Text Editors -* Support for VSCode in [vscode-go](https://github.com/Microsoft/vscode-go/pull/1699). +- Support for VSCode in [vscode-go](https://github.com/Microsoft/vscode-go/pull/1699). ### Installation @@ -45,13 +74,13 @@ go get -u github.com/mgechev/revive `revive` accepts three command line parameters: -* `-config [PATH]` - path to config file in TOML format. -* `-exclude [PATTERN]` - pattern for files/directories/packages to be excluded for linting. You can specify the files you want to exclude for linting either as package name (i.e. `github.com/mgechev/revive`), list them as individual files (i.e. `file.go`), directories (i.e. `./foo/...`), or any combination of the three. -* `-formatter [NAME]` - formatter to be used for the output. The currently available formatters are: - * `default` - will output the failures the same way that `golint` does. - * `json` - outputs the failures in JSON format. - * `friendly` - outputs the failures when found. Shows summary of all the failures. - * `stylish` - formats the failures in a table. Keep in mind that it doesn't stream the output so it might be perceived as slower compared to others. +- `-config [PATH]` - path to config file in TOML format. +- `-exclude [PATTERN]` - pattern for files/directories/packages to be excluded for linting. You can specify the files you want to exclude for linting either as package name (i.e. `github.com/mgechev/revive`), list them as individual files (i.e. `file.go`), directories (i.e. `./foo/...`), or any combination of the three. +- `-formatter [NAME]` - formatter to be used for the output. The currently available formatters are: + - `default` - will output the failures the same way that `golint` does. + - `json` - outputs the failures in JSON format. + - `friendly` - outputs the failures when found. Shows summary of all the failures. + - `stylish` - formats the failures in a table. Keep in mind that it doesn't stream the output so it might be perceived as slower compared to others. ### Sample Invocations @@ -59,10 +88,10 @@ go get -u github.com/mgechev/revive revive -config revive.toml -exclude file1.go -exclude file2.go -formatter friendly github.com/mgechev/revive package/... ``` -* The command above will use the configuration from `revive.toml` -* `revive` will ignore `file1.go` and `file2.go` -* The output will be formatted with the `friendly` formatter -* The linter will analyze `github.com/mgechev/revive` and the files in `package` +- The command above will use the configuration from `revive.toml` +- `revive` will ignore `file1.go` and `file2.go` +- The output will be formatted with the `friendly` formatter +- The linter will analyze `github.com/mgechev/revive` and the files in `package` ### Comment Annotations @@ -215,8 +244,8 @@ Let's suppose we have developed a rule called `BanStructNameRule` which disallow With the snippet above we: -* Enable the rule with name `ban-struct-name`. The `Name()` method of our rule should return a string which matches `ban-struct-name`. -* Configure the rule with the argument `Foo`. The list of arguments will be passed to `Apply(*File, Arguments)` together with the target file we're linting currently. +- Enable the rule with name `ban-struct-name`. The `Name()` method of our rule should return a string which matches `ban-struct-name`. +- Configure the rule with the argument `Foo`. The list of arguments will be passed to `Apply(*File, Arguments)` together with the target file we're linting currently. A sample rule implementation can be found [here](/rule/argument-limit.go). @@ -275,9 +304,9 @@ Currently, type checking is enabled by default. If you want to run the linter wi ## Contributors -[mgechev](https://github.com/mgechev) |[paul-at-start](https://github.com/paul-at-start) |[chavacava](https://github.com/chavacava) | -:---: |:---: |:---: | -[mgechev](https://github.com/mgechev) |[paul-at-start](https://github.com/paul-at-start) |[chavacava](https://github.com/chavacava) | +| [mgechev](https://github.com/mgechev) | [paul-at-start](https://github.com/paul-at-start) | [chavacava](https://github.com/chavacava) | +| :---------------------------------------------------------------------------------------------------------------------------: | :----------------------------------------------------------------------------------------------------------------------------------------: | :---------------------------------------------------------------------------------------------------------------------------------: | +| [mgechev](https://github.com/mgechev) | [paul-at-start](https://github.com/paul-at-start) | [chavacava](https://github.com/chavacava) | ## License From 1fa5046357720e4c3f87771d0f695796cf9f52c4 Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 8 Jun 2018 21:41:49 +0200 Subject: [PATCH 4/6] Adds new rule empty-block (#14) * Adds rule superfluous-else (an extension of indent-error-flow) * initial (non functional) version * empty-block working version * adds tests for empty-block rule * Adds empty-block to the rules table * code clean-up --- README.md | 1 + config.go | 1 + fixtures/empty-block.go | 33 +++++++++++++++++++++++++ rule/empty-block.go | 52 ++++++++++++++++++++++++++++++++++++++++ test/empty-block_test.go | 12 ++++++++++ untyped.toml | 1 + 6 files changed, 100 insertions(+) create mode 100644 fixtures/empty-block.go create mode 100644 rule/empty-block.go create mode 100644 test/empty-block_test.go diff --git a/README.md b/README.md index c316788..561d01f 100644 --- a/README.md +++ b/README.md @@ -196,6 +196,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | `cyclomatic` | int | Sets restriction for maximum Cyclomatic complexity. | no | no | | `max-public-structs` | int | The maximum number of public structs in a file. | no | no | | `file-header` | string | Header which each file should have. | no | no | +| `empty-block` | n/a | Warns on empty code blocks | no | no | | `superfluous-else` | n/a | Prevents redundant else statements (extends `indent-error-flow`) | no | no | ## Available Formatters diff --git a/config.go b/config.go index 52b629c..351a55a 100644 --- a/config.go +++ b/config.go @@ -47,6 +47,7 @@ var allRules = append([]lint.Rule{ &rule.ArgumentsLimitRule{}, &rule.CyclomaticRule{}, &rule.FileHeaderRule{}, + &rule.EmptyBlockRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/empty-block.go b/fixtures/empty-block.go new file mode 100644 index 0000000..79b9dde --- /dev/null +++ b/fixtures/empty-block.go @@ -0,0 +1,33 @@ +// Test of empty-blocks. + +package fixtures + +func f(x int) bool { // MATCH /this block is empty, you can remove it/ + +} + +func g(f func() bool) string { + { // MATCH /this block is empty, you can remove it/ + } + + if ok := f(); ok { // MATCH /this block is empty, you can remove it/ + // only a comment + } else { + println("it's NOT empty!") + } + + if ok := f(); ok { + println("it's NOT empty!") + } else { // MATCH /this block is empty, you can remove it/ + + } + + for i := 0; i < 10; i++ { // MATCH /this block is empty, you can remove it/ + + } + + for { // MATCH /this block is empty, you can remove it/ + + } + +} diff --git a/rule/empty-block.go b/rule/empty-block.go new file mode 100644 index 0000000..11e4af3 --- /dev/null +++ b/rule/empty-block.go @@ -0,0 +1,52 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// EmptyBlockRule lints given else constructs. +type EmptyBlockRule struct{} + +// Apply applies the rule to given file. +func (r *EmptyBlockRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintEmptyBlock{make(map[*ast.IfStmt]bool), onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *EmptyBlockRule) Name() string { + return "empty-block" +} + +type lintEmptyBlock struct { + ignore map[*ast.IfStmt]bool + onFailure func(lint.Failure) +} + +func (w lintEmptyBlock) Visit(node ast.Node) ast.Visitor { + block, ok := node.(*ast.BlockStmt) + if !ok { + return w + } + + if len(block.List) == 0 { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: block, + Category: "logic", + URL: "#empty-block", + Failure: "this block is empty, you can remove it", + }) + } + + return w +} diff --git a/test/empty-block_test.go b/test/empty-block_test.go new file mode 100644 index 0000000..1de0680 --- /dev/null +++ b/test/empty-block_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// TestEmptyBlock rule. +func TestEmptyBlock(t *testing.T) { + testRule(t, "empty-block", &rule.EmptyBlockRule{}) +} diff --git a/untyped.toml b/untyped.toml index 67307bb..5c7ff44 100644 --- a/untyped.toml +++ b/untyped.toml @@ -12,3 +12,4 @@ [rule.range] [rule.receiver-naming] [rule.indent-error-flow] +[rule.empty-block] From 1b84ff83dd3d93d5ef09344430533c94beb6cdab Mon Sep 17 00:00:00 2001 From: mgechev Date: Fri, 8 Jun 2018 14:22:21 -0700 Subject: [PATCH 5/6] Add missing rule in config --- config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/config.go b/config.go index 351a55a..31a0cdc 100644 --- a/config.go +++ b/config.go @@ -48,6 +48,7 @@ var allRules = append([]lint.Rule{ &rule.CyclomaticRule{}, &rule.FileHeaderRule{}, &rule.EmptyBlockRule{}, + &rule.SuperfluousElseRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ From 8198433610bb0d517116ec4ce90d2119306216ac Mon Sep 17 00:00:00 2001 From: mgechev Date: Fri, 8 Jun 2018 16:04:26 -0700 Subject: [PATCH 6/6] Update readme with vim support --- README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/README.md b/README.md index 561d01f..80dfd38 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,18 @@ Since the default behavior of `revive` is compatible with `golint`, without prov ### Text Editors - Support for VSCode in [vscode-go](https://github.com/Microsoft/vscode-go/pull/1699). +- Support for vim via [w0rp/ale](https://github.com/w0rp/ale): + +```vim +call ale#linter#Define('go', { +\ 'name': 'revive', +\ 'output_stream': 'both', +\ 'executable': 'revive', +\ 'read_buffer': 0, +\ 'command': 'revive %t', +\ 'callback': 'ale#handlers#unix#HandleAsWarning', +\}) +``` ### Installation