diff --git a/.gitignore b/.gitignore index 1988a5c..9b8cdca 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ golinter revive vendor - +*.swp diff --git a/.travis.yml b/.travis.yml index 625ef4a..dfcb4e3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,17 @@ language: go - -go: "1.10.x" - +go: 1.11.x install: make install -script: make build && make test.all +script: +- make build +- make test +- git clone https://github.com/mgechev/revive.run revive-docs && cd revive-docs +- npm i && npm run build || true +- git config --global user.email "mgechev@gmail.com" +- git config --global user.name "mgechev" +- git add . +- git commit -am 'Travis CI deployment' +- git push --force --quiet "https://${GH_TOKEN}@${GH_REF}" +env: + global: + - GH_REF: github.com/mgechev/revive.run.git + - secure: Bp2/fJOVEor5aj3rNwA3+4/wecCmX2mVQ2pQt1AJ1n+pT6MjKYywswTDT6kzK/Cu1bPcfEGh3a7XKieAhIWVKvchoyYVV8J80F76HGyqgSBuzFXvV0c32zFn2LtxQxvmtCNynjmGAV57dHtsxGmHxkX9u8JIJ4J06E2Eq9nuuflTCf5o5gHtaE7P7hQT2WL/JRJVABHUMa0XzsMuUdRNO0OBXGMm+SqiWEcZetf2Vq3tfo2LL4ula99oTKKspg0iRKiauCZZaRxyZG/V3QiR0Rl9nhTVnb6hx6/RFrxru63Pm1FUaK1gIqEq9EUMpZRTddGW77UPp9GSB81/GaUm+e0GNFjUAL2e59t72wMxCQEOT+835hVbeCjgdksg0IDbn7sR/S+rYbiCyxTuCA/4YNlDoEajl9RMxK4culsq35LnibE1x7L4Q/5blD7HwVSMhA33HSDCC5MINwTdWwsdHYiAvFo0RCi5B0GngMzE6/pJxvYhWV3YhKWrgSmhafV1QO3Qu9dCn6P+7KsEVDbUeA8Yxnugd60kQNh2vG7bdTYKaZ6GhfU12zAM15xd2SSrKl6szSAo64CYOTznNFMBMskpm05SubTW1w+xDQy8vGjIHqb6zntiqUhFhTDd326iRYfrQuhAK53XbU1NUFFOyZa8kCTlSsPWDoSOX68oH9Q= diff --git a/DEVELOPING.md b/DEVELOPING.md index 75f3752..a886033 100644 --- a/DEVELOPING.md +++ b/DEVELOPING.md @@ -11,7 +11,7 @@ go get -u github.com/mgechev/revive cd $GOPATH/src/github.com/mgechev/revive ``` -After that install the dependencies using dep: +After that install the dependencies using go modules: ```bash make install diff --git a/Gopkg.lock b/Gopkg.lock deleted file mode 100644 index 133263b..0000000 --- a/Gopkg.lock +++ /dev/null @@ -1,73 +0,0 @@ -# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. - - -[[projects]] - name = "github.com/BurntSushi/toml" - packages = ["."] - revision = "b26d9c308763d68093482582cea63d69be07a0f0" - version = "v0.3.0" - -[[projects]] - name = "github.com/fatih/color" - packages = ["."] - revision = "5b77d2a35fb0ede96d138fc9a99f5c9b6aef11b4" - version = "v1.7.0" - -[[projects]] - name = "github.com/mattn/go-colorable" - packages = ["."] - revision = "167de6bfdfba052fa6b2d3664c8f5272e23c9072" - version = "v0.0.9" - -[[projects]] - name = "github.com/mattn/go-isatty" - packages = ["."] - revision = "0360b2af4f38e8d38c7fce2a9f4e702702d73a39" - version = "v0.0.3" - -[[projects]] - name = "github.com/mattn/go-runewidth" - packages = ["."] - revision = "9e777a8366cce605130a531d2cd6363d07ad7317" - version = "v0.0.2" - -[[projects]] - branch = "master" - name = "github.com/mgechev/dots" - packages = ["."] - revision = "3d1c0cc50642eae6291b43fdd37c7d7acfff7975" - -[[projects]] - branch = "master" - name = "github.com/olekukonko/tablewriter" - packages = ["."] - revision = "d4647c9c7a84d847478d890b816b7d8b62b0b279" - -[[projects]] - name = "github.com/pkg/errors" - packages = ["."] - revision = "645ef00459ed84a119197bfb8d8205042c6df63d" - version = "v0.8.0" - -[[projects]] - branch = "master" - name = "golang.org/x/sys" - packages = ["unix"] - revision = "c11f84a56e43e20a78cee75a7c034031ecf57d1f" - -[[projects]] - branch = "master" - name = "golang.org/x/tools" - packages = [ - "go/gcexportdata", - "go/internal/gcimporter", - "go/types/typeutil" - ] - revision = "a5b4c53f6e8bdcafa95a94671bf2d1203365858b" - -[solve-meta] - analyzer-name = "dep" - analyzer-version = 1 - inputs-digest = "55ca94323f5133e6922ce74ad093146191929c198c0aa732c84e7ee6070af701" - solver-name = "gps-cdcl" - solver-version = 1 diff --git a/Gopkg.toml b/Gopkg.toml deleted file mode 100644 index e28d988..0000000 --- a/Gopkg.toml +++ /dev/null @@ -1,41 +0,0 @@ -# Gopkg.toml example -# -# Refer to https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md -# for detailed Gopkg.toml documentation. -# -# required = ["github.com/user/thing/cmd/thing"] -# ignored = ["github.com/user/project/pkgX", "bitbucket.org/user/project/pkgA/pkgY"] -# -# [[constraint]] -# name = "github.com/user/project" -# version = "1.0.0" -# -# [[constraint]] -# name = "github.com/user/project2" -# branch = "dev" -# source = "github.com/myfork/project2" -# -# [[override]] -# name = "github.com/x/y" -# version = "2.4.0" - - -[[constraint]] - name = "github.com/BurntSushi/toml" - version = "0.3.0" - -[[constraint]] - name = "github.com/fatih/color" - version = "1.5.0" - -[[constraint]] - branch = "master" - name = "github.com/olekukonko/tablewriter" - -[[constraint]] - name = "github.com/pkg/errors" - version = "0.8.0" - -[[constraint]] - branch = "master" - name = "golang.org/x/tools" diff --git a/Makefile b/Makefile index 0fafbe7..277bd3c 100644 --- a/Makefile +++ b/Makefile @@ -1,12 +1,13 @@ -deps.devtools: - @go get github.com/golang/dep/cmd/dep +.PHONY: test -install: deps.devtools - @dep ensure -v +export GO111MODULE=on + +install: + @go mod vendor build: @go build -test.all: +test: @go test -v ./test/... diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..2222b5b --- /dev/null +++ b/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,12 @@ + + + + + + + + + + + + diff --git a/README.md b/README.md index 9ad6e7b..23aea38 100644 --- a/README.md +++ b/README.md @@ -30,29 +30,32 @@ Here's how `revive` is different from `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](#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) + - [Custom Configuration](#custom-configuration) + - [Recommended Configuration](#recommended-configuration) + - [Available Rules](#available-rules) + - [Configurable rules](#configurable-rules) + - [`var-naming`](#var-naming) + - [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) + - [Contributors](#contributors) + - [License](#license) @@ -63,6 +66,7 @@ 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 Atom via [linter-revive](https://github.com/morphy2k/linter-revive). - Support for vim via [w0rp/ale](https://github.com/w0rp/ale): ```vim @@ -89,10 +93,13 @@ go get -u github.com/mgechev/revive - `-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. + - `ndjson` - outputs the failures as stream in newline delimited JSON (NDJSON) 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. + - `checkstyle` - outputs the failures in XML format compatible with that of Java's [Checkstyle](https://checkstyle.org/). ### Sample Invocations @@ -118,6 +125,8 @@ func Public() {} The snippet above, will disable `revive` between the `revive:disable` and `revive:enable` comments. If you skip `revive:enable`, the linter will be disabled for the rest of the file. +With `revive:disable-next-line` and `revive:disable-line` you can disable `revive` on a particular code line. + You can do the same on a rule level. In case you want to disable only a particular rule, you can use: ```go @@ -171,7 +180,7 @@ revive -config defaults.toml github.com/mgechev/revive This will use the configuration file `defaults.toml`, the `default` formatter, and will run linting over the `github.com/mgechev/revive` package. -### Recommended Configuration +### Custom Configuration ```shell revive -config config.toml -formatter friendly github.com/mgechev/revive @@ -179,41 +188,111 @@ revive -config config.toml -formatter friendly github.com/mgechev/revive This will use `config.toml`, the `friendly` formatter, and will run linting over the `github.com/mgechev/revive` package. +### Recommended Configuration + +The following snippet contains the recommended `revive` configuration that you can use in your project: + +```toml +ignoreGeneratedHeader = false +severity = "warning" +confidence = 0.8 +errorCode = 0 +warningCode = 0 + +[rule.blank-imports] +[rule.context-as-argument] +[rule.context-keys-type] +[rule.dot-imports] +[rule.error-return] +[rule.error-strings] +[rule.error-naming] +[rule.exported] +[rule.if-return] +[rule.increment-decrement] +[rule.var-naming] +[rule.var-declaration] +[rule.package-comments] +[rule.range] +[rule.receiver-naming] +[rule.time-naming] +[rule.unexported-return] +[rule.indent-error-flow] +[rule.errorf] +[rule.empty-block] +[rule.superfluous-else] +[rule.unused-parameter] +[rule.unreachable-code] +[rule.redefines-builtin-id] +``` + ## Available Rules List of all available rules. The rules ported from `golint` are left unchanged and indicated in the `golint` column. | Name | Config | Description | `golint` | Typed | | --------------------- | :----: | :--------------------------------------------------------------- | :------: | :---: | -| `context-keys-type` | n/a | Disallows the usage of basic types in `context.WithValue`. | yes | yes | -| `time-naming` | n/a | Conventions around the naming of time variables. | yes | yes | -| `var-declaration` | n/a | Reduces redundancies around variable declaration. | yes | yes | -| `unexported-return` | n/a | Warns when a public return is from unexported type. | yes | yes | -| `errorf` | n/a | Should replace `error.New(fmt.Sprintf())` with `error.Errorf()` | yes | yes | -| `blank-imports` | n/a | Disallows blank imports | yes | no | -| `context-as-argument` | n/a | `context.Context` should be the first argument of a function. | yes | no | -| `dot-imports` | n/a | Forbids `.` imports. | yes | no | -| `error-return` | n/a | The error return parameter should be last. | yes | no | -| `error-strings` | n/a | Conventions around error strings. | yes | no | -| `error-naming` | n/a | Naming of error variables. | yes | no | -| `exported` | n/a | Naming and commenting conventions on exported symbols. | yes | no | -| `if-return` | n/a | Redundant if when returning an error. | yes | no | -| `increment-decrement` | n/a | Use `i++` and `i--` instead of `i += 1` and `i -= 1`. | yes | no | -| `var-naming` | n/a | Naming rules. | yes | no | -| `package-comments` | n/a | Package commenting conventions. | yes | no | -| `range` | n/a | Prevents redundant variables when iterating over a collection. | yes | no | -| `receiver-naming` | n/a | Conventions around the naming of receivers. | yes | no | -| `indent-error-flow` | n/a | Prevents redundant else statements. | yes | no | -| `argument-limit` | int | Specifies the maximum number of arguments a function can receive | no | no | -| `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 | -| `confusing-naming` | n/a | Warns on methods with names that differ only by capitalization | no | no | -| `get-return ` | n/a | Warns on getters that do not yield any result | no | no | -| `modifies-param` | n/a | Warns on assignments to function parameters | no | no | -| `deep-exit` | n/a | Looks for program exits in funcs other than `main()` or `init()` | no | no | +| [`context-keys-type`](./RULES_DESCRIPTIONS.md#context-key-types) | n/a | Disallows the usage of basic types in `context.WithValue`. | yes | yes | +| [`time-naming`](./RULES_DESCRIPTIONS.md#time-naming) | n/a | Conventions around the naming of time variables. | yes | yes | +| [`var-declaration`](./RULES_DESCRIPTIONS.md#var-declaration) | n/a | Reduces redundancies around variable declaration. | yes | yes | +| [`unexported-return`](./RULES_DESCRIPTIONS.md#unexported-return) | n/a | Warns when a public return is from unexported type. | yes | yes | +| [`errorf`](./RULES_DESCRIPTIONS.md#errorf) | n/a | Should replace `errors.New(fmt.Sprintf())` with `fmt.Errorf()` | yes | yes | +| [`blank-imports`](./RULES_DESCRIPTIONS.md#blank-imports) | n/a | Disallows blank imports | yes | no | +| [`context-as-argument`](./RULES_DESCRIPTIONS.md#context-as-argument) | n/a | `context.Context` should be the first argument of a function. | yes | no | +| [`dot-imports`](./RULES_DESCRIPTIONS.md#dot-imports) | n/a | Forbids `.` imports. | yes | no | +| [`error-return`](./RULES_DESCRIPTIONS.md#error-return) | n/a | The error return parameter should be last. | yes | no | +| [`error-strings`](./RULES_DESCRIPTIONS.md#error-strings) | n/a | Conventions around error strings. | yes | no | +| [`error-naming`](./RULES_DESCRIPTIONS.md#error-naming) | n/a | Naming of error variables. | yes | no | +| [`exported`](./RULES_DESCRIPTIONS.md#exported) | n/a | Naming and commenting conventions on exported symbols. | yes | no | +| [`if-return`](./RULES_DESCRIPTIONS.md#if-return) | n/a | Redundant if when returning an error. | yes | no | +| [`increment-decrement`](./RULES_DESCRIPTIONS.md#increment-decrement) | n/a | Use `i++` and `i--` instead of `i += 1` and `i -= 1`. | yes | no | +| [`var-naming`](./RULES_DESCRIPTIONS.md#var-naming) | whitelist & blacklist of initialisms | Naming rules. | yes | no | +| [`package-comments`](./RULES_DESCRIPTIONS.md#package-comments) | n/a | Package commenting conventions. | yes | no | +| [`range`](./RULES_DESCRIPTIONS.md#range) | n/a | Prevents redundant variables when iterating over a collection. | yes | no | +| [`receiver-naming`](./RULES_DESCRIPTIONS.md#receiver-naming) | n/a | Conventions around the naming of receivers. | yes | no | +| [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow) | n/a | Prevents redundant else statements. | yes | no | +| [`argument-limit`](./RULES_DESCRIPTIONS.md#argument-limit) | int | Specifies the maximum number of arguments a function can receive | no | no | +| [`cyclomatic`](./RULES_DESCRIPTIONS.md#cyclomatic) | int | Sets restriction for maximum Cyclomatic complexity. | no | no | +| [`max-public-structs`](./RULES_DESCRIPTIONS.md#max-public-structs) | int | The maximum number of public structs in a file. | no | no | +| [`file-header`](./RULES_DESCRIPTIONS.md#file-header) | string | Header which each file should have. | no | no | +| [`empty-block`](./RULES_DESCRIPTIONS.md#empty-block) | n/a | Warns on empty code blocks | no | no | +| [`superfluous-else`](./RULES_DESCRIPTIONS.md#superfluous-else) | n/a | Prevents redundant else statements (extends [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)) | no | no | +| [`confusing-naming`](./RULES_DESCRIPTIONS.md#confusing-naming) | n/a | Warns on methods with names that differ only by capitalization | no | no | +| [`get-return`](./RULES_DESCRIPTIONS.md#get-return) | n/a | Warns on getters that do not yield any result | no | no | +| [`modifies-parameter`](./RULES_DESCRIPTIONS.md#modifies-parameter) | n/a | Warns on assignments to function parameters | no | no | +| [`confusing-results`](./RULES_DESCRIPTIONS.md#confusing-results) | n/a | Suggests to name potentially confusing function results | no | no | +| [`deep-exit`](./RULES_DESCRIPTIONS.md#deep-exit) | n/a | Looks for program exits in funcs other than `main()` or `init()` | no | no | +| [`unused-parameter`](./RULES_DESCRIPTIONS.md#unused-parameter) | n/a | Suggests to rename or remove unused function parameters | no | no | +| [`unreachable-code`](./RULES_DESCRIPTIONS.md#unreachable-code) | n/a | Warns on unreachable code | no | no | +| [`add-constant`](./RULES_DESCRIPTIONS.md#add-constant) | map | Suggests using constant for magic numbers and string literals | no | no | +| [`flag-parameter`](./RULES_DESCRIPTIONS.md#flag-parameter) | n/a | Warns on boolean parameters that create a control coupling | no | no | +| [`unnecessary-stmt`](./RULES_DESCRIPTIONS.md#unnecessary-stmt) | n/a | Suggests removing or simplifying unnecessary statements | no | no | +| [`struct-tag`](./RULES_DESCRIPTIONS.md#struct-tag) | n/a | Checks common struct tags like `json`,`xml`,`yaml` | no | no | +| [`modifies-value-receiver`](./RULES_DESCRIPTIONS.md#modifies-value-receiver) | n/a | Warns on assignments to value-passed method receivers | no | yes | +| [`constant-logical-expr`](./RULES_DESCRIPTIONS.md#constant-logical-expr) | n/a | Warns on constant logical expressions | no | no | +| [`bool-literal-in-expr`](./RULES_DESCRIPTIONS.md#bool-literal-in-expr)| n/a | Suggests removing Boolean literals from logic expressions | no | no | +| [`redefines-builtin-id`](./RULES_DESCRIPTIONS.md#redefines-builtin-id)| n/a | Warns on redefinitions of builtin identifiers | no | no | +| [`function-result-limit`](./RULES_DESCRIPTIONS.md#function-result-limit) | int | Specifies the maximum number of results a function can return | no | no | +| [`imports-blacklist`](./RULES_DESCRIPTIONS.md#imports-blacklist) | []string | Disallows importing the specified packages | no | no | +| [`range-val-in-closure`](./RULES_DESCRIPTIONS.md#range-val-in-closure)| n/a | Warns if range value is used in a closure dispatched as goroutine| no | no | +| [`waitgroup-by-value`](./RULES_DESCRIPTIONS.md#waitgroup-by-value) | n/a | Warns on functions taking sync.WaitGroup as a by-value parameter | no | no | +| [`atomic`](./RULES_DESCRIPTIONS.md#atomic) | n/a | Check for common mistaken usages of the `sync/atomic` package | no | no | +| [`empty-lines`](./RULES_DESCRIPTIONS.md#empty-lines) | n/a | Warns when there are heading or trailing newlines in a block | no | no | +| [`line-lenght-limit`](./RULES_DESCRIPTIONS.md#line-lenght-limit) | int | Specifies the maximum number of characters in a line | no | no | + +## Configurable rules + +Here you can find how you can configure some of the existing rules: + +### `var-naming` + +This rule accepts two slices of strings, a whitelist and a blacklist of initialisms. By default the rule behaves exactly as the alternative in `golint` but optionally, you can relax it (see [golint/lint/issues/89](https://github.com/golang/lint/issues/89)) + +```toml +[rule.var-naming] + arguments = [["ID"], ["VM"]] +``` + +This way, revive will not warn for identifier called `customId` but will warn that `customVm` should be called `customVM`. ## Available Formatters @@ -221,15 +300,29 @@ This section lists all the available formatters and provides a screenshot for ea ### Friendly -![Friendly formatter](/assets/friendly-formatter.png) +![Friendly formatter](/assets/formatter-friendly.png) ### Stylish -![Stylish formatter](/assets/stylish-formatter.png) +![Stylish formatter](/assets/formatter-stylish.png) ### Default -![Default formatter](/assets/default-formatter.png) +The default formatter produces the same output as `golint`. + +![Default formatter](/assets/formatter-default.png) + +### Plain + +The plain formatter produces the same output as the default formatter and appends URL to the rule description. + +![Plain formatter](/assets/formatter-plain.png) + +### Unix + +The unix formatter produces the same output as the default formatter but surrounds the rules in `[]`. + +![Unix formatter](/assets/formatter-unix.png) ## Extensibility @@ -321,9 +414,13 @@ Currently, type checking is enabled by default. If you want to run the linter wi ## Contributors -[mgechev](https://github.com/mgechev) |[chavacava](https://github.com/chavacava) |[tamird](https://github.com/tamird) |[paul-at-start](https://github.com/paul-at-start) |[vkrol](https://github.com/vkrol) | -:---: |:---: |:---: |:---: |:---: | -[mgechev](https://github.com/mgechev) |[chavacava](https://github.com/chavacava) |[tamird](https://github.com/tamird) |[paul-at-start](https://github.com/paul-at-start) |[vkrol](https://github.com/vkrol) | +[mgechev](https://github.com/mgechev) |[chavacava](https://github.com/chavacava) |[xuri](https://github.com/xuri) |[gsamokovarov](https://github.com/gsamokovarov) |[morphy2k](https://github.com/morphy2k) |[z0mbie42](https://github.com/z0mbie42) | +:---: |:---: |:---: |:---: |:---: |:---: | +[mgechev](https://github.com/mgechev) |[chavacava](https://github.com/chavacava) |[xuri](https://github.com/xuri) |[gsamokovarov](https://github.com/gsamokovarov) |[morphy2k](https://github.com/morphy2k) |[z0mbie42](https://github.com/z0mbie42) | + +[tamird](https://github.com/tamird) |[paul-at-start](https://github.com/paul-at-start) |[psapezhko](https://github.com/psapezhko) |[Jarema](https://github.com/Jarema) |[vkrol](https://github.com/vkrol) |[haya14busa](https://github.com/haya14busa) | +:---: |:---: |:---: |:---: |:---: |:---: | +[tamird](https://github.com/tamird) |[paul-at-start](https://github.com/paul-at-start) |[psapezhko](https://github.com/psapezhko) |[Jarema](https://github.com/Jarema) |[vkrol](https://github.com/vkrol) |[haya14busa](https://github.com/haya14busa) | ## License diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md new file mode 100644 index 0000000..0522c62 --- /dev/null +++ b/RULES_DESCRIPTIONS.md @@ -0,0 +1,428 @@ +# Description of available rules + +List of all available rules. + + +- [Description of available rules](#description-of-available-rules) + - [add-constant](#add-constant) + - [argument-limit](#argument-limit) + - [atomic](#atomic) + - [blank-imports](#blank-imports) + - [bool-literal-in-expr](#bool-literal-in-expr) + - [confusing-naming](#confusing-naming) + - [confusing-results](#confusing-results) + - [constant-logical-expr](#constant-logical-expr) + - [context-as-argument](#context-as-argument) + - [context-keys-type](#context-keys-type) + - [cyclomatic](#cyclomatic) + - [deep-exit](#deep-exit) + - [dot-imports](#dot-imports) + - [empty-block](#empty-block) + - [empty-lines](#empty-lines) + - [error-naming](#error-naming) + - [error-return](#error-return) + - [error-strings](#error-strings) + - [errorf](#errorf) + - [exported](#exported) + - [file-header](#file-header) + - [flag-parameter](#flag-parameter) + - [function-result-limit](#function-result-limit) + - [get-return](#get-return) + - [if-return](#if-return) + - [increment-decrement](#increment-decrement) + - [indent-error-flow](#indent-error-flow) + - [imports-blacklist](#imports-blacklist) + - [line-lenght-limit](#line-lenght-limit) + - [max-public-structs](#max-public-structs) + - [modifies-parameter](#modifies-parameter) + - [modifies-value-receiver](#modifies-value-receiver) + - [package-comments](#package-comments) + - [range](#range) + - [range-val-in-closure](#range-val-in-closure) + - [receiver-naming](#receiver-naming) + - [redefines-builtin-id](#redefines-builtin-id) + - [struct-tag](#struct-tag) + - [superfluous-else](#superfluous-else) + - [time-naming](#time-naming) + - [var-naming](#var-naming) + - [var-declaration](#var-declaration) + - [unexported-return](#unexported-return) + - [unnecessary-stmt](#unnecessary-stmt) + - [unreachable-code](#unreachable-code) + - [unused-parameter](#unused-parameter) + - [waitgroup-by-value](#waitgroup-by-value) + +## add-constant + +_Description_: Suggests using constant for [magic numbers](https://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants) and string literals. + +_Configuration_: + +* `maxLitCount` : (string) maximum number of instances of a string literal that are tolerated before warn. +* `allowStr`: (string) comma-separated list of allowed string literals +* `allowInts`: (string) comma-separated list of allowed integers +* `allowFloats`: (string) comma-separated list of allowed floats + +Example: + +```toml +[rule.add-constant] + arguments = [{maxLitCount = "3",allowStrs ="\"\"",allowInts="0,1,2",allowFloats="0.0,0.,1.0,1.,2.0,2."}] +``` + +## argument-limit + +_Description_: Warns when a function receives more parameters than the maximum set by the rule's configuration. +Enforcing a maximum number of parameters helps to keep the code readable and maintainable. + +_Configuration_: (int) the maximum number of parameters allowed per function. + +Example: + +```toml +[argument-limit] + arguments =[4] +``` + +## atomic + +_Description_: Check for commonly mistaken usages of the `sync/atomic` package + +_Configuration_: N/A + +## blank-imports + +_Description_: Blank import should be only in a main or test package, or have a comment justifying it. + +_Configuration_: N/A + +## bool-literal-in-expr + +_Description_: Using Boolean literals (`true`, `false`) in logic expressions may make the code less readable. This rule suggests removing Boolean literals from logic expressions. + +_Configuration_: N/A + +## confusing-naming + +_Description_: Methods or fields of `struct` that have names different only by capitalization could be confusing. + +_Configuration_: N/A + +## confusing-results + +_Description_: Function or methods that return multiple, no named, values of the same type could induce error. + +_Configuration_: N/A + +## constant-logical-expr + +_Description_: The rule spots logical expressions that evaluate always to the same value. + +_Configuration_: N/A + +## context-as-argument + +_Description_: By [convention](https://github.com/golang/go/wiki/CodeReviewComments#contexts), `context.Context` should be the first parameter of a function. This rule spots function declarations that do not follow the convention. + +_Configuration_: N/A + +## context-keys-type + +_Description_: Basic types should not be used as a key in `context.WithValue`. + +_Configuration_: N/A + +## cyclomatic + +_Description_: [Cyclomatic complexity](https://en.wikipedia.org/wiki/Cyclomatic_complexity) is a measure of code complexity. Enforcing a maximum complexity per function helps to keep code readable and maintainable. + +_Configuration_: (int) the maximum function complexity + +Example: + +```toml +[cyclomatic] + arguments =[3] +``` + +## deep-exit + +_Description_: Packages exposing functions that can stop program execution by exiting are hard to reuse. This rule looks for program exits in functions other than `main()` or `init()`. + +_Configuration_: N/A + +## dot-imports + +_Description_: Importing with `.` makes the programs much harder to understand because it is unclear whether names belong to the current package or to an imported package. + +More information [here](https://github.com/golang/go/wiki/CodeReviewComments#import-dot) + +_Configuration_: N/A + +## empty-block + +_Description_: Empty blocks make code less readable and could be a symptom of a bug or unfinished refactoring. + +_Configuration_: N/A + +## empty-lines + +_Description_: Sometimes `gofmt` is not enough to enforce a common formatting of a code-base; this rule warns when there are heading or trailing newlines in code blocks. + +_Configuration_: N/A + +## error-naming + +_Description_: By convention, for the sake of readability, variables of type `error` must be named with the prefix `err`. + +_Configuration_: N/A + +## error-return + +_Description_: By convention, for the sake of readability, the errors should be last in the list of returned values by a function. + +_Configuration_: N/A + +## error-strings + +_Description_: By convention, for better readability, error messages should not be capitalized or end with punctuation or a newline. + +More information [here](https://github.com/golang/go/wiki/CodeReviewComments#error-strings) + +_Configuration_: N/A + +## errorf + +_Description_: It is possible to get a simpler program by replacing `errors.New(fmt.Sprintf())` with `fmt.Errorf()`. This rule spots that kind of simplification opportunities. + +_Configuration_: N/A + +## exported + +_Description_: Exported function and methods should have comments. This warns on undocumented exported functions and methods. + +More information [here](https://github.com/golang/go/wiki/CodeReviewComments#doc-comments) + +_Configuration_: N/A + +## file-header + +_Description_: This rule helps to enforce a common header for all source files in a project by spotting those files that do not have the specified header. + +_Configuration_: (string) the header to look for in source files. + +Example: + +```toml +[file-header] + arguments =["This is the text that must appear at the top of source files."] +``` + +## flag-parameter + +_Description_: If a function controls the flow of another by passing it information on what to do, both functions are said to be [control-coupled](https://en.wikipedia.org/wiki/Coupling_(computer_programming)#Procedural_programming). +Coupling among functions must be minimized for better maintainability of the code. +This rule warns on boolean parameters that create a control coupling. + +_Configuration_: N/A + +## function-result-limit + +_Description_: Functions returning too many results can be hard to understand/use. + +_Configuration_: (int) the maximum allowed return values + +Example: + +```toml +[function-result-limit] + arguments =[3] +``` + +## get-return + +_Description_: Typically, functions with names prefixed with _Get_ are supposed to return a value. + +_Configuration_: N/A + +## if-return + +_Description_: Checking if an error is _nil_ to just after return the error or nil is redundant. + +_Configuration_: N/A + +## increment-decrement + +_Description_: By convention, for better readability, incrementing an integer variable by 1 is recommended to be done using the `++` operator. +This rule spots expressions like `i += 1` and `i -= 1` and proposes to change them into `i++` and `i--`. + +_Configuration_: N/A + +## indent-error-flow + +_Description_: To improve the readability of code, it is recommended to reduce the indentation as much as possible. +This rule highlights redundant _else-blocks_ that can be eliminated from the code. + +More information [here](https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow) + +_Configuration_: N/A + +## imports-blacklist + +_Description_: Warns when importing black-listed packages. + +_Configuration_: black-list of package names + +Example: + +```toml +[imports-blacklist] + arguments =["crypto/md5", "crypto/sha1"] +``` + +## line-lenght-limit + +_Description_: Warns in the presence of code lines longer than a configured maximum. + +_Configuration_: (int) maximum line length in characters. + +Example: + +```toml +[line-lenght-limit] + arguments =[80] +``` + +## max-public-structs + +_Description_: Packages declaring too many public structs can be hard to understand/use, +and could be a symptom of bad design. + +This rule warns on files declaring more than a configured, maximum number of public structs. + +_Configuration_: (int) the maximum allowed public structs + +Example: + +```toml +[max-public-structs] + arguments =[3] +``` + +## modifies-parameter + +_Description_: A function that modifies its parameters can be hard to understand. It can also be misleading if the arguments are passed by value by the caller. +This rule warns when a function modifies one or more of its parameters. + +_Configuration_: N/A + +## modifies-value-receiver + +_Description_: A method that modifies its receiver value can have undesired behavior. The modification can be also the root of a bug because the actual value receiver could be a copy of that used at the calling site. +This rule warns when a method modifies its receiver. + +_Configuration_: N/A + +## package-comments + +_Description_: Packages should have comments. This rule warns on undocumented packages and when packages comments are detached to the `package` keyword. + +More information [here](https://github.com/golang/go/wiki/CodeReviewComments#package-comments) + +_Configuration_: N/A + +## range + +_Description_: This rule suggests a shorter way of writing ranges that do not use the second value. + +_Configuration_: N/A + +## range-val-in-closure + +_Description_: Range variables in a loop are reused at each iteration; therefore a goroutine created in a loop will point to the range variable with from the upper scope. This way, the goroutine could use the variable with an undesired value. +This rule warns when a range value (or index) is used inside a closure + +_Configuration_: N/A + +## receiver-naming + +_Description_: By convention, receiver names in a method should reflect their identity. For example, if the receiver is of type `Parts`, `p` is an adequate name for it. Contrary to other languages, it is not idiomatic to name receivers as `this` or `self`. + +_Configuration_: N/A + +## redefines-builtin-id + +_Description_: Constant names like `false`, `true`, `nil`, function names like `append`, `make`, and basic type names like `bool`, and `byte` are not reserved words of the language; therefore the can be redefined. +Even if possible, redefining these built in names can lead to bugs very difficult to detect. + +_Configuration_: N/A + +## struct-tag + +_Description_: Struct tags are not checked at compile time. +This rule, checks and warns if it finds errors in common struct tags types like: asn1, default, json, protobuf, xml, yaml. + +_Configuration_: N/A + +## superfluous-else + +_Description_: To improve the readability of code, it is recommended to reduce the indentation as much as possible. +This rule highlights redundant _else-blocks_ that can be eliminated from the code. + +_Configuration_: N/A + +## time-naming + +_Description_: Using unit-specific suffix like "Secs", "Mins", ... when naming variables of type `time.Duration` can be misleading, this rule highlights those cases. + +_Configuration_: N/A + +## var-naming + +_Description_: This rule warns when [variable](https://github.com/golang/go/wiki/CodeReviewComments#variable-names) or [package](https://github.com/golang/go/wiki/CodeReviewComments#package-names) naming conventions are not followed. + +_Configuration_: This rule accepts two slices of strings, a whitelist and a blacklist of initialisms. By default, the rule behaves exactly as the alternative in `golint` but optionally, you can relax it (see [golint/lint/issues/89](https://github.com/golang/lint/issues/89)) + +Example: + +```toml +[rule.var-naming] + arguments = [["ID"], ["VM"]] +``` + +## var-declaration + +_Description_: This rule proposes simplifications of variable declarations. + +_Configuration_: N/A + +## unexported-return + +_Description_: This rule warns when an exported function or method returns a value of an un-exported type. + +_Configuration_: N/A + +## unnecessary-stmt + +_Description_: This rule suggests to remove redundant statements like a `break` at the end of a case block, for improving the code's readability. + +_Configuration_: N/A + +## unreachable-code + +_Description_: This rule spots and proposes to remove [unreachable code](https://en.wikipedia.org/wiki/Unreachable_code). + +_Configuration_: N/A + +## unused-parameter + +_Description_: This rule warns on unused parameters. Functions or methods with unused parameters can be a symptom of an unfinished refactoring or a bug. + +_Configuration_: N/A + +## waitgroup-by-value + +_Description_: Function parameters that are passed by value, are in fact a copy of the original argument. Passing a copy of a `sync.WaitGroup` is usually not what the developer wants to do. +This rule warns when a `sync.WaitGroup` expected as a by-value parameter in a function or method. + +_Configuration_: N/A diff --git a/assets/default-formatter.png b/assets/default-formatter.png deleted file mode 100644 index 5292959..0000000 Binary files a/assets/default-formatter.png and /dev/null differ diff --git a/assets/formatter-default.png b/assets/formatter-default.png new file mode 100644 index 0000000..f35852b Binary files /dev/null and b/assets/formatter-default.png differ diff --git a/assets/formatter-friendly.png b/assets/formatter-friendly.png new file mode 100644 index 0000000..f93d992 Binary files /dev/null and b/assets/formatter-friendly.png differ diff --git a/assets/formatter-plain.png b/assets/formatter-plain.png new file mode 100644 index 0000000..67e654e Binary files /dev/null and b/assets/formatter-plain.png differ diff --git a/assets/formatter-stylish.png b/assets/formatter-stylish.png new file mode 100644 index 0000000..d044c5b Binary files /dev/null and b/assets/formatter-stylish.png differ diff --git a/assets/formatter-unix.png b/assets/formatter-unix.png new file mode 100644 index 0000000..a68b479 Binary files /dev/null and b/assets/formatter-unix.png differ diff --git a/assets/friendly-formatter.png b/assets/friendly-formatter.png deleted file mode 100644 index 0838ea2..0000000 Binary files a/assets/friendly-formatter.png and /dev/null differ diff --git a/assets/logo.png b/assets/logo.png index 13d62ae..e7fbd94 100644 Binary files a/assets/logo.png and b/assets/logo.png differ diff --git a/assets/stylish-formatter.png b/assets/stylish-formatter.png deleted file mode 100644 index 7ceaa2e..0000000 Binary files a/assets/stylish-formatter.png and /dev/null differ diff --git a/config.go b/config.go index 760b650..89bede5 100644 --- a/config.go +++ b/config.go @@ -49,18 +49,45 @@ var allRules = append([]lint.Rule{ &rule.FileHeaderRule{}, &rule.EmptyBlockRule{}, &rule.SuperfluousElseRule{}, + &rule.ConfusingNamingRule{}, &rule.GetReturnRule{}, &rule.ModifiesParamRule{}, + &rule.ConfusingResultsRule{}, &rule.DeepExitRule{}, +<<<<<<< HEAD &rule.ADSPrintRule{}, &rule.ADSLostErrRule{}, +======= + &rule.UnusedParamRule{}, + &rule.UnreachableCodeRule{}, + &rule.AddConstantRule{}, + &rule.FlagParamRule{}, + &rule.UnnecessaryStmtRule{}, + &rule.StructTagRule{}, + &rule.ModifiesValRecRule{}, + &rule.ConstantLogicalExprRule{}, + &rule.BoolLiteralRule{}, + &rule.RedefinesBuiltinIDRule{}, + &rule.ImportsBlacklistRule{}, + &rule.FunctionResultsLimitRule{}, + &rule.MaxPublicStructsRule{}, + &rule.RangeValInClosureRule{}, + &rule.WaitGroupByValueRule{}, + &rule.AtomicRule{}, + &rule.EmptyLinesRule{}, + &rule.LineLengthLimitRule{}, +>>>>>>> ad7df7dc375879b9c25628acf9d643378cfaf102 }, defaultRules...) var allFormatters = []lint.Formatter{ &formatter.Stylish{}, &formatter.Friendly{}, &formatter.JSON{}, + &formatter.NDJSON{}, &formatter.Default{}, + &formatter.Unix{}, + &formatter.Checkstyle{}, + &formatter.Plain{}, } func getFormatters() map[string]lint.Formatter { diff --git a/defaults.toml b/defaults.toml index d5d43ac..2e99473 100644 --- a/defaults.toml +++ b/defaults.toml @@ -22,4 +22,4 @@ warningCode = 0 [rule.time-naming] [rule.unexported-return] [rule.indent-error-flow] -[rule.errorf] \ No newline at end of file +[rule.errorf] diff --git a/fixtures/add-constant.go b/fixtures/add-constant.go new file mode 100644 index 0000000..d240f40 --- /dev/null +++ b/fixtures/add-constant.go @@ -0,0 +1,16 @@ +package fixtures + +func foo(a, b, c, d int) { + a = 1.0 // ignore + b = "ignore" + c = 2 // ignore + println("lit", 12) // MATCH /avoid magic numbers like '12', create a named constant for it/ + if a == 12.50 { // MATCH /avoid magic numbers like '12.50', create a named constant for it/ + if b == "lit" { + c = "lit" // MATCH /string literal "lit" appears, at least, 3 times, create a named constant for it/ + } + for i := 0; i < 1; i++ { + println("lit") + } + } +} diff --git a/fixtures/atomic.go b/fixtures/atomic.go new file mode 100644 index 0000000..cbd0550 --- /dev/null +++ b/fixtures/atomic.go @@ -0,0 +1,45 @@ +package fixtures + +import ( + "sync/atomic" +) + +type Counter uint64 + +func AtomicTests() { + x := uint64(1) + x = atomic.AddUint64(&x, 1) // MATCH /direct assignment to atomic value/ + _, x = 10, atomic.AddUint64(&x, 1) // MATCH /direct assignment to atomic value/ + x, _ = atomic.AddUint64(&x, 1), 10 // MATCH /direct assignment to atomic value/ + + y := &x + *y = atomic.AddUint64(y, 1) // MATCH /direct assignment to atomic value/ + + var su struct{ Counter uint64 } + su.Counter = atomic.AddUint64(&su.Counter, 1) // MATCH /direct assignment to atomic value/ + z1 := atomic.AddUint64(&su.Counter, 1) + _ = z1 // Avoid err "z declared and not used" + + var sp struct{ Counter *uint64 } + *sp.Counter = atomic.AddUint64(sp.Counter, 1) // MATCH /direct assignment to atomic value/ + z2 := atomic.AddUint64(sp.Counter, 1) + _ = z2 // Avoid err "z declared and not used" + + au := []uint64{10, 20} + au[0] = atomic.AddUint64(&au[0], 1) // MATCH /direct assignment to atomic value/ + au[1] = atomic.AddUint64(&au[0], 1) + + ap := []*uint64{&au[0], &au[1]} + *ap[0] = atomic.AddUint64(ap[0], 1) // MATCH /direct assignment to atomic value/ + *ap[1] = atomic.AddUint64(ap[0], 1) +} + +type T struct{} + +func (T) AddUint64(addr *uint64, delta uint64) uint64 { return 0 } + +func NonAtomic() { + x := uint64(1) + var atomic T + x = atomic.AddUint64(&x, 1) // MATCH /direct assignment to atomic value/ +} diff --git a/fixtures/bool-literal-in-expr.go b/fixtures/bool-literal-in-expr.go new file mode 100644 index 0000000..22614c6 --- /dev/null +++ b/fixtures/bool-literal-in-expr.go @@ -0,0 +1,54 @@ +package fixtures + +func foo(a, b, c, d int) bool { + if bar == true { // MATCH /omit Boolean literal in expression/ + + } + for f() || false != yes { // MATCH /omit Boolean literal in expression/ + + } + + return b > c == false // MATCH /omit Boolean literal in expression/ +} + +// from github.com/jmespath/go-jmespath/functions.go +func jpfToNumber(arguments []interface{}) (interface{}, error) { + arg := arguments[0] + // code skipped + if arg == true || // MATCH /omit Boolean literal in expression/ + arg == false { // MATCH /omit Boolean literal in expression/ + return nil, nil + } + return nil, errors.New("unknown type") +} + +// from gopkg.in/yaml.v2/resolve.go +func resolve(tag string, in string) (rtag string, out interface{}) { + if err == nil { + if true || intv == int64(int(intv)) { // MATCH /Boolean expression seems to always evaluate to true/ + return yaml_INT_TAG, int(intv) + } else { + return yaml_INT_TAG, intv + } + } +} + +// from github.com/miekg/dns/msg_helpers.go +func packDataDomainNames(names []string, msg []byte, off int, compression map[string]int, compress bool) (int, error) { + var err error + for j := 0; j < len(names); j++ { + off, err = PackDomainName(names[j], msg, off, compression, false && compress) // MATCH /Boolean expression seems to always evaluate to false/ + if err != nil { + return len(msg), err + } + } + return off, nil +} + +func isTrue(arg bool) bool { + return arg +} + +func main() { + isTrue(true) +} diff --git a/fixtures/confusing-naming1.go b/fixtures/confusing-naming1.go new file mode 100644 index 0000000..ba12b07 --- /dev/null +++ b/fixtures/confusing-naming1.go @@ -0,0 +1,62 @@ +// Test of confusing-naming rule. + +// Package pkg ... +package pkg + +type foo struct{} + +func (t foo) aFoo() { + return +} + +func (t *foo) AFoo() { // MATCH /Method 'AFoo' differs only by capitalization to method 'aFoo' in the same source file/ + return +} + +type bar struct{} + +func (t *bar) aBar() { + return +} + +func (t *bar) aFoo() { // Should not warn + return +} + +func aGlobal() { + +} + +func AGlobal() { // MATCH /Method 'AGlobal' differs only by capitalization to function 'aGlobal' in the same source file/ +} + +func ABar() { // Should not warn + +} + +func aFoo() { // Should not warn + +} + +func (t foo) ABar() { // Should not warn + return +} + +func (t bar) ABar() { // MATCH /Method 'ABar' differs only by capitalization to method 'aBar' in the same source file/ + return +} + +func x() {} + +type tFoo struct { + asd string + aSd int // MATCH /Field 'aSd' differs only by capitalization to other field in the struct type tFoo/ + qwe, asD bool // MATCH /Field 'asD' differs only by capitalization to other field in the struct type tFoo/ + zxc float32 +} + +type tBar struct { + asd string + qwe bool + zxc float32 +} diff --git a/fixtures/confusing-naming2.go b/fixtures/confusing-naming2.go new file mode 100644 index 0000000..616e5de --- /dev/null +++ b/fixtures/confusing-naming2.go @@ -0,0 +1,7 @@ +// Test of confusing-naming rule. + +// Package pkg ... +package pkg + +func aglobal() { // MATCH /Function 'aglobal' differs only by capitalization to other function in the same package/ +} diff --git a/fixtures/confusing-results.go b/fixtures/confusing-results.go new file mode 100644 index 0000000..772fc45 --- /dev/null +++ b/fixtures/confusing-results.go @@ -0,0 +1,20 @@ +package fixtures + +func getfoo() (int, int, error) { // MATCH /unnamed results of the same type may be confusing, consider using named results/ + +} + +func getBar(a, b int) (int, error, int) { +} + +func Getbaz(a string, b int) (int, float32, string, string) { // MATCH /unnamed results of the same type may be confusing, consider using named results/ + +} + +func GetTaz(a string, b int) string { + +} + +func (t *t) GetTaz(a int, b int) { + +} diff --git a/fixtures/constant-logical-expr.go b/fixtures/constant-logical-expr.go new file mode 100644 index 0000000..5adb404 --- /dev/null +++ b/fixtures/constant-logical-expr.go @@ -0,0 +1,26 @@ +package fixtures + +// from github.com/ugorji/go/codec/helper.go +func isNaN(f float64) bool { return f != f } // MATCH /expression always evaluates to false/ + +func skip(f float64) bool { return f != g } + +func foo1(f float64) bool { return foo2(2.) > foo2(2.) } // MATCH /expression always evaluates to false/ + +func foo2(f float64) bool { return f < f } // MATCH /expression always evaluates to false/ + +func foo3(f float64) bool { return f <= f } // MATCH /expression always evaluates to false/ + +func foo4(f float64) bool { return f >= f } // MATCH /expression always evaluates to false/ + +func foo5(f float64) bool { return f == f } // MATCH /expression always evaluates to true/ + +func foo6(f float64) bool { return fmt.Sprintf("%s", buf1.Bytes()) == fmt.Sprintf("%s", buf1.Bytes()) } // MATCH /expression always evaluates to true/ + +func foo7(f float64) bool { + return fFoo(fBar(isNaN(10.), bpar), 10000) || fFoo(fBar(isNaN(10.), bpar), 10000) // MATCH /left and right hand-side sub-expressions are the same/ +} + +func foo8(f float64) bool { + return fFoo(fBar(isNaN(10.), bpar), 10000) && fFoo(fBar(isNaN(10.), bpar), 10000) // MATCH /left and right hand-side sub-expressions are the same/ +} diff --git a/fixtures/empty-lines.go b/fixtures/empty-lines.go new file mode 100644 index 0000000..0e9d57c --- /dev/null +++ b/fixtures/empty-lines.go @@ -0,0 +1,162 @@ +// Test of empty-lines. + +package fixtures + +import "time" + +func f1(x *int) bool { // MATCH /extra empty line at the start of a block/ + + return x > 2 +} + +func f2(x *int) bool { + return x > 2 // MATCH /extra empty line at the end of a block/ + +} + +func f3(x *int) bool { // MATCH /extra empty line at the start of a block/ + + return x > 2 // MATCH /extra empty line at the end of a block/ + +} + +func f4(x *int) bool { + // This is fine. + return x > 2 +} + +func f5(x *int) bool { // MATCH /extra empty line at the start of a block/ + + // This is _not_ fine. + return x > 2 +} + +func f6(x *int) bool { + return x > 2 + // This is fine. +} + +func f7(x *int) bool { + return x > 2 // MATCH /extra empty line at the end of a block/ + // This is _not_ fine. + +} + +func f8(*int) bool { + if x > 2 { // MATCH /extra empty line at the start of a block/ + + return true + } + + return false +} + +func f9(*int) bool { + if x > 2 { + return true // MATCH /extra empty line at the end of a block/ + + } + + return false +} + +func f10(*int) bool { // MATCH /extra empty line at the start of a block/ + + if x > 2 { + return true + } + + return false +} + +func f11(x *int) bool { + if x > 2 { + return true + } +} + +func f12(x *int) bool { + if x > 2 { // MATCH /extra empty line at the end of a block/ + return true + } + +} + +func f13(x *int) bool { + switch { + case x == 2: + return false + } +} + +func f14(x *int) bool { + switch { // MATCH /extra empty line at the end of a block/ + case x == 2: + return false + } + +} + +func f15(x *int) bool { + switch { + case x == 2: // MATCH /extra empty line at the end of a block/ + return false + + } +} + +func f16(x *int) bool { + return Query( + qm("x = ?", x), + ).Execute() +} + +func f17(x *int) bool { + return Query( // MATCH /extra empty line at the end of a block/ + qm("x = ?", x), + ).Execute() + +} + +func f18(x *int) bool { + if true { + if true { // MATCH /extra empty line at the end of a block/ + return true + } + + // TODO: should we handle the error here? + } + + return false +} + +func w() { + select { + case <-time.After(dur): + // TODO: Handle Ctrl-C is pressed in `mysql` client. + // return 1 when SLEEP() is KILLed + } + return 0, false, nil +} + +func x() { + if tagArray[2] == "req" { + bit := len(u.reqFields) + u.reqFields = append(u.reqFields, name) + reqMask = uint64(1) << uint(bit) + // TODO: if we have more than 64 required fields, we end up + // not verifying that all required fields are present. + // Fix this, perhaps using a count of required fields? + } + + if err == nil { // No need to refresh if the stream is over or failed. + // Consider any buffered body data (read from the conn but not + // consumed by the client) when computing flow control for this + // stream. + v := int(cs.inflow.available()) + cs.bufPipe.Len() + if v < transportDefaultStreamFlow-transportDefaultStreamMinRefresh { + streamAdd = int32(transportDefaultStreamFlow - v) + cs.inflow.add(streamAdd) + } + } +} diff --git a/fixtures/flag-param.go b/fixtures/flag-param.go new file mode 100644 index 0000000..3b235a6 --- /dev/null +++ b/fixtures/flag-param.go @@ -0,0 +1,11 @@ +package fixtures + +func foo(a bool, b int) { // MATCH /parameter 'a' seems to be a control flag, avoid control coupling/ + if a { + + } +} + +func foo(a bool, b int) { + str := mystruct{a, b} +} diff --git a/fixtures/function-result-limit.go b/fixtures/function-result-limit.go new file mode 100644 index 0000000..e278a80 --- /dev/null +++ b/fixtures/function-result-limit.go @@ -0,0 +1,17 @@ +package fixtures + +func foo() (a, b, c, d) { // MATCH /maximum number of return results per function exceeded; max 3 but got 4/ + var a, b, c, d int +} + +func bar(a, b int) { + +} + +func baz(a string, b int) { + +} + +func qux() (string, string, int, string, int) { // MATCH /maximum number of return results per function exceeded; max 3 but got 5/ + +} diff --git a/fixtures/golint/exported.go b/fixtures/golint/exported.go new file mode 100644 index 0000000..fb2880e --- /dev/null +++ b/fixtures/golint/exported.go @@ -0,0 +1,15 @@ +// Package golint comment +package golint + +type ( + // O is a shortcut (alias) for map[string]interface{}, e.g. a JSON object. + O = map[string]interface{} + + // A is shortcut for []O. + A = []O + + // This Person type is simple + Person = map[string]interface{} +) + +type Foo struct{} // MATCH /exported type Foo should have comment or be unexported/ diff --git a/fixtures/imports-blacklist.go b/fixtures/imports-blacklist.go new file mode 100644 index 0000000..cdd5ef4 --- /dev/null +++ b/fixtures/imports-blacklist.go @@ -0,0 +1,7 @@ +package fixtures + +import ( + "crypto/md5" // MATCH /should not use the following blacklisted import: "crypto/md5"/ + "crypto/sha1" // MATCH /should not use the following blacklisted import: "crypto/sha1"/ + "strings" +) diff --git a/fixtures/line-length-limit.go b/fixtures/line-length-limit.go new file mode 100644 index 0000000..abc1200 --- /dev/null +++ b/fixtures/line-length-limit.go @@ -0,0 +1,7 @@ +package fixtures + +import "fmt" + +func foo(a, b int) { + fmt.Printf("single line characters out of limit") // MATCH /line is 105 characters, out of limit 100/ +} diff --git a/fixtures/modifies-value-receiver.go b/fixtures/modifies-value-receiver.go new file mode 100644 index 0000000..e7355a4 --- /dev/null +++ b/fixtures/modifies-value-receiver.go @@ -0,0 +1,13 @@ +package fixtures + +type data struct { + num int + key *string + items map[string]bool +} + +func (this data) vmethod() { + this.num = 8 // MATCH /suspicious assignment to a by-value method receiver/ + *this.key = "v.key" + this.items["vmethod"] = true +} diff --git a/fixtures/range-val-in-closure.go b/fixtures/range-val-in-closure.go new file mode 100644 index 0000000..cdda5d0 --- /dev/null +++ b/fixtures/range-val-in-closure.go @@ -0,0 +1,37 @@ +package fixtures + +import "fmt" + +func foo() { + mySlice := []string{"A", "B", "C"} + for index, value := range mySlice { + go func() { + fmt.Printf("Index: %d\n", index) // MATCH /loop variable index captured by func literal/ + fmt.Printf("Value: %s\n", value) // MATCH /loop variable value captured by func literal/ + }() + } + + myDict := make(map[string]int) + myDict["A"] = 1 + myDict["B"] = 2 + myDict["C"] = 3 + for key, value := range myDict { + defer func() { + fmt.Printf("Index: %d\n", key) // MATCH /loop variable key captured by func literal/ + fmt.Printf("Value: %s\n", value) // MATCH /loop variable value captured by func literal/ + }() + } + + for i, newg := range groups { + go func(newg int) { + newg.run(m.opts.Context,i) // MATCH /loop variable i captured by func literal/ + }(newg) + } + + for i, newg := range groups { + newg := newg + go func() { + newg.run(m.opts.Context,i) // MATCH /loop variable i captured by func literal/ + }() + } +} diff --git a/fixtures/redefines-builtin-id.go b/fixtures/redefines-builtin-id.go new file mode 100644 index 0000000..5b25f02 --- /dev/null +++ b/fixtures/redefines-builtin-id.go @@ -0,0 +1,21 @@ +package fixtures + +func (this data) vmethod() { + nil := true // MATCH /assignment creates a shadow of built-in identifier nil/ + iota = 1 // MATCH /assignment modifies built-in identifier iota/ +} + +func append(i, j int) { // MATCH /redefinition of the built-in function append/ + +} + +type Type int16 // MATCH /redefinition of the built-in type Type/ + +func delete(set []int64, i int) (y []int64) { // MATCH /redefinition of the built-in function delete/ + for j, v := range set { + if j != i { + y = append(y, v) + } + } + return +} diff --git a/fixtures/struct-tag.go b/fixtures/struct-tag.go new file mode 100644 index 0000000..86f2447 --- /dev/null +++ b/fixtures/struct-tag.go @@ -0,0 +1,53 @@ +package fixtures + +type decodeAndValidateRequest struct { + // BEAWRE : the flag of URLParam should match the const string URLParam + URLParam string `json:"-" path:"url_param" validate:"numeric"` + Text string `json:"text" validate:"max=10"` + DefaultInt int `json:"defaultInt" default:"10.0"` // MATCH /field's type and default value's type mismatch/ + DefaultInt2 int `json:"defaultInt" default:"10"` + DefaultString string `json:"defaultString" default:"foo"` + DefaultBool bool `json:"defaultBool" default:"trues"` // MATCH /field's type and default value's type mismatch/ + DefaultBool2 bool `json:"defaultBool" default:"true"` + DefaultBool3 bool `json:"defaultBool" default:"false"` + DefaultFloat float64 `json:"defaultFloat" default:"f10.0"` // MATCH /field's type and default value's type mismatch/ + DefaultFloat2 float64 `json:"defaultFloat" default:"10.0"` + MandatoryStruct mandatoryStruct `json:"mandatoryStruct" required:"trues"` // MATCH /required should be 'true' or 'false'/ + MandatoryStruct2 mandatoryStruct `json:"mandatoryStruct" required:"true"` + MandatoryStruct4 mandatoryStruct `json:"mandatoryStruct" required:"false"` + OptionalStruct *optionalStruct `json:"optionalStruct,omitempty"` + OptionalQuery string `json:"-" querystring:"queryfoo"` +} + +type RangeAllocation struct { + metav1.TypeMeta `json:",inline"` // MATCH /unknown option 'inline' in JSON tag/ + metav1.ObjectMeta `json:"metadata,omitempty"` + Range string `json:"range,flow"` // MATCH /unknown option 'flow' in JSON tag/ + Data []byte `json:"data,inline"` // MATCH /unknown option 'inline' in JSON tag/ +} + +type RangeAllocation struct { + metav1.TypeMeta `bson:",minsize"` + metav1.ObjectMeta `bson:"metadata,omitempty"` + Range string `bson:"range,flow"` // MATCH /unknown option 'flow' in BSON tag/ + Data []byte `bson:"data,inline"` +} + +type TestContextSpecificTags2 struct { + A int `asn1:"explicit,tag:1"` + B int `asn1:"tag:2"` + S string `asn1:"tag:0,utf8"` + Ints []int `asn1:"set"` + Version int `asn1:"optional,explicit,default:0,tag:0"` // MATCH /duplicated tag number 0/ + Time time.Time `asn1:"explicit,tag:4,other"` // MATCH /unknown option 'other' in ASN1 tag/ +} + +type VirtualMachineRelocateSpecDiskLocator struct { + DynamicData + + DiskId int32 `xml:"diskId,attr,cdata"` + Datastore ManagedObjectReference `xml:"datastore,chardata,innerxml"` + DiskMoveType string `xml:"diskMoveType,omitempty,comment"` + DiskBackingInfo BaseVirtualDeviceBackingInfo `xml:"diskBackingInfo,omitempty,any"` + Profile []BaseVirtualMachineProfileSpec `xml:"profile,omitempty,other"` // MATCH /unknown option 'other' in XML tag/ +} diff --git a/fixtures/unnecessary-stmt.go b/fixtures/unnecessary-stmt.go new file mode 100644 index 0000000..9969a8a --- /dev/null +++ b/fixtures/unnecessary-stmt.go @@ -0,0 +1,49 @@ +package fixtures + +func foo(a, b, c, d int) { + switch n := node.(type) { // MATCH /switch with only one case can be replaced by an if-then/ + case *ast.SwitchStmt: + caseSelector := func(n ast.Node) bool { + _, ok := n.(*ast.CaseClause) + return ok + } + cases := pick(n.Body, caseSelector, nil) + if len(cases) == 1 { + cs, ok := cases[0].(*ast.CaseClause) + if ok && len(cs.List) == 1 { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: n, + Category: "style", + Failure: "switch can be replaced by an if-then", + }) + } + } + } +} + +func bar() { + a := 1 + + switch a { + case 1, 2: + a++ + } + +loop: + for { + switch a { + case 1: + a++ + println("one") + break // MATCH /omit unnecessary break at the end of case clause/ + case 2: + println("two") + break loop + default: + println("default") + } + } + + return // MATCH /omit unnecessary return statement/ +} diff --git a/fixtures/unreachable-code.go b/fixtures/unreachable-code.go new file mode 100644 index 0000000..346e1cb --- /dev/null +++ b/fixtures/unreachable-code.go @@ -0,0 +1,37 @@ +package fixtures + +import ( + "fmt" + "log" + "os" +) + +func foo() int { + log.Fatalf("%s", "About to fail") // ignore + return 0 // MATCH /unreachable code after this statement/ + return 1 + Println("unreachable") +} + +func f() { + fmt.Println("Hello, playground") + if true { + return // MATCH /unreachable code after this statement/ + Println("unreachable") + os.Exit(2) // ignore + Println("also unreachable") + } + return // MATCH /unreachable code after this statement/ + fmt.Println("Bye, playground") +} + +func g() { + fmt.Println("Hello, playground") + if true { + return // ignore if next stmt is labeled + label: + os.Exit(2) // ignore + } + + fmt.Println("Bye, playground") +} diff --git a/fixtures/unused-param.go b/fixtures/unused-param.go new file mode 100644 index 0000000..bd7acc3 --- /dev/null +++ b/fixtures/unused-param.go @@ -0,0 +1,169 @@ +package fixtures + +import ( + "fmt" + "go/ast" + "io/ioutil" + "os" + "runtime" + "testing" + "time" + + "github.com/mgechev/revive/lint" +) + +func f0(param int) { + param := param +} + +func f1(param int) { // MATCH /parameter 'param' seems to be unused, consider removing or renaming it as _/ + if param := fn(); predicate(param) { + // do stuff + } +} + +func f2(param int) { // MATCH /parameter 'param' seems to be unused, consider removing or renaming it as _/ + switch param := fn(); param { + default: + + } +} + +func f3(param myStruct) { + a := param.field +} + +func f4(param myStruct, c int) { // MATCH /parameter 'c' seems to be unused, consider removing or renaming it as _/ + param.field = "aString" + param.c = "sss" +} + +func f5(a int, _ float) { // MATCH /parameter 'a' seems to be unused, consider removing or renaming it as _/ + fmt.Printf("Hello, Golang\n") + { + if true { + a := 2 + b := a + } + } +} + +func f6(unused string) { // MATCH /parameter 'unused' seems to be unused, consider removing or renaming it as _/ + switch unused := runtime.GOOS; unused { + case "darwin": + fmt.Println("OS X.") + case "linux": + fmt.Println("Linux.") + default: + fmt.Printf("%s.", unused) + } + for unused := 0; unused < 10; unused++ { + sum += unused + } + { + unused := 1 + } +} + +func f6bis(unused string) { + switch unused := runtime.GOOS; unused { + case "darwin": + fmt.Println("OS X.") + case "linux": + fmt.Println("Linux.") + default: + fmt.Printf("%s.", unused) + } + for unused := 0; unused < 10; unused++ { + sum += unused + } + { + unused := 1 + } + + fmt.Print(unused) +} + +func f7(pl int) { + for i := 0; pl < i; i-- { + + } +} + +func getCompareFailCause(n *node, which int, prevValue string, prevIndex uint64) string { + switch which { + case CompareIndexNotMatch: + return fmt.Sprintf("[%v != %v]", prevIndex, n.ModifiedIndex) + case CompareValueNotMatch: + return fmt.Sprintf("[%v != %v]", prevValue, n.Value) + default: + return fmt.Sprintf("[%v != %v] [%v != %v]", prevValue, n.Value, prevIndex, n.ModifiedIndex) + } +} + +func assertSuccess(t *testing.T, baseDir string, fi os.FileInfo, src []byte, rules []lint.Rule, config map[string]lint.RuleConfig) error { // MATCH /parameter 'src' seems to be unused, consider removing or renaming it as _/ + l := lint.New(func(file string) ([]byte, error) { + return ioutil.ReadFile(baseDir + file) + }) + + ps, err := l.Lint([][]string{[]string{fi.Name()}}, rules, lint.Config{ + Rules: config, + }) + if err != nil { + return err + } + + failures := "" + for p := range ps { + failures += p.Failure + } + if failures != "" { + t.Errorf("Expected the rule to pass but got the following failures: %s", failures) + } + return nil +} + +func (w lintCyclomatic) Visit(n ast.Node) ast.Visitor { // MATCH /parameter 'n' seems to be unused, consider removing or renaming it as _/ + f := w.file + for _, decl := range f.AST.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok { + c := complexity(fn) + if c > w.complexity { + w.onFailure(lint.Failure{ + Confidence: 1, + Category: "maintenance", + Failure: fmt.Sprintf("function %s has cyclomatic complexity %d", funcName(fn), c), + Node: fn, + }) + } + } + } + return nil +} + +func extÛ°timeÛ°Sleep(fr *frame, args []value) value { // MATCH /parameter 'fr' seems to be unused, consider removing or renaming it as _/ + time.Sleep(time.Duration(args[0].(int64))) + return nil +} + +func (c *chanList) remove(id uint32) { + id -= c.offset +} + +func (c *chanList) remove1(id uint32) { + id *= c.offset +} + +func (c *chanList) remove2(id uint32) { + id /= c.offset +} + +func (c *chanList) remove3(id uint32) { + id += c.offset +} + +func encodeFixed64Rpc(dAtA []byte, offset int, v uint64, i int) int { + dAtA[offset+i] = uint8(v) + + return 8 +} diff --git a/fixtures/var-naming.go b/fixtures/var-naming.go new file mode 100644 index 0000000..2cf86f3 --- /dev/null +++ b/fixtures/var-naming.go @@ -0,0 +1,7 @@ +package fixtures + +func foo() string { + customId := "result" + customVm := "result" // MATCH /var customVm should be customVM/ + return customId +} diff --git a/fixtures/waitgroup-by-value.go b/fixtures/waitgroup-by-value.go new file mode 100644 index 0000000..37fbb4e --- /dev/null +++ b/fixtures/waitgroup-by-value.go @@ -0,0 +1,21 @@ +package fixtures + +import ( + "sync" +) + +func foo(a int, b float32, c char, d sync.WaitGroup) { // MATCH /sync.WaitGroup passed by value, the function will get a copy of the original one/ + +} + +func bar(a, b sync.WaitGroup) { // MATCH /sync.WaitGroup passed by value, the function will get a copy of the original one/ + +} + +func baz(zz sync.WaitGroup) { // MATCH /sync.WaitGroup passed by value, the function will get a copy of the original one/ + +} + +func ok(zz *sync.WaitGroup) { + +} diff --git a/formatter/checkstyle.go b/formatter/checkstyle.go new file mode 100644 index 0000000..6f7e20a --- /dev/null +++ b/formatter/checkstyle.go @@ -0,0 +1,76 @@ +package formatter + +import ( + "bytes" + "encoding/xml" + "github.com/mgechev/revive/lint" + plainTemplate "text/template" +) + +// Checkstyle is an implementation of the Formatter interface +// which formats the errors to Checkstyle-like format. +type Checkstyle struct { + Metadata lint.FormatterMetadata +} + +// Name returns the name of the formatter +func (f *Checkstyle) Name() string { + return "checkstyle" +} + +type issue struct { + Line int + Col int + What string + Confidence float64 + Severity lint.Severity + RuleName string +} + +// Format formats the failures gotten from the lint. +func (f *Checkstyle) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) { + var issues = map[string][]issue{} + for failure := range failures { + buf := new(bytes.Buffer) + xml.Escape(buf, []byte(failure.Failure)) + what := buf.String() + iss := issue{ + Line: failure.Position.Start.Line, + Col: failure.Position.Start.Column, + What: what, + Confidence: failure.Confidence, + Severity: severity(config, failure), + RuleName: failure.RuleName, + } + fn := failure.GetFilename() + if issues[fn] == nil { + issues[fn] = make([]issue, 0) + } + issues[fn] = append(issues[fn], iss) + } + + t, err := plainTemplate.New("revive").Parse(checkstyleTemplate) + if err != nil { + return "", err + } + + buf := new(bytes.Buffer) + + err = t.Execute(buf, issues) + if err != nil { + return "", err + } + + return buf.String(), nil +} + +const checkstyleTemplate = ` + +{{- range $k, $v := . }} + + {{- range $i, $issue := $v }} + + {{- end }} + +{{- end }} +` diff --git a/formatter/default.go b/formatter/default.go index 06a07cd..415c95c 100644 --- a/formatter/default.go +++ b/formatter/default.go @@ -7,7 +7,7 @@ import ( ) // Default is an implementation of the Formatter interface -// which formats the errors to JSON. +// which formats the errors to text. type Default struct { Metadata lint.FormatterMetadata } @@ -18,7 +18,7 @@ func (f *Default) Name() string { } // Format formats the failures gotten from the lint. -func (f *Default) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) { +func (f *Default) Format(failures <-chan lint.Failure, _ lint.RulesConfig) (string, error) { for failure := range failures { fmt.Printf("%v: %s\n", failure.Position.Start, failure.Failure) } diff --git a/formatter/friendly.go b/formatter/friendly.go index 84b5d34..01f22c8 100644 --- a/formatter/friendly.go +++ b/formatter/friendly.go @@ -72,7 +72,7 @@ func (f *Friendly) printHeaderRow(failure lint.Failure, severity lint.Severity) if severity == lint.SeverityError { emoji = errorEmoji } - fmt.Print(f.table([][]string{{emoji, failure.RuleName, color.GreenString(failure.Failure)}})) + fmt.Print(f.table([][]string{{emoji, "https://revive.run/r#" + failure.RuleName, color.GreenString(failure.Failure)}})) } func (f *Friendly) printFilePosition(failure lint.Failure) { diff --git a/formatter/json.go b/formatter/json.go index d24c4b8..74ed585 100644 --- a/formatter/json.go +++ b/formatter/json.go @@ -17,11 +17,20 @@ func (f *JSON) Name() string { return "json" } +// jsonObject defines a JSON object of an failure +type jsonObject struct { + Severity lint.Severity + lint.Failure `json:",inline"` +} + // Format formats the failures gotten from the lint. func (f *JSON) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) { - var slice []lint.Failure + var slice []jsonObject for failure := range failures { - slice = append(slice, failure) + obj := jsonObject{} + obj.Severity = severity(config, failure) + obj.Failure = failure + slice = append(slice, obj) } result, err := json.Marshal(slice) if err != nil { diff --git a/formatter/ndjson.go b/formatter/ndjson.go new file mode 100644 index 0000000..7b5ba61 --- /dev/null +++ b/formatter/ndjson.go @@ -0,0 +1,34 @@ +package formatter + +import ( + "encoding/json" + "os" + + "github.com/mgechev/revive/lint" +) + +// NDJSON is an implementation of the Formatter interface +// which formats the errors to NDJSON stream. +type NDJSON struct { + Metadata lint.FormatterMetadata +} + +// Name returns the name of the formatter +func (f *NDJSON) Name() string { + return "ndjson" +} + +// Format formats the failures gotten from the lint. +func (f *NDJSON) Format(failures <-chan lint.Failure, config lint.RulesConfig) (string, error) { + enc := json.NewEncoder(os.Stdout) + for failure := range failures { + obj := jsonObject{} + obj.Severity = severity(config, failure) + obj.Failure = failure + err := enc.Encode(obj) + if err != nil { + return "", err + } + } + return "", nil +} diff --git a/formatter/plain.go b/formatter/plain.go new file mode 100644 index 0000000..aef9a0c --- /dev/null +++ b/formatter/plain.go @@ -0,0 +1,26 @@ +package formatter + +import ( + "fmt" + + "github.com/mgechev/revive/lint" +) + +// Plain is an implementation of the Formatter interface +// which formats the errors to JSON. +type Plain struct { + Metadata lint.FormatterMetadata +} + +// Name returns the name of the formatter +func (f *Plain) Name() string { + return "plain" +} + +// Format formats the failures gotten from the lint. +func (f *Plain) Format(failures <-chan lint.Failure, _ lint.RulesConfig) (string, error) { + for failure := range failures { + fmt.Printf("%v: %s %s\n", failure.Position.Start, failure.Failure, "https://revive.run/r#"+failure.RuleName) + } + return "", nil +} diff --git a/formatter/stylish.go b/formatter/stylish.go index 8e86725..9d3286c 100644 --- a/formatter/stylish.go +++ b/formatter/stylish.go @@ -22,11 +22,11 @@ func (f *Stylish) Name() string { func formatFailure(failure lint.Failure, severity lint.Severity) []string { fString := color.CyanString(failure.Failure) - fName := color.RedString(failure.RuleName) + fName := color.RedString("https://revive.run/r#" + failure.RuleName) lineColumn := failure.Position pos := fmt.Sprintf("(%d, %d)", lineColumn.Start.Line, lineColumn.Start.Column) if severity == lint.SeverityWarning { - fName = color.YellowString(failure.RuleName) + fName = color.YellowString("https://revive.run/r#" + failure.RuleName) } return []string{failure.GetFilename(), pos, fName, fString} } diff --git a/formatter/unix.go b/formatter/unix.go new file mode 100644 index 0000000..849866f --- /dev/null +++ b/formatter/unix.go @@ -0,0 +1,27 @@ +package formatter + +import ( + "fmt" + + "github.com/mgechev/revive/lint" +) + +// Unix is an implementation of the Formatter interface +// which formats the errors to a simple line based error format +// main.go:24:9: [errorf] should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...) +type Unix struct { + Metadata lint.FormatterMetadata +} + +// Name returns the name of the formatter +func (f *Unix) Name() string { + return "unix" +} + +// Format formats the failures gotten from the lint. +func (f *Unix) Format(failures <-chan lint.Failure, _ lint.RulesConfig) (string, error) { + for failure := range failures { + fmt.Printf("%v: [%s] %s\n", failure.Position.Start, failure.RuleName, failure.Failure) + } + return "", nil +} diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..48b56cf --- /dev/null +++ b/go.mod @@ -0,0 +1,15 @@ +module github.com/mgechev/revive + +require ( + github.com/BurntSushi/toml v0.3.0 + github.com/fatih/color v1.7.0 + github.com/fatih/structtag v1.0.0 + github.com/mattn/go-colorable v0.0.9 // indirect + github.com/mattn/go-isatty v0.0.4 // indirect + github.com/mattn/go-runewidth v0.0.3 // indirect + github.com/mgechev/dots v0.0.0-20180605013149-8e09d8ea2757 + github.com/olekukonko/tablewriter v0.0.0-20180912035003-be2c049b30cc + github.com/pkg/errors v0.8.0 + golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e // indirect + golang.org/x/tools v0.0.0-20180911133044-677d2ff680c1 +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..3f3de89 --- /dev/null +++ b/go.sum @@ -0,0 +1,22 @@ +github.com/BurntSushi/toml v0.3.0 h1:e1/Ivsx3Z0FVTV0NSOv/aVgbUWyQuzj7DDnFblkRvsY= +github.com/BurntSushi/toml v0.3.0/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/fatih/color v1.7.0 h1:DkWD4oS2D8LGGgTQ6IvwJJXSL5Vp2ffcQg58nFV38Ys= +github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= +github.com/fatih/structtag v1.0.0 h1:pTHj65+u3RKWYPSGaU290FpI/dXxTaHdVwVwbcPKmEc= +github.com/fatih/structtag v1.0.0/go.mod h1:IKitwq45uXL/yqi5mYghiD3w9H6eTOvI9vnk8tXMphA= +github.com/mattn/go-colorable v0.0.9 h1:UVL0vNpWh04HeJXV0KLcaT7r06gOH2l4OW6ddYRUIY4= +github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= +github.com/mattn/go-isatty v0.0.4 h1:bnP0vzxcAdeI1zdubAl5PjU6zsERjGZb7raWodagDYs= +github.com/mattn/go-isatty v0.0.4/go.mod h1:M+lRXTBqGeGNdLjl/ufCoiOlB5xdOkqRJdNxMWT7Zi4= +github.com/mattn/go-runewidth v0.0.3 h1:a+kO+98RDGEfo6asOGMmpodZq4FNtnGP54yps8BzLR4= +github.com/mattn/go-runewidth v0.0.3/go.mod h1:LwmH8dsx7+W8Uxz3IHJYH5QSwggIsqBzpuz5H//U1FU= +github.com/mgechev/dots v0.0.0-20180605013149-8e09d8ea2757 h1:KTwJ7Lo3KDKMknRYN5JEFRGIM4IkG59QjFFM2mxsMEU= +github.com/mgechev/dots v0.0.0-20180605013149-8e09d8ea2757/go.mod h1:KQ7+USdGKfpPjXk4Ga+5XxQM4Lm4e3gAogrreFAYpOg= +github.com/olekukonko/tablewriter v0.0.0-20180912035003-be2c049b30cc h1:rQ1O4ZLYR2xXHXgBCCfIIGnuZ0lidMQw2S5n1oOv+Wg= +github.com/olekukonko/tablewriter v0.0.0-20180912035003-be2c049b30cc/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= +github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw= +github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e h1:o3PsSEY8E4eXWkXrIP9YJALUkVZqzHJT5DOasTyn8Vs= +golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/tools v0.0.0-20180911133044-677d2ff680c1 h1:dzEuQYa6+a3gROnSlgly5ERUm4SZKJt+dh+4iSbO+bI= +golang.org/x/tools v0.0.0-20180911133044-677d2ff680c1/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= diff --git a/lint/failure.go b/lint/failure.go index 63ffa4e..479b0cb 100644 --- a/lint/failure.go +++ b/lint/failure.go @@ -29,7 +29,6 @@ type Failure struct { Position FailurePosition Node ast.Node `json:"-"` Confidence float64 - URL string // For future use ReplacementLine string } diff --git a/lint/file.go b/lint/file.go index ee50dee..c829edb 100644 --- a/lint/file.go +++ b/lint/file.go @@ -56,6 +56,11 @@ func (f *File) Render(x interface{}) string { return buf.String() } +// CommentMap builds a comment map for the file. +func (f *File) CommentMap() ast.CommentMap { + return ast.NewCommentMap(f.Pkg.fset, f.AST, f.AST.Comments) +} + var basicTypeKinds = map[types.BasicKind]string{ types.UntypedBool: "bool", types.UntypedInt: "int", diff --git a/lint/package.go b/lint/package.go index 4dbaa03..7b6046f 100644 --- a/lint/package.go +++ b/lint/package.go @@ -77,7 +77,9 @@ func (p *Package) TypeCheck() error { anyFile = f astFiles = append(astFiles, f.AST) } - typesPkg, err := config.Check(anyFile.AST.Name.Name, p.fset, astFiles, info) + + typesPkg, err := check(config, anyFile.AST.Name.Name, p.fset, astFiles, info) + // Remember the typechecking info, even if config.Check failed, // since we will get partial information. p.TypesPkg = typesPkg @@ -86,6 +88,20 @@ func (p *Package) TypeCheck() error { return err } +// check function encapsulates the call to go/types.Config.Check method and +// recovers if the called method panics (see issue #59) +func check(config *types.Config, n string, fset *token.FileSet, astFiles []*ast.File, info *types.Info) (p *types.Package, err error) { + defer func() { + if r := recover(); r != nil { + err, _ = r.(error) + p = nil + return + } + }() + + return config.Check(n, fset, astFiles, info) +} + // TypeOf returns the type of an expression. func (p *Package) TypeOf(expr ast.Expr) types.Type { if p.TypesInfo == nil { diff --git a/lint/utils.go b/lint/utils.go index c9e83e5..28657c6 100644 --- a/lint/utils.go +++ b/lint/utils.go @@ -6,7 +6,7 @@ import ( ) // Name returns a different name if it should be different. -func Name(name string) (should string) { +func Name(name string, whitelist, blacklist []string) (should string) { // Fast path for simple cases: "_" and all lowercase. if name == "_" { return name @@ -56,7 +56,17 @@ func Name(name string) (should string) { // [w,i) is a word. word := string(runes[w:i]) - if u := strings.ToUpper(word); commonInitialisms[u] { + ignoreInitWarnings := map[string]bool{} + for _, i := range whitelist { + ignoreInitWarnings[i] = true + } + + extraInits := map[string]bool{} + for _, i := range blacklist { + extraInits[i] = true + } + + if u := strings.ToUpper(word); (commonInitialisms[u] || extraInits[u]) && !ignoreInitWarnings[u] { // Keep consistent case, which is lowercase only at the start. if w == 0 && unicode.IsLower(runes[w]) { u = strings.ToLower(u) diff --git a/rule/add-constant.go b/rule/add-constant.go new file mode 100644 index 0000000..881bbd0 --- /dev/null +++ b/rule/add-constant.go @@ -0,0 +1,151 @@ +package rule + +import ( + "fmt" + "github.com/mgechev/revive/lint" + "go/ast" + "strconv" + "strings" +) + +const ( + defaultStrLitLimit = 2 + kindFLOAT = "FLOAT" + kindINT = "INT" + kindSTRING = "STRING" +) + +type whiteList map[string]map[string]bool + +func newWhiteList() whiteList { + return map[string]map[string]bool{kindINT: map[string]bool{}, kindFLOAT: map[string]bool{}, kindSTRING: map[string]bool{}} +} + +func (wl whiteList) add(kind string, list string) { + elems := strings.Split(list, ",") + for _, e := range elems { + wl[kind][e] = true + } +} + +// AddConstantRule lints unused params in functions. +type AddConstantRule struct{} + +// Apply applies the rule to given file. +func (r *AddConstantRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + strLitLimit := defaultStrLitLimit + var whiteList = newWhiteList() + if len(arguments) > 0 { + args, ok := arguments[0].(map[string]interface{}) + if !ok { + panic(fmt.Sprintf("Invalid argument to the add-constant rule. Expecting a k,v map, got %T", arguments[0])) + } + for k, v := range args { + kind := "" + switch k { + case "allowFloats": + kind = kindFLOAT + fallthrough + case "allowInts": + if kind == "" { + kind = kindINT + } + fallthrough + case "allowStrs": + if kind == "" { + kind = kindSTRING + } + list, ok := v.(string) + if !ok { + panic(fmt.Sprintf("Invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v)) + } + whiteList.add(kind, list) + case "maxLitCount": + sl, ok := v.(string) + if !ok { + panic(fmt.Sprintf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v' (%T)", v, v)) + } + + limit, err := strconv.Atoi(sl) + if err != nil { + panic(fmt.Sprintf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v'", v)) + } + strLitLimit = limit + } + } + } + + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintAddConstantRule{onFailure: onFailure, strLits: make(map[string]int, 0), strLitLimit: strLitLimit, whiteLst: whiteList} + + ast.Walk(w, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *AddConstantRule) Name() string { + return "add-constant" +} + +type lintAddConstantRule struct { + onFailure func(lint.Failure) + strLits map[string]int + strLitLimit int + whiteLst whiteList +} + +func (w lintAddConstantRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.GenDecl: + return nil // skip declarations + case *ast.BasicLit: + switch kind := n.Kind.String(); kind { + case kindFLOAT, kindINT: + w.checkNumLit(kind, n) + case kindSTRING: + w.checkStrLit(n) + } + } + + return w + +} + +func (w lintAddConstantRule) checkStrLit(n *ast.BasicLit) { + if w.whiteLst[kindSTRING][n.Value] { + return + } + + count := w.strLits[n.Value] + if count >= 0 { + w.strLits[n.Value] = count + 1 + if w.strLits[n.Value] > w.strLitLimit { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: n, + Category: "style", + Failure: fmt.Sprintf("string literal %s appears, at least, %d times, create a named constant for it", n.Value, w.strLits[n.Value]), + }) + w.strLits[n.Value] = -1 // mark it to avoid failing again on the same literal + } + } +} + +func (w lintAddConstantRule) checkNumLit(kind string, n *ast.BasicLit) { + if w.whiteLst[kind][n.Value] { + return + } + + w.onFailure(lint.Failure{ + Confidence: 1, + Node: n, + Category: "style", + Failure: fmt.Sprintf("avoid magic numbers like '%s', create a named constant for it", n.Value), + }) +} diff --git a/rule/atomic.go b/rule/atomic.go new file mode 100644 index 0000000..572e141 --- /dev/null +++ b/rule/atomic.go @@ -0,0 +1,94 @@ +package rule + +import ( + "go/ast" + "go/token" + "go/types" + + "github.com/mgechev/revive/lint" +) + +// AtomicRule lints given else constructs. +type AtomicRule struct{} + +// Apply applies the rule to given file. +func (r *AtomicRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + walker := atomic{ + pkgTypesInfo: file.Pkg.TypesInfo, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(walker, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *AtomicRule) Name() string { + return "atomic" +} + +type atomic struct { + pkgTypesInfo *types.Info + onFailure func(lint.Failure) +} + +func (w atomic) Visit(node ast.Node) ast.Visitor { + n, ok := node.(*ast.AssignStmt) + if !ok { + return w + } + + if len(n.Lhs) != len(n.Rhs) { + return nil // skip assignment sub-tree + } + if len(n.Lhs) == 1 && n.Tok == token.DEFINE { + return nil // skip assignment sub-tree + } + + for i, right := range n.Rhs { + call, ok := right.(*ast.CallExpr) + if !ok { + continue + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + continue + } + pkgIdent, _ := sel.X.(*ast.Ident) + if w.pkgTypesInfo != nil { + pkgName, ok := w.pkgTypesInfo.Uses[pkgIdent].(*types.PkgName) + if !ok || pkgName.Imported().Path() != "sync/atomic" { + continue + } + } + + switch sel.Sel.Name { + case "AddInt32", "AddInt64", "AddUint32", "AddUint64", "AddUintptr": + left := n.Lhs[i] + if len(call.Args) != 2 { + continue + } + arg := call.Args[0] + broken := false + + if uarg, ok := arg.(*ast.UnaryExpr); ok && uarg.Op == token.AND { + broken = gofmt(left) == gofmt(uarg.X) + } else if star, ok := left.(*ast.StarExpr); ok { + broken = gofmt(star.X) == gofmt(arg) + } + + if broken { + w.onFailure(lint.Failure{ + Confidence: 1, + Failure: "direct assignment to atomic value", + Node: n, + }) + } + } + } + return w +} diff --git a/rule/blank-imports.go b/rule/blank-imports.go index bd3ed1b..0a00e37 100644 --- a/rule/blank-imports.go +++ b/rule/blank-imports.go @@ -10,7 +10,7 @@ import ( type BlankImportsRule struct{} // Apply applies the rule to given file. -func (r *BlankImportsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *BlankImportsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST @@ -38,7 +38,7 @@ type lintBlankImports struct { onFailure func(lint.Failure) } -func (w lintBlankImports) Visit(n ast.Node) ast.Visitor { +func (w lintBlankImports) Visit(_ ast.Node) ast.Visitor { // In package main and in tests, we don't complain about blank imports. if w.file.Pkg.IsMain() || w.file.IsTest() { return nil @@ -67,7 +67,6 @@ func (w lintBlankImports) Visit(n ast.Node) ast.Visitor { Failure: "a blank import should be only in a main or test package, or have a comment justifying it", Confidence: 1, Category: "imports", - URL: "", }) } } diff --git a/rule/bool-literal-in-expr.go b/rule/bool-literal-in-expr.go new file mode 100644 index 0000000..0a4e696 --- /dev/null +++ b/rule/bool-literal-in-expr.go @@ -0,0 +1,73 @@ +package rule + +import ( + "go/ast" + "go/token" + + "github.com/mgechev/revive/lint" +) + +// BoolLiteralRule warns when logic expressions contains Boolean literals. +type BoolLiteralRule struct{} + +// Apply applies the rule to given file. +func (r *BoolLiteralRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + astFile := file.AST + w := &lintBoolLiteral{astFile, onFailure} + ast.Walk(w, astFile) + + return failures +} + +// Name returns the rule name. +func (r *BoolLiteralRule) Name() string { + return "bool-literal-in-expr" +} + +type lintBoolLiteral struct { + file *ast.File + onFailure func(lint.Failure) +} + +func (w *lintBoolLiteral) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.BinaryExpr: + if !isBoolOp(n.Op) { + return w + } + + lexeme, ok := isExprABooleanLit(n.X) + if !ok { + lexeme, ok = isExprABooleanLit(n.Y) + + if !ok { + return w + } + } + + isConstant := (n.Op == token.LAND && lexeme == "false") || (n.Op == token.LOR && lexeme == "true") + + if isConstant { + w.addFailure(n, "Boolean expression seems to always evaluate to "+lexeme, "logic") + } else { + w.addFailure(n, "omit Boolean literal in expression", "style") + } + } + + return w +} + +func (w lintBoolLiteral) addFailure(node ast.Node, msg string, cat string) { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: node, + Category: cat, + Failure: msg, + }) +} diff --git a/rule/confusing-naming.go b/rule/confusing-naming.go new file mode 100644 index 0000000..143bb18 --- /dev/null +++ b/rule/confusing-naming.go @@ -0,0 +1,190 @@ +package rule + +import ( + "fmt" + "go/ast" + + "strings" + "sync" + + "github.com/mgechev/revive/lint" +) + +type referenceMethod struct { + fileName string + id *ast.Ident +} + +type pkgMethods struct { + pkg *lint.Package + methods map[string]map[string]*referenceMethod + mu *sync.Mutex +} + +type packages struct { + pkgs []pkgMethods + mu sync.Mutex +} + +func (ps *packages) methodNames(lp *lint.Package) pkgMethods { + ps.mu.Lock() + + for _, pkg := range ps.pkgs { + if pkg.pkg == lp { + ps.mu.Unlock() + return pkg + } + } + + pkgm := pkgMethods{pkg: lp, methods: make(map[string]map[string]*referenceMethod), mu: &sync.Mutex{}} + ps.pkgs = append(ps.pkgs, pkgm) + + ps.mu.Unlock() + return pkgm +} + +var allPkgs = packages{pkgs: make([]pkgMethods, 1)} + +// ConfusingNamingRule lints method names that differ only by capitalization +type ConfusingNamingRule struct{} + +// Apply applies the rule to given file. +func (r *ConfusingNamingRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + fileAst := file.AST + pkgm := allPkgs.methodNames(file.Pkg) + walker := lintConfusingNames{ + fileName: file.Name, + pkgm: pkgm, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(&walker, fileAst) + + return failures +} + +// Name returns the rule name. +func (r *ConfusingNamingRule) Name() string { + return "confusing-naming" +} + +//checkMethodName checks if a given method/function name is similar (just case differences) to other method/function of the same struct/file. +func checkMethodName(holder string, id *ast.Ident, w *lintConfusingNames) { + if id.Name == "init" && holder == defaultStructName { + // ignore init functions + return + } + + pkgm := w.pkgm + name := strings.ToUpper(id.Name) + + pkgm.mu.Lock() + defer pkgm.mu.Unlock() + + if pkgm.methods[holder] != nil { + if pkgm.methods[holder][name] != nil { + refMethod := pkgm.methods[holder][name] + // confusing names + var kind string + if holder == defaultStructName { + kind = "function" + } else { + kind = "method" + } + var fileName string + if w.fileName == refMethod.fileName { + fileName = "the same source file" + } else { + fileName = refMethod.fileName + } + w.onFailure(lint.Failure{ + Failure: fmt.Sprintf("Method '%s' differs only by capitalization to %s '%s' in %s", id.Name, kind, refMethod.id.Name, fileName), + Confidence: 1, + Node: id, + Category: "naming", + }) + + return + } + } else { + pkgm.methods[holder] = make(map[string]*referenceMethod, 1) + } + + // update the black list + if pkgm.methods[holder] == nil { + println("no entry for '", holder, "'") + } + pkgm.methods[holder][name] = &referenceMethod{fileName: w.fileName, id: id} +} + +type lintConfusingNames struct { + fileName string + pkgm pkgMethods + onFailure func(lint.Failure) +} + +const defaultStructName = "_" // used to map functions + +//getStructName of a function receiver. Defaults to defaultStructName +func getStructName(r *ast.FieldList) string { + result := defaultStructName + + if r == nil || len(r.List) < 1 { + return result + } + + t := r.List[0].Type + + if p, _ := t.(*ast.StarExpr); p != nil { // if a pointer receiver => dereference pointer receiver types + t = p.X + } + + if p, _ := t.(*ast.Ident); p != nil { + result = p.Name + } + + return result +} + +func checkStructFields(fields *ast.FieldList, structName string, w *lintConfusingNames) { + bl := make(map[string]bool, len(fields.List)) + for _, f := range fields.List { + for _, id := range f.Names { + normName := strings.ToUpper(id.Name) + if bl[normName] { + w.onFailure(lint.Failure{ + Failure: fmt.Sprintf("Field '%s' differs only by capitalization to other field in the struct type %s", id.Name, structName), + Confidence: 1, + Node: id, + Category: "naming", + }) + } else { + bl[normName] = true + } + } + } +} + +func (w *lintConfusingNames) Visit(n ast.Node) ast.Visitor { + switch v := n.(type) { + case *ast.FuncDecl: + // Exclude naming warnings for functions that are exported to C but + // not exported in the Go API. + // See https://github.com/golang/lint/issues/144. + if ast.IsExported(v.Name.Name) || !isCgoExported(v) { + checkMethodName(getStructName(v.Recv), v.Name, w) + } + case *ast.TypeSpec: + if s, ok := v.Type.(*ast.StructType); ok { + checkStructFields(s.Fields, v.Name.Name, w) + } + + default: + // will add other checks like field names, struct names, etc. + } + + return w +} diff --git a/rule/confusing-results.go b/rule/confusing-results.go new file mode 100644 index 0000000..1d386b3 --- /dev/null +++ b/rule/confusing-results.go @@ -0,0 +1,67 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// ConfusingResultsRule lints given function declarations +type ConfusingResultsRule struct{} + +// Apply applies the rule to given file. +func (r *ConfusingResultsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + fileAst := file.AST + walker := lintConfusingResults{ + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(walker, fileAst) + + return failures +} + +// Name returns the rule name. +func (r *ConfusingResultsRule) Name() string { + return "confusing-results" +} + +type lintConfusingResults struct { + onFailure func(lint.Failure) +} + +func (w lintConfusingResults) Visit(n ast.Node) ast.Visitor { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Type.Results == nil || len(fn.Type.Results.List) < 2 { + return w + } + lastType := "" + for _, result := range fn.Type.Results.List { + if len(result.Names) > 0 { + return w + } + + t, ok := result.Type.(*ast.Ident) + if !ok { + return w + } + + if t.Name == lastType { + w.onFailure(lint.Failure{ + Node: n, + Confidence: 1, + Category: "naming", + Failure: "unnamed results of the same type may be confusing, consider using named results", + }) + break + } + lastType = t.Name + + } + + return w +} diff --git a/rule/constant-logical-expr.go b/rule/constant-logical-expr.go new file mode 100644 index 0000000..6a91561 --- /dev/null +++ b/rule/constant-logical-expr.go @@ -0,0 +1,88 @@ +package rule + +import ( + "github.com/mgechev/revive/lint" + "go/ast" + "go/token" +) + +// ConstantLogicalExprRule warns on constant logical expressions. +type ConstantLogicalExprRule struct{} + +// Apply applies the rule to given file. +func (r *ConstantLogicalExprRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + astFile := file.AST + w := &lintConstantLogicalExpr{astFile, onFailure} + ast.Walk(w, astFile) + return failures +} + +// Name returns the rule name. +func (r *ConstantLogicalExprRule) Name() string { + return "constant-logical-expr" +} + +type lintConstantLogicalExpr struct { + file *ast.File + onFailure func(lint.Failure) +} + +func (w *lintConstantLogicalExpr) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.BinaryExpr: + if !w.isOperatorWithLogicalResult(n.Op) { + return w + } + + if gofmt(n.X) != gofmt(n.Y) { // check if subexpressions are the same + return w + } + + if n.Op == token.EQL { + w.newFailure(n, "expression always evaluates to true") + return w + } + + if w.isInequalityOperator(n.Op) { + w.newFailure(n, "expression always evaluates to false") + return w + } + + w.newFailure(n, "left and right hand-side sub-expressions are the same") + } + + return w +} + +func (w *lintConstantLogicalExpr) isOperatorWithLogicalResult(t token.Token) bool { + switch t { + case token.LAND, token.LOR, token.EQL, token.LSS, token.GTR, token.NEQ, token.LEQ, token.GEQ: + return true + } + + return false +} + +func (w *lintConstantLogicalExpr) isInequalityOperator(t token.Token) bool { + switch t { + case token.LSS, token.GTR, token.NEQ, token.LEQ, token.GEQ: + return true + } + + return false +} + +func (w lintConstantLogicalExpr) newFailure(node ast.Node, msg string) { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: node, + Category: "logic", + Failure: msg, + }) +} diff --git a/rule/context-as-argument.go b/rule/context-as-argument.go index 1e04afe..0ed28a8 100644 --- a/rule/context-as-argument.go +++ b/rule/context-as-argument.go @@ -10,7 +10,7 @@ import ( type ContextAsArgumentRule struct{} // Apply applies the rule to given file. -func (r *ContextAsArgumentRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ContextAsArgumentRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST @@ -50,7 +50,6 @@ func (w lintContextArguments) Visit(n ast.Node) ast.Visitor { w.onFailure(lint.Failure{ Node: fn, Category: "arg-order", - URL: "https://golang.org/pkg/context/", Failure: "context.Context should be the first parameter of a function", Confidence: 0.9, }) diff --git a/rule/context-keys-type.go b/rule/context-keys-type.go index 240a691..9c2f0bb 100644 --- a/rule/context-keys-type.go +++ b/rule/context-keys-type.go @@ -12,7 +12,7 @@ import ( type ContextKeysType struct{} // Apply applies the rule to given file. -func (r *ContextKeysType) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ContextKeysType) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST diff --git a/rule/cyclomatic.go b/rule/cyclomatic.go index b7e9c0f..48ea80a 100644 --- a/rule/cyclomatic.go +++ b/rule/cyclomatic.go @@ -47,7 +47,7 @@ type lintCyclomatic struct { onFailure func(lint.Failure) } -func (w lintCyclomatic) Visit(n ast.Node) ast.Visitor { +func (w lintCyclomatic) Visit(_ ast.Node) ast.Visitor { f := w.file for _, decl := range f.AST.Decls { if fn, ok := decl.(*ast.FuncDecl); ok { diff --git a/rule/deep-exit.go b/rule/deep-exit.go index 1dbe933..c286b8d 100644 --- a/rule/deep-exit.go +++ b/rule/deep-exit.go @@ -11,7 +11,7 @@ import ( type DeepExitRule struct{} // Apply applies the rule to given file. -func (r *DeepExitRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *DeepExitRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { failures = append(failures, failure) @@ -81,7 +81,6 @@ func (w lintDeepExit) Visit(node ast.Node) ast.Visitor { Confidence: 1, Node: ce, Category: "bad practice", - URL: "#deep-exit", Failure: fmt.Sprintf("calls to %s.%s only in main() or init() functions", pkg, fn), }) } diff --git a/rule/dot-imports.go b/rule/dot-imports.go index bd625c5..78419d7 100644 --- a/rule/dot-imports.go +++ b/rule/dot-imports.go @@ -10,7 +10,7 @@ import ( type DotImportsRule struct{} // Apply applies the rule to given file. -func (r *DotImportsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *DotImportsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST @@ -38,7 +38,7 @@ type lintImports struct { onFailure func(lint.Failure) } -func (w lintImports) Visit(n ast.Node) ast.Visitor { +func (w lintImports) Visit(_ ast.Node) ast.Visitor { for i, is := range w.fileAst.Imports { _ = i if is.Name != nil && is.Name.Name == "." && !w.file.IsTest() { @@ -47,7 +47,6 @@ func (w lintImports) Visit(n ast.Node) ast.Visitor { Failure: "should not use dot imports", Node: is, Category: "imports", - URL: "#import-dot", }) } } diff --git a/rule/empty-block.go b/rule/empty-block.go index eed573f..7861394 100644 --- a/rule/empty-block.go +++ b/rule/empty-block.go @@ -10,7 +10,7 @@ import ( type EmptyBlockRule struct{} // Apply applies the rule to given file. -func (r *EmptyBlockRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *EmptyBlockRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { @@ -59,7 +59,6 @@ func (w lintEmptyBlock) Visit(node ast.Node) ast.Visitor { Confidence: 1, Node: block, Category: "logic", - URL: "#empty-block", Failure: "this block is empty, you can remove it", }) } diff --git a/rule/empty-lines.go b/rule/empty-lines.go new file mode 100644 index 0000000..61d9281 --- /dev/null +++ b/rule/empty-lines.go @@ -0,0 +1,113 @@ +package rule + +import ( + "go/ast" + "go/token" + + "github.com/mgechev/revive/lint" +) + +// EmptyLinesRule lints empty lines in blocks. +type EmptyLinesRule struct{} + +// Apply applies the rule to given file. +func (r *EmptyLinesRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintEmptyLines{file, file.CommentMap(), onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *EmptyLinesRule) Name() string { + return "empty-lines" +} + +type lintEmptyLines struct { + file *lint.File + cmap ast.CommentMap + onFailure func(lint.Failure) +} + +func (w lintEmptyLines) Visit(node ast.Node) ast.Visitor { + block, ok := node.(*ast.BlockStmt) + if !ok { + return w + } + + w.checkStart(block) + w.checkEnd(block) + + return w +} + +func (w lintEmptyLines) checkStart(block *ast.BlockStmt) { + if len(block.List) == 0 { + return + } + + start := w.position(block.Lbrace) + firstNode := block.List[0] + + if w.commentBetween(start, firstNode) { + return + } + + first := w.position(firstNode.Pos()) + if first.Line-start.Line > 1 { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: block, + Category: "style", + Failure: "extra empty line at the start of a block", + }) + } +} + +func (w lintEmptyLines) checkEnd(block *ast.BlockStmt) { + if len(block.List) < 1 { + return + } + + end := w.position(block.Rbrace) + lastNode := block.List[len(block.List)-1] + + if w.commentBetween(end, lastNode) { + return + } + + last := w.position(lastNode.End()) + if end.Line-last.Line > 1 { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: lastNode, + Category: "style", + Failure: "extra empty line at the end of a block", + }) + } +} + +func (w lintEmptyLines) commentBetween(position token.Position, node ast.Node) bool { + comments := w.cmap.Filter(node).Comments() + if len(comments) == 0 { + return false + } + + for _, comment := range comments { + start, end := w.position(comment.Pos()), w.position(comment.End()) + if start.Line-position.Line == 1 || position.Line-end.Line == 1 { + return true + } + } + + return false +} + +func (w lintEmptyLines) position(pos token.Pos) token.Position { + return w.file.ToPosition(pos) +} diff --git a/rule/error-naming.go b/rule/error-naming.go index 851b566..3a10806 100644 --- a/rule/error-naming.go +++ b/rule/error-naming.go @@ -13,7 +13,7 @@ import ( type ErrorNamingRule struct{} // Apply applies the rule to given file. -func (r *ErrorNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ErrorNamingRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST @@ -41,7 +41,7 @@ type lintErrors struct { onFailure func(lint.Failure) } -func (w lintErrors) Visit(n ast.Node) ast.Visitor { +func (w lintErrors) Visit(_ ast.Node) ast.Visitor { for _, decl := range w.fileAst.Decls { gd, ok := decl.(*ast.GenDecl) if !ok || gd.Tok != token.VAR { diff --git a/rule/error-return.go b/rule/error-return.go index 47b9dd3..df847b3 100644 --- a/rule/error-return.go +++ b/rule/error-return.go @@ -10,7 +10,7 @@ import ( type ErrorReturnRule struct{} // Apply applies the rule to given file. -func (r *ErrorReturnRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ErrorReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST diff --git a/rule/error-strings.go b/rule/error-strings.go index 95cf39a..b8a5b7e 100644 --- a/rule/error-strings.go +++ b/rule/error-strings.go @@ -14,7 +14,7 @@ import ( type ErrorStringsRule struct{} // Apply applies the rule to given file. -func (r *ErrorStringsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ErrorStringsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST @@ -69,7 +69,6 @@ func (w lintErrorStrings) Visit(n ast.Node) ast.Visitor { w.onFailure(lint.Failure{ Node: str, Confidence: conf, - URL: "#error-strings", Category: "errors", Failure: "error strings should not be capitalized or end with punctuation or a newline", }) diff --git a/rule/errorf.go b/rule/errorf.go index 4068022..1bffbab 100644 --- a/rule/errorf.go +++ b/rule/errorf.go @@ -13,7 +13,7 @@ import ( type ErrorfRule struct{} // Apply applies the rule to given file. -func (r *ErrorfRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ErrorfRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST diff --git a/rule/exported.go b/rule/exported.go index bb95599..714acf6 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -15,7 +15,7 @@ import ( type ExportedRule struct{} // Apply applies the rule to given file. -func (r *ExportedRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ExportedRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure if isTest(file) { @@ -80,7 +80,6 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { w.onFailure(lint.Failure{ Node: fn, Confidence: 1, - URL: "#doc-comments", Category: "comments", Failure: fmt.Sprintf("exported %s %s should have comment or be unexported", kind, name), }) @@ -92,7 +91,6 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { w.onFailure(lint.Failure{ Node: fn.Doc, Confidence: 0.8, - URL: "#doc-comments", Category: "comments", Failure: fmt.Sprintf(`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix), }) @@ -123,7 +121,6 @@ func (w *lintExported) checkStutter(id *ast.Ident, thing string) { w.onFailure(lint.Failure{ Node: id, Confidence: 0.8, - URL: "#package-names", Category: "naming", Failure: fmt.Sprintf("%s name will be used as %s.%s by other packages, and that stutters; consider calling this %s", thing, pkg, name, rem), }) @@ -138,7 +135,6 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { w.onFailure(lint.Failure{ Node: t, Confidence: 1, - URL: "#doc-comments", Category: "comments", Failure: fmt.Sprintf("exported type %v should have comment or be unexported", t.Name), }) @@ -146,8 +142,11 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { } s := doc.Text() - articles := [...]string{"A", "An", "The"} + articles := [...]string{"A", "An", "The", "This"} for _, a := range articles { + if t.Name.Name == a { + continue + } if strings.HasPrefix(s, a+" ") { s = s[len(a)+1:] break @@ -157,7 +156,6 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { w.onFailure(lint.Failure{ Node: doc, Confidence: 1, - URL: "#doc-comments", Category: "comments", Failure: fmt.Sprintf(`comment on exported type %v should be of the form "%v ..." (with optional leading article)`, t.Name, t.Name), }) @@ -202,7 +200,6 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD w.onFailure(lint.Failure{ Confidence: 1, Node: vs, - URL: "#doc-comments", Category: "comments", Failure: fmt.Sprintf("exported %s %s should have comment%s or be unexported", kind, name, block), }) @@ -224,7 +221,6 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD w.onFailure(lint.Failure{ Confidence: 1, Node: doc, - URL: "#doc-comments", Category: "comments", Failure: fmt.Sprintf(`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix), }) diff --git a/rule/file-header.go b/rule/file-header.go index 70135db..9b4fbc3 100644 --- a/rule/file-header.go +++ b/rule/file-header.go @@ -51,7 +51,7 @@ type lintFileHeader struct { onFailure func(lint.Failure) } -func (w lintFileHeader) Visit(n ast.Node) ast.Visitor { +func (w lintFileHeader) Visit(_ ast.Node) ast.Visitor { g := w.fileAst.Comments[0] failure := lint.Failure{ Node: w.fileAst, diff --git a/rule/flag-param.go b/rule/flag-param.go new file mode 100644 index 0000000..6cb6dae --- /dev/null +++ b/rule/flag-param.go @@ -0,0 +1,104 @@ +package rule + +import ( + "fmt" + "github.com/mgechev/revive/lint" + "go/ast" +) + +// FlagParamRule lints given else constructs. +type FlagParamRule struct{} + +// Apply applies the rule to given file. +func (r *FlagParamRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintFlagParamRule{onFailure: onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *FlagParamRule) Name() string { + return "flag-parameter" +} + +type lintFlagParamRule struct { + onFailure func(lint.Failure) +} + +func (w lintFlagParamRule) Visit(node ast.Node) ast.Visitor { + fd, ok := node.(*ast.FuncDecl) + if !ok { + return w + } + + if fd.Body == nil { + return nil // skip whole function declaration + } + + for _, p := range fd.Type.Params.List { + t := p.Type + + id, ok := t.(*ast.Ident) + if !ok { + continue + } + + if id.Name != "bool" { + continue + } + + cv := conditionVisitor{p.Names, fd, w} + ast.Walk(cv, fd.Body) + } + + return w +} + +type conditionVisitor struct { + ids []*ast.Ident + fd *ast.FuncDecl + linter lintFlagParamRule +} + +func (w conditionVisitor) Visit(node ast.Node) ast.Visitor { + ifStmt, ok := node.(*ast.IfStmt) + if !ok { + return w + } + + fselect := func(n ast.Node) bool { + ident, ok := n.(*ast.Ident) + if !ok { + return false + } + + for _, id := range w.ids { + if ident.Name == id.Name { + return true + } + } + + return false + } + + uses := pick(ifStmt.Cond, fselect, nil) + + if len(uses) < 1 { + return w + } + + w.linter.onFailure(lint.Failure{ + Confidence: 1, + Node: w.fd.Type.Params, + Category: "bad practice", + Failure: fmt.Sprintf("parameter '%s' seems to be a control flag, avoid control coupling", uses[0]), + }) + + return nil +} diff --git a/rule/function-result-limit.go b/rule/function-result-limit.go new file mode 100644 index 0000000..1850fc4 --- /dev/null +++ b/rule/function-result-limit.go @@ -0,0 +1,68 @@ +package rule + +import ( + "fmt" + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// FunctionResultsLimitRule lints given else constructs. +type FunctionResultsLimitRule struct{} + +// Apply applies the rule to given file. +func (r *FunctionResultsLimitRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + if len(arguments) != 1 { + panic(`invalid configuration for "function-result-limit"`) + } + + max, ok := arguments[0].(int64) // Alt. non panicking version + if !ok { + panic(fmt.Sprintf(`invalid value passed as return results number to the "function-result-limit" rule; need int64 but got %T`, arguments[0])) + } + if max < 0 { + panic(`the value passed as return results number to the "function-result-limit" rule cannot be negative`) + } + + var failures []lint.Failure + + walker := lintFunctionResultsNum{ + max: int(max), + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(walker, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *FunctionResultsLimitRule) Name() string { + return "function-result-limit" +} + +type lintFunctionResultsNum struct { + max int + onFailure func(lint.Failure) +} + +func (w lintFunctionResultsNum) Visit(n ast.Node) ast.Visitor { + node, ok := n.(*ast.FuncDecl) + if ok { + num := 0 + if node.Type.Results != nil { + num = node.Type.Results.NumFields() + } + if num > w.max { + w.onFailure(lint.Failure{ + Confidence: 1, + Failure: fmt.Sprintf("maximum number of return results per function exceeded; max %d but got %d", w.max, num), + Node: node.Type, + }) + return w + } + } + return w +} diff --git a/rule/get-return.go b/rule/get-return.go index ed2ee61..494ab66 100644 --- a/rule/get-return.go +++ b/rule/get-return.go @@ -12,7 +12,7 @@ import ( type GetReturnRule struct{} // Apply applies the rule to given file. -func (r *GetReturnRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *GetReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { @@ -62,7 +62,6 @@ func (w lintReturnRule) Visit(node ast.Node) ast.Visitor { Confidence: 0.8, Node: fd, Category: "logic", - URL: "#get-return", Failure: fmt.Sprintf("function '%s' seems to be a getter but it does not return any result", fd.Name.Name), }) } diff --git a/rule/if-return.go b/rule/if-return.go index e53dd0a..c275d27 100644 --- a/rule/if-return.go +++ b/rule/if-return.go @@ -12,7 +12,7 @@ import ( type IfReturnRule struct{} // Apply applies the rule to given file. -func (r *IfReturnRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *IfReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { diff --git a/rule/imports-blacklist.go b/rule/imports-blacklist.go new file mode 100644 index 0000000..3732083 --- /dev/null +++ b/rule/imports-blacklist.go @@ -0,0 +1,69 @@ +package rule + +import ( + "fmt" + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// ImportsBlacklistRule lints given else constructs. +type ImportsBlacklistRule struct{} + +// Apply applies the rule to given file. +func (r *ImportsBlacklistRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + blacklist := make(map[string]bool, len(arguments)) + + for _, arg := range arguments { + argStr, ok := arg.(string) + if !ok { + panic(fmt.Sprintf("Invalid argument to the imports-blacklist rule. Expecting a string, got %T", arg)) + } + // we add quotes if nt present, because when parsed, the value of the AST node, will be quoted + if len(argStr) > 2 && argStr[0] != '"' && argStr[len(argStr)-1] != '"' { + argStr = fmt.Sprintf(`"%s"`, argStr) + } + blacklist[argStr] = true + } + + fileAst := file.AST + walker := blacklistedImports{ + file: file, + fileAst: fileAst, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + blacklist: blacklist, + } + + ast.Walk(walker, fileAst) + + return failures +} + +// Name returns the rule name. +func (r *ImportsBlacklistRule) Name() string { + return "imports-blacklist" +} + +type blacklistedImports struct { + file *lint.File + fileAst *ast.File + onFailure func(lint.Failure) + blacklist map[string]bool +} + +func (w blacklistedImports) Visit(_ ast.Node) ast.Visitor { + for _, is := range w.fileAst.Imports { + if is.Path != nil && !w.file.IsTest() && w.blacklist[is.Path.Value] { + w.onFailure(lint.Failure{ + Confidence: 1, + Failure: fmt.Sprintf("should not use the following blacklisted import: %s", is.Path.Value), + Node: is, + Category: "imports", + }) + } + } + return nil +} diff --git a/rule/increment-decrement.go b/rule/increment-decrement.go index d15ab18..5d6b176 100644 --- a/rule/increment-decrement.go +++ b/rule/increment-decrement.go @@ -12,7 +12,7 @@ import ( type IncrementDecrementRule struct{} // Apply applies the rule to given file. -func (r *IncrementDecrementRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *IncrementDecrementRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST diff --git a/rule/indent-error-flow.go b/rule/indent-error-flow.go index c4621ea..4c9799b 100644 --- a/rule/indent-error-flow.go +++ b/rule/indent-error-flow.go @@ -11,7 +11,7 @@ import ( type IndentErrorFlowRule struct{} // Apply applies the rule to given file. -func (r *IndentErrorFlowRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *IndentErrorFlowRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { @@ -71,10 +71,8 @@ func (w lintElse) Visit(node ast.Node) ast.Visitor { Confidence: 1, Node: ifStmt.Else, Category: "indent", - URL: "#indent-error-flow", Failure: "if block ends with a return statement, so drop this else and outdent its block" + extra, }) } return w } - diff --git a/rule/line-length-limit.go b/rule/line-length-limit.go new file mode 100644 index 0000000..5ee0570 --- /dev/null +++ b/rule/line-length-limit.go @@ -0,0 +1,84 @@ +package rule + +import ( + "bufio" + "bytes" + "fmt" + "go/token" + "strings" + "unicode/utf8" + + "github.com/mgechev/revive/lint" +) + +// LineLengthLimitRule lints given else constructs. +type LineLengthLimitRule struct{} + +// Apply applies the rule to given file. +func (r *LineLengthLimitRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + if len(arguments) != 1 { + panic(`invalid configuration for "line-length-limit"`) + } + + max, ok := arguments[0].(int64) // Alt. non panicking version + if !ok || max < 0 { + panic(`invalid value passed as argument number to the "line-length-limit" rule`) + } + + var failures []lint.Failure + checker := lintLineLengthNum{ + max: int(max), + file: file, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + checker.check() + + return failures +} + +// Name returns the rule name. +func (r *LineLengthLimitRule) Name() string { + return "line-length-limit" +} + +type lintLineLengthNum struct { + max int + file *lint.File + onFailure func(lint.Failure) +} + +func (r lintLineLengthNum) check() { + f := bytes.NewReader(r.file.Content()) + spaces := strings.Repeat(" ", 4) // tab width = 4 + l := 1 + s := bufio.NewScanner(f) + for s.Scan() { + t := s.Text() + t = strings.Replace(t, "\t", spaces, -1) + c := utf8.RuneCountInString(t) + if c > r.max { + r.onFailure(lint.Failure{ + Category: "code-style", + Position: lint.FailurePosition{ + // Offset not set; it is non-trivial, and doesn't appear to be needed. + Start: token.Position{ + Filename: r.file.Name, + Line: l, + Column: 0, + }, + End: token.Position{ + Filename: r.file.Name, + Line: l, + Column: c, + }, + }, + Confidence: 1, + Failure: fmt.Sprintf("line is %d characters, out of limit %d", c, r.max), + }) + } + l++ + } +} diff --git a/rule/max-public-structs.go b/rule/max-public-structs.go index 638ac28..9a2d07c 100644 --- a/rule/max-public-structs.go +++ b/rule/max-public-structs.go @@ -1,7 +1,6 @@ package rule import ( - "fmt" "go/ast" "strings" @@ -31,7 +30,6 @@ func (r *MaxPublicStructsRule) Apply(file *lint.File, arguments lint.Arguments) panic(`invalid value passed as argument number to the "max-public-structs" rule`) } - fmt.Println("Name:", max, walker.current) if walker.current > max { walker.onFailure(lint.Failure{ Failure: "you have exceeded the maximum number of public struct declarations", diff --git a/rule/modifies-param.go b/rule/modifies-param.go index f849fad..e9f1b44 100644 --- a/rule/modifies-param.go +++ b/rule/modifies-param.go @@ -11,7 +11,7 @@ import ( type ModifiesParamRule struct{} // Apply applies the rule to given file. -func (r *ModifiesParamRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ModifiesParamRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { @@ -43,23 +43,6 @@ func retrieveParamNames(pl []*ast.Field) map[string]bool { return result } -func checkAssign(lhs []ast.Expr, w lintModifiesParamRule) { - for _, e := range lhs { - id, ok := e.(*ast.Ident) - if ok { - if w.params[id.Name] { - w.onFailure(lint.Failure{ - Confidence: 0.5, - Node: id, - Category: "bad practice", - URL: "#modifies-parameter", - Failure: fmt.Sprintf("parameter '%s' seems to be a modified", id.Name), - }) - } - } - } -} - func (w lintModifiesParamRule) Visit(node ast.Node) ast.Visitor { switch v := node.(type) { case *ast.FuncDecl: @@ -87,7 +70,6 @@ func checkParam(id *ast.Ident, w *lintModifiesParamRule) { Confidence: 0.5, // confidence is low because of shadow variables Node: id, Category: "bad practice", - URL: "#modifies-parameter", Failure: fmt.Sprintf("parameter '%s' seems to be modified", id), }) } diff --git a/rule/modifies-value-receiver.go b/rule/modifies-value-receiver.go new file mode 100644 index 0000000..4fe22dd --- /dev/null +++ b/rule/modifies-value-receiver.go @@ -0,0 +1,134 @@ +package rule + +import ( + "go/ast" + "strings" + + "github.com/mgechev/revive/lint" +) + +// ModifiesValRecRule lints assignments to value method-receivers. +type ModifiesValRecRule struct{} + +// Apply applies the rule to given file. +func (r *ModifiesValRecRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintModifiesValRecRule{file: file, onFailure: onFailure} + file.Pkg.TypeCheck() + ast.Walk(w, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *ModifiesValRecRule) Name() string { + return "modifies-value-receiver" +} + +type lintModifiesValRecRule struct { + file *lint.File + onFailure func(lint.Failure) +} + +func (w lintModifiesValRecRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.FuncDecl: + if n.Recv == nil { + return nil // skip, not a method + } + + receiver := n.Recv.List[0] + if _, ok := receiver.Type.(*ast.StarExpr); ok { + return nil // skip, method with pointer receiver + } + + if w.skipType(receiver.Type) { + return nil // skip, receiver is a map or array + } + + if len(receiver.Names) < 1 { + return nil // skip, anonymous receiver + } + + receiverName := receiver.Names[0].Name + if receiverName == "_" { + return nil // skip, anonymous receiver + } + + fselect := func(n ast.Node) bool { + // look for assignments with the receiver in the right hand + asgmt, ok := n.(*ast.AssignStmt) + if !ok { + return false + } + + for _, exp := range asgmt.Lhs { + switch e := exp.(type) { + case *ast.IndexExpr: // receiver...[] = ... + continue + case *ast.StarExpr: // *receiver = ... + continue + case *ast.SelectorExpr: // receiver.field = ... + name := w.getNameFromExpr(e.X) + if name == "" || name != receiverName { + continue + } + + if w.skipType(ast.Expr(e.Sel)) { + continue + } + + case *ast.Ident: // receiver := ... + if e.Name != receiverName { + continue + } + default: + continue + } + + return true + } + + return false + } + + assignmentsToReceiver := pick(n.Body, fselect, nil) + + for _, assignment := range assignmentsToReceiver { + w.onFailure(lint.Failure{ + Node: assignment, + Confidence: 1, + Failure: "suspicious assignment to a by-value method receiver", + }) + } + } + + return w +} + +func (w lintModifiesValRecRule) skipType(t ast.Expr) bool { + rt := w.file.Pkg.TypeOf(t) + if rt == nil { + return false + } + + rt = rt.Underlying() + rtName := rt.String() + + // skip when receiver is a map or array + return strings.HasPrefix(rtName, "[]") || strings.HasPrefix(rtName, "map[") +} + +func (lintModifiesValRecRule) getNameFromExpr(ie ast.Expr) string { + ident, ok := ie.(*ast.Ident) + if !ok { + return "" + } + + return ident.Name +} diff --git a/rule/package-comments.go b/rule/package-comments.go index 5a473dd..00fc5bb 100644 --- a/rule/package-comments.go +++ b/rule/package-comments.go @@ -17,7 +17,7 @@ import ( type PackageCommentsRule struct{} // Apply applies the rule to given file. -func (r *PackageCommentsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *PackageCommentsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure if isTest(file) { @@ -45,7 +45,7 @@ type lintPackageComments struct { onFailure func(lint.Failure) } -func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { +func (l *lintPackageComments) Visit(_ ast.Node) ast.Visitor { if l.file.IsTest() { return nil } @@ -84,7 +84,6 @@ func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { }, Confidence: 0.9, Failure: "package comment is detached; there should be no blank lines between it and the package statement", - URL: ref, }) return nil } @@ -96,7 +95,6 @@ func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { Node: l.fileAst, Confidence: 0.2, Failure: "should have a package comment, unless it's in another file for this package", - URL: ref, }) return nil } @@ -107,7 +105,6 @@ func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { Node: l.fileAst.Doc, Confidence: 1, Failure: "package comment should not have leading space", - URL: ref, }) s = ts } @@ -118,7 +115,6 @@ func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { Node: l.fileAst.Doc, Confidence: 1, Failure: fmt.Sprintf(`package comment should be of the form "%s..."`, prefix), - URL: ref, }) } return nil diff --git a/rule/range-val-in-closure.go b/rule/range-val-in-closure.go new file mode 100644 index 0000000..857787b --- /dev/null +++ b/rule/range-val-in-closure.go @@ -0,0 +1,111 @@ +package rule + +import ( + "fmt" + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// RangeValInClosureRule lints given else constructs. +type RangeValInClosureRule struct{} + +// Apply applies the rule to given file. +func (r *RangeValInClosureRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + walker := rangeValInClosure{ + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(walker, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *RangeValInClosureRule) Name() string { + return "range-val-in-closure" +} + +type rangeValInClosure struct { + onFailure func(lint.Failure) +} + +func (w rangeValInClosure) Visit(node ast.Node) ast.Visitor { + + // Find the variables updated by the loop statement. + var vars []*ast.Ident + addVar := func(expr ast.Expr) { + if id, ok := expr.(*ast.Ident); ok { + vars = append(vars, id) + } + } + var body *ast.BlockStmt + switch n := node.(type) { + case *ast.RangeStmt: + body = n.Body + addVar(n.Key) + addVar(n.Value) + case *ast.ForStmt: + body = n.Body + switch post := n.Post.(type) { + case *ast.AssignStmt: + // e.g. for p = head; p != nil; p = p.next + for _, lhs := range post.Lhs { + addVar(lhs) + } + case *ast.IncDecStmt: + // e.g. for i := 0; i < n; i++ + addVar(post.X) + } + } + if vars == nil { + return w + } + + // Inspect a go or defer statement + // if it's the last one in the loop body. + // (We give up if there are following statements, + // because it's hard to prove go isn't followed by wait, + // or defer by return.) + if len(body.List) == 0 { + return w + } + var last *ast.CallExpr + switch s := body.List[len(body.List)-1].(type) { + case *ast.GoStmt: + last = s.Call + case *ast.DeferStmt: + last = s.Call + default: + return w + } + lit, ok := last.Fun.(*ast.FuncLit) + if !ok { + return w + } + if lit.Type == nil { + // Not referring to a variable (e.g. struct field name) + return w + } + ast.Inspect(lit.Body, func(n ast.Node) bool { + id, ok := n.(*ast.Ident) + if !ok || id.Obj == nil { + return true + } + for _, v := range vars { + if v.Obj == id.Obj { + w.onFailure(lint.Failure{ + Confidence: 1, + Failure: fmt.Sprintf("loop variable %v captured by func literal", id.Name), + Node: n, + }) + } + } + return true + }) + return w +} diff --git a/rule/range.go b/rule/range.go index a1b490b..d18492c 100644 --- a/rule/range.go +++ b/rule/range.go @@ -12,7 +12,7 @@ import ( type RangeRule struct{} // Apply applies the rule to given file. -func (r *RangeRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *RangeRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { diff --git a/rule/receiver-naming.go b/rule/receiver-naming.go index ff5cf27..589d5f0 100644 --- a/rule/receiver-naming.go +++ b/rule/receiver-naming.go @@ -11,7 +11,7 @@ import ( type ReceiverNamingRule struct{} // Apply applies the rule to given file. -func (r *ReceiverNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ReceiverNamingRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST @@ -52,7 +52,6 @@ func (w lintReceiverName) Visit(n ast.Node) ast.Visitor { w.onFailure(lint.Failure{ Node: n, Confidence: 1, - URL: ref, Category: "naming", Failure: "receiver name should not be an underscore, omit the name if it is unused", }) @@ -62,7 +61,6 @@ func (w lintReceiverName) Visit(n ast.Node) ast.Visitor { w.onFailure(lint.Failure{ Node: n, Confidence: 1, - URL: ref, Category: "naming", Failure: `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`, }) @@ -73,7 +71,6 @@ func (w lintReceiverName) Visit(n ast.Node) ast.Visitor { w.onFailure(lint.Failure{ Node: n, Confidence: 1, - URL: ref, Category: "naming", Failure: fmt.Sprintf("receiver name %s should be consistent with previous receiver name %s for %s", name, prev, recv), }) diff --git a/rule/redefines-builtin-id.go b/rule/redefines-builtin-id.go new file mode 100644 index 0000000..947b8aa --- /dev/null +++ b/rule/redefines-builtin-id.go @@ -0,0 +1,145 @@ +package rule + +import ( + "fmt" + "github.com/mgechev/revive/lint" + "go/ast" + "go/token" +) + +// RedefinesBuiltinIDRule warns when a builtin identifier is shadowed. +type RedefinesBuiltinIDRule struct{} + +// Apply applies the rule to given file. +func (r *RedefinesBuiltinIDRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + var builtInConstAndVars = map[string]bool{ + "true": true, + "false": true, + "iota": true, + "nil": true, + } + + var builtFunctions = map[string]bool{ + "append": true, + "cap": true, + "close": true, + "complex": true, + "copy": true, + "delete": true, + "imag": true, + "len": true, + "make": true, + "new": true, + "panic": true, + "print": true, + "println": true, + "real": true, + "recover": true, + } + + var builtInTypes = map[string]bool{ + "ComplexType": true, + "FloatType": true, + "IntegerType": true, + "Type": true, + "Type1": true, + "bool": true, + "byte": true, + "complex128": true, + "complex64": true, + "error": true, + "float32": true, + "float64": true, + "int": true, + "int16": true, + "int32": true, + "int64": true, + "int8": true, + "rune": true, + "string": true, + "uint": true, + "uint16": true, + "uint32": true, + "uint64": true, + "uint8": true, + "uintptr": true, + } + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + astFile := file.AST + w := &lintRedefinesBuiltinID{builtInConstAndVars, builtFunctions, builtInTypes, onFailure} + ast.Walk(w, astFile) + + return failures +} + +// Name returns the rule name. +func (r *RedefinesBuiltinIDRule) Name() string { + return "redefines-builtin-id" +} + +type lintRedefinesBuiltinID struct { + constsAndVars map[string]bool + funcs map[string]bool + types map[string]bool + onFailure func(lint.Failure) +} + +func (w *lintRedefinesBuiltinID) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.GenDecl: + if n.Tok != token.TYPE { + return nil // skip if not type declaration + } + typeSpec, ok := n.Specs[0].(*ast.TypeSpec) + if !ok { + return nil + } + id := typeSpec.Name.Name + if w.types[id] { + w.addFailure(n, fmt.Sprintf("redefinition of the built-in type %s", id)) + } + case *ast.FuncDecl: + if n.Recv != nil { + return w // skip methods + } + + id := n.Name.Name + if w.funcs[id] { + w.addFailure(n, fmt.Sprintf("redefinition of the built-in function %s", id)) + } + case *ast.AssignStmt: + for _, e := range n.Lhs { + id, ok := e.(*ast.Ident) + if !ok { + continue + } + + if w.constsAndVars[id.Name] { + var msg string + if n.Tok == token.DEFINE { + msg = fmt.Sprintf("assignment creates a shadow of built-in identifier %s", id.Name) + } else { + msg = fmt.Sprintf("assignment modifies built-in identifier %s", id.Name) + } + w.addFailure(n, msg) + } + } + } + + return w +} + +func (w lintRedefinesBuiltinID) addFailure(node ast.Node, msg string) { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: node, + Category: "logic", + Failure: msg, + }) +} diff --git a/rule/struct-tag.go b/rule/struct-tag.go new file mode 100644 index 0000000..2ed1410 --- /dev/null +++ b/rule/struct-tag.go @@ -0,0 +1,226 @@ +package rule + +import ( + "fmt" + "github.com/fatih/structtag" + "github.com/mgechev/revive/lint" + "go/ast" + "strconv" + "strings" +) + +// StructTagRule lints struct tags. +type StructTagRule struct{} + +// Apply applies the rule to given file. +func (r *StructTagRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintStructTagRule{onFailure: onFailure} + + ast.Walk(w, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *StructTagRule) Name() string { + return "struct-tag" +} + +type lintStructTagRule struct { + onFailure func(lint.Failure) + usedTagNbr map[string]bool // list of used tag numbers +} + +func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.StructType: + if n.Fields == nil || n.Fields.NumFields() < 1 { + return nil // skip empty structs + } + w.usedTagNbr = map[string]bool{} // init + for _, f := range n.Fields.List { + if f.Tag != nil { + w.checkTaggedField(f) + } + } + } + + return w + +} + +// checkTaggedField checks the tag of the given field. +// precondition: the field has a tag +func (w lintStructTagRule) checkTaggedField(f *ast.Field) { + tags, err := structtag.Parse(strings.Trim(f.Tag.Value, "`")) + if err != nil || tags == nil { + w.addFailure(f.Tag, "malformed tag") + return + } + + for _, tag := range tags.Tags() { + switch key := tag.Key; key { + case "asn1": + msg, ok := w.checkASN1Tag(f.Type, tag) + if !ok { + w.addFailure(f.Tag, msg) + } + case "bson": + msg, ok := w.checkBSONTag(tag.Options) + if !ok { + w.addFailure(f.Tag, msg) + } + case "default": + if !w.typeValueMatch(f.Type, tag.Name) { + w.addFailure(f.Tag, "field's type and default value's type mismatch") + } + case "json": + msg, ok := w.checkJSONTag(tag.Options) + if !ok { + w.addFailure(f.Tag, msg) + } + case "protobuf": + // Not implemented yet + case "required": + if tag.Name != "true" && tag.Name != "false" { + w.addFailure(f.Tag, "required should be 'true' or 'false'") + } + case "xml": + msg, ok := w.checkXMLTag(tag.Options) + if !ok { + w.addFailure(f.Tag, msg) + } + case "yaml": + msg, ok := w.checkYAMLTag(tag.Options) + if !ok { + w.addFailure(f.Tag, msg) + } + default: + // unknown key + } + } +} + +func (w lintStructTagRule) checkASN1Tag(t ast.Expr, tag *structtag.Tag) (string, bool) { + checkList := append(tag.Options, tag.Name) + for _, opt := range checkList { + switch opt { + case "application", "explicit", "generalized", "ia5", "omitempty", "optional", "set", "utf8": + + default: + if strings.HasPrefix(opt, "tag:") { + parts := strings.Split(opt, ":") + tagNumber := parts[1] + if w.usedTagNbr[tagNumber] { + return fmt.Sprintf("duplicated tag number %s", tagNumber), false + } + w.usedTagNbr[tagNumber] = true + + continue + } + + if strings.HasPrefix(opt, "default:") { + parts := strings.Split(opt, ":") + if len(parts) < 2 { + return "malformed default for ASN1 tag", false + } + if !w.typeValueMatch(t, parts[1]) { + return "field's type and default value's type mismatch", false + } + + continue + } + + return fmt.Sprintf("unknown option '%s' in ASN1 tag", opt), false + } + } + + return "", true +} + +func (w lintStructTagRule) checkBSONTag(options []string) (string, bool) { + for _, opt := range options { + switch opt { + case "inline", "minsize", "omitempty": + default: + return fmt.Sprintf("unknown option '%s' in BSON tag", opt), false + } + } + + return "", true +} + +func (w lintStructTagRule) checkJSONTag(options []string) (string, bool) { + for _, opt := range options { + switch opt { + case "omitempty", "string": + default: + return fmt.Sprintf("unknown option '%s' in JSON tag", opt), false + } + } + + return "", true +} + +func (w lintStructTagRule) checkXMLTag(options []string) (string, bool) { + for _, opt := range options { + switch opt { + case "any", "attr", "cdata", "chardata", "comment", "innerxml", "omitempty", "typeattr": + default: + return fmt.Sprintf("unknown option '%s' in XML tag", opt), false + } + } + + return "", true +} + +func (w lintStructTagRule) checkYAMLTag(options []string) (string, bool) { + for _, opt := range options { + switch opt { + case "flow", "inline", "omitempty": + default: + return fmt.Sprintf("unknown option '%s' in YAML tag", opt), false + } + } + + return "", true +} + +func (w lintStructTagRule) typeValueMatch(t ast.Expr, val string) bool { + tID, ok := t.(*ast.Ident) + if !ok { + return true + } + + typeMatches := true + switch tID.Name { + case "bool": + typeMatches = val == "true" || val == "false" + case "float64": + _, err := strconv.ParseFloat(val, 64) + typeMatches = err == nil + case "int": + _, err := strconv.ParseInt(val, 10, 64) + typeMatches = err == nil + case "string": + case "nil": + default: + // unchecked type + } + + return typeMatches +} + +func (w lintStructTagRule) addFailure(n ast.Node, msg string) { + w.onFailure(lint.Failure{ + Node: n, + Failure: msg, + Confidence: 1, + }) +} diff --git a/rule/superfluous-else.go b/rule/superfluous-else.go index 21f0749..c29be9e 100644 --- a/rule/superfluous-else.go +++ b/rule/superfluous-else.go @@ -12,7 +12,7 @@ import ( type SuperfluousElseRule struct{} // Apply applies the rule to given file. -func (r *SuperfluousElseRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *SuperfluousElseRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { failures = append(failures, failure) @@ -109,7 +109,6 @@ func newFailure(node ast.Node, msg string) lint.Failure { Confidence: 1, Node: node, Category: "indent", - URL: "#indent-error-flow", Failure: msg, } } diff --git a/rule/time-naming.go b/rule/time-naming.go index 18dfaf1..a93f4b5 100644 --- a/rule/time-naming.go +++ b/rule/time-naming.go @@ -13,7 +13,7 @@ import ( type TimeNamingRule struct{} // Apply applies the rule to given file. -func (r *TimeNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *TimeNamingRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { @@ -50,7 +50,7 @@ func (w *lintTimeNames) Visit(node ast.Node) ast.Visitor { if pt, ok := typ.(*types.Pointer); ok { typ = pt.Elem() } - if !isNamedType(w.file.Pkg, typ, "time", "Duration") { + if !isNamedType(typ, "time", "Duration") { continue } suffix := "" @@ -83,7 +83,7 @@ var timeSuffixes = []string{ "MS", "Ms", } -func isNamedType(p *lint.Package, typ types.Type, importPath, name string) bool { +func isNamedType(typ types.Type, importPath, name string) bool { n, ok := typ.(*types.Named) if !ok { return false diff --git a/rule/unexported-return.go b/rule/unexported-return.go index 27ca4c4..c9c8a41 100644 --- a/rule/unexported-return.go +++ b/rule/unexported-return.go @@ -12,7 +12,7 @@ import ( type UnexportedReturnRule struct{} // Apply applies the rule to given file. -func (r *UnexportedReturnRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *UnexportedReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST diff --git a/rule/unnecessary-stmt.go b/rule/unnecessary-stmt.go new file mode 100644 index 0000000..732d8a8 --- /dev/null +++ b/rule/unnecessary-stmt.go @@ -0,0 +1,107 @@ +package rule + +import ( + "go/ast" + "go/token" + + "github.com/mgechev/revive/lint" +) + +// UnnecessaryStmtRule warns on unnecessary statements. +type UnnecessaryStmtRule struct{} + +// Apply applies the rule to given file. +func (r *UnnecessaryStmtRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintUnnecessaryStmtRule{onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *UnnecessaryStmtRule) Name() string { + return "unnecessary-stmt" +} + +type lintUnnecessaryStmtRule struct { + onFailure func(lint.Failure) +} + +func (w lintUnnecessaryStmtRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.FuncDecl: + if n.Body == nil || n.Type.Results != nil { + return w + } + stmts := n.Body.List + if len(stmts) == 0 { + return w + } + + lastStmt := stmts[len(stmts)-1] + rs, ok := lastStmt.(*ast.ReturnStmt) + if !ok { + return w + } + + if len(rs.Results) == 0 { + w.newFailure(lastStmt, "omit unnecessary return statement") + } + + case *ast.SwitchStmt: + w.checkSwitchBody(n.Body) + case *ast.TypeSwitchStmt: + w.checkSwitchBody(n.Body) + case *ast.CaseClause: + if n.Body == nil { + return w + } + stmts := n.Body + if len(stmts) == 0 { + return w + } + + lastStmt := stmts[len(stmts)-1] + rs, ok := lastStmt.(*ast.BranchStmt) + if !ok { + return w + } + + if rs.Tok == token.BREAK && rs.Label == nil { + w.newFailure(lastStmt, "omit unnecessary break at the end of case clause") + } + } + + return w +} + +func (w lintUnnecessaryStmtRule) checkSwitchBody(b *ast.BlockStmt) { + cases := b.List + if len(cases) != 1 { + return + } + + cc, ok := cases[0].(*ast.CaseClause) + if !ok { + return + } + + if len(cc.List) > 1 { // skip cases with multiple expressions + return + } + + w.newFailure(b, "switch with only one case can be replaced by an if-then") +} + +func (w lintUnnecessaryStmtRule) newFailure(node ast.Node, msg string) { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: node, + Category: "style", + Failure: msg, + }) +} diff --git a/rule/unreachable-code.go b/rule/unreachable-code.go new file mode 100644 index 0000000..c81e9e7 --- /dev/null +++ b/rule/unreachable-code.go @@ -0,0 +1,114 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// UnreachableCodeRule lints unreachable code. +type UnreachableCodeRule struct{} + +// Apply applies the rule to given file. +func (r *UnreachableCodeRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + var branchingFunctions = map[string]map[string]bool{ + "os": map[string]bool{"Exit": true}, + "log": map[string]bool{ + "Fatal": true, + "Fatalf": true, + "Fatalln": true, + "Panic": true, + "Panicf": true, + "Panicln": true, + }, + } + + w := lintUnreachableCode{onFailure, branchingFunctions} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *UnreachableCodeRule) Name() string { + return "unreachable-code" +} + +type lintUnreachableCode struct { + onFailure func(lint.Failure) + branchingFunctions map[string]map[string]bool +} + +func (w lintUnreachableCode) Visit(node ast.Node) ast.Visitor { + blk, ok := node.(*ast.BlockStmt) + if !ok { + return w + } + + if len(blk.List) < 2 { + return w + } +loop: + for i, stmt := range blk.List[:len(blk.List)-1] { + // println("iterating ", len(blk.List)) + next := blk.List[i+1] + if _, ok := next.(*ast.LabeledStmt); ok { + continue // skip if next statement is labeled + } + + switch s := stmt.(type) { + case *ast.ReturnStmt: + w.onFailure(newUnreachableCodeFailure(s)) + break loop + case *ast.BranchStmt: + token := s.Tok.String() + if token != "fallthrough" { + w.onFailure(newUnreachableCodeFailure(s)) + break loop + } + case *ast.ExprStmt: + ce, ok := s.X.(*ast.CallExpr) + if !ok { + continue + } + // it's a function call + fc, ok := ce.Fun.(*ast.SelectorExpr) + if !ok { + continue + } + + id, ok := fc.X.(*ast.Ident) + + if !ok { + continue + } + fn := fc.Sel.Name + pkg := id.Name + if !w.branchingFunctions[pkg][fn] { // it isn't a call to a branching function + continue + } + + if _, ok := next.(*ast.ReturnStmt); ok { // return statement needed to satisfy function signature + continue + } + + w.onFailure(newUnreachableCodeFailure(s)) + break loop + } + } + + return w +} + +func newUnreachableCodeFailure(node ast.Node) lint.Failure { + return lint.Failure{ + Confidence: 1, + Node: node, + Category: "logic", + Failure: "unreachable code after this statement", + } +} diff --git a/rule/unused-param.go b/rule/unused-param.go new file mode 100644 index 0000000..f4de228 --- /dev/null +++ b/rule/unused-param.go @@ -0,0 +1,242 @@ +package rule + +import ( + "fmt" + "go/ast" + "go/token" + + "github.com/mgechev/revive/lint" +) + +// UnusedParamRule lints unused params in functions. +type UnusedParamRule struct{} + +// Apply applies the rule to given file. +func (r *UnusedParamRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintUnusedParamRule{onFailure: onFailure} + + ast.Walk(w, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *UnusedParamRule) Name() string { + return "unused-parameter" +} + +type lintUnusedParamRule struct { + onFailure func(lint.Failure) +} + +func (w lintUnusedParamRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.FuncDecl: + fv := newFuncVisitor(retrieveNamedParams(n.Type.Params.List)) + if n.Body != nil { + ast.Walk(fv, n.Body) + checkUnusedParams(w, fv.params, n) + } + return nil + } + + return w +} + +type scope struct { + vars map[string]bool +} + +func newScope() scope { + return scope{make(map[string]bool, 0)} +} + +func (s *scope) addVars(exps []ast.Expr) { + for _, e := range exps { + if id, ok := e.(*ast.Ident); ok { + s.vars[id.Name] = true + } + } +} + +type scopeStack struct { + stk []scope +} + +func (s *scopeStack) openScope() { + s.stk = append(s.stk, newScope()) +} + +func (s *scopeStack) closeScope() { + if len(s.stk) > 0 { + s.stk = s.stk[:len(s.stk)-1] + } +} + +func (s *scopeStack) currentScope() scope { + if len(s.stk) > 0 { + return s.stk[len(s.stk)-1] + } + + panic("no current scope") +} + +func newScopeStack() scopeStack { + return scopeStack{make([]scope, 0)} +} + +type funcVisitor struct { + sStk scopeStack + params map[string]bool +} + +func newFuncVisitor(params map[string]bool) funcVisitor { + return funcVisitor{sStk: newScopeStack(), params: params} +} + +func walkStmtList(v ast.Visitor, list []ast.Stmt) { + for _, s := range list { + ast.Walk(v, s) + } +} + +func (v funcVisitor) Visit(node ast.Node) ast.Visitor { + varSelector := func(n ast.Node) bool { + id, ok := n.(*ast.Ident) + return ok && id.Obj != nil && id.Obj.Kind.String() == "var" + } + switch n := node.(type) { + case *ast.BlockStmt: + v.sStk.openScope() + walkStmtList(v, n.List) + v.sStk.closeScope() + return nil + case *ast.AssignStmt: + var uses []ast.Node + if isOpAssign(n.Tok) { // Case of id += expr + uses = append(uses, pickFromExpList(n.Lhs, varSelector, nil)...) + } else { // Case of id[expr] = expr + indexSelector := func(n ast.Node) bool { + _, ok := n.(*ast.IndexExpr) + return ok + } + f := func(n ast.Node) []ast.Node { + ie, ok := n.(*ast.IndexExpr) + if !ok { // not possible + return nil + } + + return pick(ie.Index, varSelector, nil) + } + + uses = append(uses, pickFromExpList(n.Lhs, indexSelector, f)...) + } + + uses = append(uses, pickFromExpList(n.Rhs, varSelector, nil)...) + + markParamListAsUsed(uses, v) + cs := v.sStk.currentScope() + cs.addVars(n.Lhs) + case *ast.Ident: + if n.Obj != nil { + if n.Obj.Kind.String() == "var" { + markParamAsUsed(n, v) + } + } + case *ast.ForStmt: + v.sStk.openScope() + if n.Init != nil { + ast.Walk(v, n.Init) + } + uses := pickFromExpList([]ast.Expr{n.Cond}, varSelector, nil) + markParamListAsUsed(uses, v) + ast.Walk(v, n.Body) + v.sStk.closeScope() + return nil + case *ast.SwitchStmt: + v.sStk.openScope() + if n.Init != nil { + ast.Walk(v, n.Init) + } + uses := pickFromExpList([]ast.Expr{n.Tag}, varSelector, nil) + markParamListAsUsed(uses, v) + // Analyze cases (they are not BlockStmt but a list of Stmt) + cases := n.Body.List + for _, c := range cases { + cc, ok := c.(*ast.CaseClause) + if !ok { + continue + } + uses := pickFromExpList(cc.List, varSelector, nil) + markParamListAsUsed(uses, v) + v.sStk.openScope() + for _, stmt := range cc.Body { + ast.Walk(v, stmt) + } + v.sStk.closeScope() + } + + v.sStk.closeScope() + return nil + } + + return v +} + +func retrieveNamedParams(pl []*ast.Field) map[string]bool { + result := make(map[string]bool, len(pl)) + for _, p := range pl { + for _, n := range p.Names { + if n.Name != "_" { + result[n.Name] = true + } + } + } + return result +} + +func checkUnusedParams(w lintUnusedParamRule, params map[string]bool, n *ast.FuncDecl) { + for k, v := range params { + if v { + w.onFailure(lint.Failure{ + Confidence: 0.8, // confidence is not 1.0 because of shadow variables + Node: n, + Category: "bad practice", + Failure: fmt.Sprintf("parameter '%s' seems to be unused, consider removing or renaming it as _", k), + }) + } + } + +} + +func markParamListAsUsed(ids []ast.Node, v funcVisitor) { + for _, id := range ids { + markParamAsUsed(id.(*ast.Ident), v) + } +} + +func markParamAsUsed(id *ast.Ident, v funcVisitor) { // TODO: constraint parameters to receive just a list of params and a scope stack + for _, s := range v.sStk.stk { + if s.vars[id.Name] { + return + } + } + + if v.params[id.Name] { + v.params[id.Name] = false + } +} + +func isOpAssign(aTok token.Token) bool { + return aTok == token.ADD_ASSIGN || aTok == token.AND_ASSIGN || + aTok == token.MUL_ASSIGN || aTok == token.OR_ASSIGN || + aTok == token.QUO_ASSIGN || aTok == token.REM_ASSIGN || + aTok == token.SHL_ASSIGN || aTok == token.SHR_ASSIGN || + aTok == token.SUB_ASSIGN || aTok == token.XOR_ASSIGN +} diff --git a/rule/utils.go b/rule/utils.go index 08c4f0b..6ba542b 100644 --- a/rule/utils.go +++ b/rule/utils.go @@ -1,8 +1,10 @@ package rule import ( + "bytes" "fmt" "go/ast" + "go/printer" "go/token" "go/types" "regexp" @@ -61,47 +63,6 @@ func isCgoExported(f *ast.FuncDecl) bool { return false } -var commonInitialisms = map[string]bool{ - "ACL": true, - "API": true, - "ASCII": true, - "CPU": true, - "CSS": true, - "DNS": true, - "EOF": true, - "GUID": true, - "HTML": true, - "HTTP": true, - "HTTPS": true, - "ID": true, - "IP": true, - "JSON": true, - "LHS": true, - "QPS": true, - "RAM": true, - "RHS": true, - "RPC": true, - "SLA": true, - "SMTP": true, - "SQL": true, - "SSH": true, - "TCP": true, - "TLS": true, - "TTL": true, - "UDP": true, - "UI": true, - "UID": true, - "UUID": true, - "URI": true, - "URL": true, - "UTF8": true, - "VM": true, - "XML": true, - "XMPP": true, - "XSRF": true, - "XSS": true, -} - var allCapsRE = regexp.MustCompile(`^[A-Z0-9_]+$`) func isIdent(expr ast.Expr, ident string) bool { @@ -146,3 +107,85 @@ func srcLine(src []byte, p token.Position) string { } return string(src[lo:hi]) } + +// pick yields a list of nodes by picking them from a sub-ast with root node n. +// Nodes are selected by applying the fselect function +// f function is applied to each selected node before inseting it in the final result. +// If f==nil then it defaults to the identity function (ie it returns the node itself) +func pick(n ast.Node, fselect func(n ast.Node) bool, f func(n ast.Node) []ast.Node) []ast.Node { + var result []ast.Node + + if n == nil { + return result + } + + if f == nil { + f = func(n ast.Node) []ast.Node { return []ast.Node{n} } + } + + onSelect := func(n ast.Node) { + result = append(result, f(n)...) + } + p := picker{fselect: fselect, onSelect: onSelect} + ast.Walk(p, n) + return result +} + +func pickFromExpList(l []ast.Expr, fselect func(n ast.Node) bool, f func(n ast.Node) []ast.Node) []ast.Node { + result := make([]ast.Node, 0) + for _, e := range l { + result = append(result, pick(e, fselect, f)...) + } + return result +} + +type picker struct { + fselect func(n ast.Node) bool + onSelect func(n ast.Node) +} + +func (p picker) Visit(node ast.Node) ast.Visitor { + if p.fselect == nil { + return nil + } + + if p.fselect(node) { + p.onSelect(node) + } + + return p +} + +// isBoolOp returns true if the given token corresponds to +// a bool operator +func isBoolOp(t token.Token) bool { + switch t { + case token.LAND, token.LOR, token.EQL, token.NEQ: + return true + } + + return false +} + +const ( + trueName = "true" + falseName = "false" +) + +func isExprABooleanLit(n ast.Node) (lexeme string, ok bool) { + oper, ok := n.(*ast.Ident) + + if !ok { + return "", false + } + + return oper.Name, (oper.Name == trueName || oper.Name == falseName) +} + +// gofmt returns a string representation of the expression. +func gofmt(x ast.Expr) string { + buf := bytes.Buffer{} + fs := token.NewFileSet() + printer.Fprint(&buf, fs, x) + return buf.String() +} diff --git a/rule/var-declarations.go b/rule/var-declarations.go index c8e0993..4411321 100644 --- a/rule/var-declarations.go +++ b/rule/var-declarations.go @@ -13,7 +13,7 @@ import ( type VarDeclarationsRule struct{} // Apply applies the rule to given file. -func (r *VarDeclarationsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *VarDeclarationsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure fileAst := file.AST diff --git a/rule/var-naming.go b/rule/var-naming.go index bd190db..768f65b 100644 --- a/rule/var-naming.go +++ b/rule/var-naming.go @@ -16,10 +16,23 @@ type VarNamingRule struct{} func (r *VarNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { var failures []lint.Failure + var whitelist []string + var blacklist []string + + if len(arguments) >= 1 { + whitelist = getList(arguments[0], "whitelist") + } + + if len(arguments) >= 2 { + blacklist = getList(arguments[1], "blacklist") + } + fileAst := file.AST walker := lintNames{ - file: file, - fileAst: fileAst, + file: file, + fileAst: fileAst, + whitelist: whitelist, + blacklist: blacklist, onFailure: func(failure lint.Failure) { failures = append(failures, failure) }, @@ -32,7 +45,6 @@ func (r *VarNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint. Confidence: 1, Node: walker.fileAst, Category: "naming", - URL: "http://golang.org/doc/effective_go.html#package-names", }) } @@ -72,7 +84,6 @@ func check(id *ast.Ident, thing string, w *lintNames) { Confidence: 0.8, Node: id, Category: "naming", - URL: "#mixed-caps", }) return } @@ -83,11 +94,10 @@ func check(id *ast.Ident, thing string, w *lintNames) { Confidence: 0.8, Node: id, Category: "naming", - URL: "#mixed-caps", }) } - should := lint.Name(id.Name) + should := lint.Name(id.Name, w.whitelist, w.blacklist) if id.Name == should { return } @@ -98,7 +108,6 @@ func check(id *ast.Ident, thing string, w *lintNames) { Confidence: 0.9, Node: id, Category: "naming", - URL: "http://golang.org/doc/effective_go.html#mixed-caps", }) return } @@ -107,7 +116,6 @@ func check(id *ast.Ident, thing string, w *lintNames) { Confidence: 0.8, Node: id, Category: "naming", - URL: "#initialisms", }) } @@ -117,6 +125,8 @@ type lintNames struct { lastGen *ast.GenDecl genDeclMissingComments map[*ast.GenDecl]bool onFailure func(lint.Failure) + whitelist []string + blacklist []string } func (w *lintNames) Visit(n ast.Node) ast.Visitor { @@ -202,3 +212,19 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { } return w } + +func getList(arg interface{}, argName string) []string { + temp, ok := arg.([]interface{}) + if !ok { + panic(fmt.Sprintf("Invalid argument to the var-naming rule. Expecting a %s of type slice with initialisms, got %T", argName, arg)) + } + var list []string + for _, v := range temp { + if val, ok := v.(string); ok { + list = append(list, val) + } else { + panic(fmt.Sprintf("Invalid %s values of the var-naming rule. Expecting slice of strings but got element of type %T", val, arg)) + } + } + return list +} diff --git a/rule/waitgroup-by-value.go b/rule/waitgroup-by-value.go new file mode 100644 index 0000000..b869291 --- /dev/null +++ b/rule/waitgroup-by-value.go @@ -0,0 +1,66 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// WaitGroupByValueRule lints sync.WaitGroup passed by copy in functions. +type WaitGroupByValueRule struct{} + +// Apply applies the rule to given file. +func (r *WaitGroupByValueRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintWaitGroupByValueRule{onFailure: onFailure} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *WaitGroupByValueRule) Name() string { + return "waitgroup-by-value" +} + +type lintWaitGroupByValueRule struct { + onFailure func(lint.Failure) +} + +func (w lintWaitGroupByValueRule) Visit(node ast.Node) ast.Visitor { + // look for function declarations + fd, ok := node.(*ast.FuncDecl) + if !ok { + return w + } + + // Check all function's parameters + for _, field := range fd.Type.Params.List { + if !w.isWaitGroup(field.Type) { + continue + } + + w.onFailure(lint.Failure{ + Confidence: 1, + Node: field, + Failure: "sync.WaitGroup passed by value, the function will get a copy of the original one", + }) + } + + return nil +} + +func (lintWaitGroupByValueRule) isWaitGroup(ft ast.Expr) bool { + se, ok := ft.(*ast.SelectorExpr) + if !ok { + return false + } + + x, _ := se.X.(*ast.Ident) + sel := se.Sel.Name + return x.Name == "sync" && sel == "WaitGroup" +} diff --git a/test/add-constant_test.go b/test/add-constant_test.go new file mode 100644 index 0000000..e4e4aec --- /dev/null +++ b/test/add-constant_test.go @@ -0,0 +1,21 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestAddConstant(t *testing.T) { + args := []interface{}{map[string]interface{}{ + "maxLitCount": "2", + "allowStrs": "\"\"", + "allowInts": "0,1,2", + "allowFloats": "0.0,1.0", + }} + + testRule(t, "add-constant", &rule.AddConstantRule{}, &lint.RuleConfig{ + Arguments: args, + }) +} diff --git a/test/atomic_test.go b/test/atomic_test.go new file mode 100644 index 0000000..1bf29bb --- /dev/null +++ b/test/atomic_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// Atomic rule. +func TestAtomic(t *testing.T) { + testRule(t, "atomic", &rule.AtomicRule{}) +} diff --git a/test/bool-literal-in-expr_test.go b/test/bool-literal-in-expr_test.go new file mode 100644 index 0000000..445ee93 --- /dev/null +++ b/test/bool-literal-in-expr_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// BoolLiteral rule. +func TestBoolLiteral(t *testing.T) { + testRule(t, "bool-literal-in-expr", &rule.BoolLiteralRule{}) +} diff --git a/test/confusing-naming_test.go b/test/confusing-naming_test.go new file mode 100644 index 0000000..42f57fe --- /dev/null +++ b/test/confusing-naming_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// TestConfusingNaming rule. +func TestConfusingNaming(t *testing.T) { + testRule(t, "confusing-naming1", &rule.ConfusingNamingRule{}) +} diff --git a/test/confusing-results_test.go b/test/confusing-results_test.go new file mode 100644 index 0000000..5a8f2c5 --- /dev/null +++ b/test/confusing-results_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestConfusingResults(t *testing.T) { + testRule(t, "confusing-results", &rule.ConfusingResultsRule{}) +} diff --git a/test/constant-logical-expr_test.go b/test/constant-logical-expr_test.go new file mode 100644 index 0000000..bb80ade --- /dev/null +++ b/test/constant-logical-expr_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// ConstantLogicalExpr rule. +func TestConstantLogicalExpr(t *testing.T) { + testRule(t, "constant-logical-expr", &rule.ConstantLogicalExprRule{}) +} diff --git a/test/empty-lines_test.go b/test/empty-lines_test.go new file mode 100644 index 0000000..ef27605 --- /dev/null +++ b/test/empty-lines_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// TestEmptyLines rule. +func TestEmptyLines(t *testing.T) { + testRule(t, "empty-lines", &rule.EmptyLinesRule{}) +} diff --git a/test/flag-param_test.go b/test/flag-param_test.go new file mode 100644 index 0000000..7042173 --- /dev/null +++ b/test/flag-param_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestFlagParam(t *testing.T) { + testRule(t, "flag-param", &rule.FlagParamRule{}) +} diff --git a/test/function-result-limit_test.go b/test/function-result-limit_test.go new file mode 100644 index 0000000..d7addfd --- /dev/null +++ b/test/function-result-limit_test.go @@ -0,0 +1,14 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestFunctionResultsLimit(t *testing.T) { + testRule(t, "function-result-limit", &rule.FunctionResultsLimitRule{}, &lint.RuleConfig{ + Arguments: []interface{}{int64(3)}, + }) +} diff --git a/test/import-blacklist_test.go b/test/import-blacklist_test.go new file mode 100644 index 0000000..6132b7b --- /dev/null +++ b/test/import-blacklist_test.go @@ -0,0 +1,16 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestImportsBlacklist(t *testing.T) { + args := []interface{}{"crypto/md5", "crypto/sha1"} + + testRule(t, "imports-blacklist", &rule.ImportsBlacklistRule{}, &lint.RuleConfig{ + Arguments: args, + }) +} diff --git a/test/line-length-limit_test.go b/test/line-length-limit_test.go new file mode 100644 index 0000000..24807db --- /dev/null +++ b/test/line-length-limit_test.go @@ -0,0 +1,14 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestLineLengthLimit(t *testing.T) { + testRule(t, "line-length-limit", &rule.LineLengthLimitRule{}, &lint.RuleConfig{ + Arguments: []interface{}{int64(100)}, + }) +} diff --git a/test/modifies-value-receiver_test.go b/test/modifies-value-receiver_test.go new file mode 100644 index 0000000..b16f535 --- /dev/null +++ b/test/modifies-value-receiver_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestModifiesValRec(t *testing.T) { + testRule(t, "modifies-value-receiver", &rule.ModifiesValRecRule{}) +} diff --git a/test/range-val-in-closure_test.go b/test/range-val-in-closure_test.go new file mode 100644 index 0000000..f446ce1 --- /dev/null +++ b/test/range-val-in-closure_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestRangeValInClosure(t *testing.T) { + testRule(t, "range-val-in-closure", &rule.RangeValInClosureRule{}, &lint.RuleConfig{}) +} diff --git a/test/redefines-builtin-id_test.go b/test/redefines-builtin-id_test.go new file mode 100644 index 0000000..d7fac96 --- /dev/null +++ b/test/redefines-builtin-id_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// Tests RedefinesBuiltinID rule. +func TestRedefinesBuiltinID(t *testing.T) { + testRule(t, "redefines-builtin-id", &rule.RedefinesBuiltinIDRule{}) +} diff --git a/test/struct-tag_test.go b/test/struct-tag_test.go new file mode 100644 index 0000000..d8b2a0a --- /dev/null +++ b/test/struct-tag_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// TestStructTag tests struct-tag rule +func TestStructTag(t *testing.T) { + testRule(t, "struct-tag", &rule.StructTagRule{}) +} diff --git a/test/unnecessary-stmt_test.go b/test/unnecessary-stmt_test.go new file mode 100644 index 0000000..577a8aa --- /dev/null +++ b/test/unnecessary-stmt_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +// TestUnnecessaryStmt rule. +func TestUnnecessaryStmt(t *testing.T) { + testRule(t, "unnecessary-stmt", &rule.UnnecessaryStmtRule{}) +} diff --git a/test/unreachable-code_test.go b/test/unreachable-code_test.go new file mode 100644 index 0000000..e8c3de9 --- /dev/null +++ b/test/unreachable-code_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestUnreachableCode(t *testing.T) { + testRule(t, "unreachable-code", &rule.UnreachableCodeRule{}) +} diff --git a/test/unused-param_test.go b/test/unused-param_test.go new file mode 100644 index 0000000..c8b706b --- /dev/null +++ b/test/unused-param_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestUnusedParam(t *testing.T) { + testRule(t, "unused-param", &rule.UnusedParamRule{}) +} diff --git a/test/utils.go b/test/utils.go index 58fb7df..7d76268 100644 --- a/test/utils.go +++ b/test/utils.go @@ -35,13 +35,13 @@ func testRule(t *testing.T, filename string, rule lint.Rule, config ...*lint.Rul c[rule.Name()] = *config[0] } if parseInstructions(t, filename, src) == nil { - assertSuccess(t, baseDir, stat, src, []lint.Rule{rule}, c) + assertSuccess(t, baseDir, stat, []lint.Rule{rule}, c) return } assertFailures(t, baseDir, stat, src, []lint.Rule{rule}, c) } -func assertSuccess(t *testing.T, baseDir string, fi os.FileInfo, src []byte, rules []lint.Rule, config map[string]lint.RuleConfig) error { +func assertSuccess(t *testing.T, baseDir string, fi os.FileInfo, rules []lint.Rule, config map[string]lint.RuleConfig) error { l := lint.New(func(file string) ([]byte, error) { return ioutil.ReadFile(baseDir + file) }) @@ -220,7 +220,8 @@ func srcLine(src []byte, p token.Position) string { return string(src[lo:hi]) } -func TestLine(t *testing.T) { +// TestLine tests srcLine function +func TestLine(t *testing.T) { //revive:disable-line:exported tests := []struct { src string offset int @@ -242,7 +243,8 @@ func TestLine(t *testing.T) { } } -func TestLintName(t *testing.T) { +// TestLintName tests lint.Name function +func TestLintName(t *testing.T) { //revive:disable-line:exported tests := []struct { name, want string }{ @@ -275,7 +277,7 @@ func TestLintName(t *testing.T) { {"IEEE802_16Bit", "IEEE802_16Bit"}, } for _, test := range tests { - got := lint.Name(test.name) + got := lint.Name(test.name, nil, nil) if got != test.want { t.Errorf("lintName(%q) = %q, want %q", test.name, got, test.want) } @@ -301,7 +303,8 @@ func exportedType(typ types.Type) bool { return true } -func TestExportedType(t *testing.T) { +// TestExportedType tests exportedType function +func TestExportedType(t *testing.T) { //revive:disable-line:exported tests := []struct { typString string exp bool @@ -356,7 +359,8 @@ func isGenerated(src []byte) bool { return false } -func TestIsGenerated(t *testing.T) { +// TestIsGenerated tests isGenerated function +func TestIsGenerated(t *testing.T) { //revive:disable-line:exported tests := []struct { source string generated bool diff --git a/test/var-naming_test.go b/test/var-naming_test.go new file mode 100644 index 0000000..2d0bf75 --- /dev/null +++ b/test/var-naming_test.go @@ -0,0 +1,14 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestVarNaming(t *testing.T) { + testRule(t, "var-naming", &rule.VarNamingRule{}, &lint.RuleConfig{ + Arguments: []interface{}{[]interface{}{"ID"}, []interface{}{"VM"}}, + }) +} diff --git a/test/waitgroup-by-value_test.go b/test/waitgroup-by-value_test.go new file mode 100644 index 0000000..398ac9a --- /dev/null +++ b/test/waitgroup-by-value_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestWaitGroupByValue(t *testing.T) { + testRule(t, "waitgroup-by-value", &rule.WaitGroupByValueRule{}) +} diff --git a/untyped.toml b/untyped.toml index 5c7ff44..c824dd0 100644 --- a/untyped.toml +++ b/untyped.toml @@ -13,3 +13,9 @@ [rule.receiver-naming] [rule.indent-error-flow] [rule.empty-block] +[rule.range-val-in-closure] +[rule.waitgroup-by-value] +[rule.atomic] +[rule.empty-lines] +[rule.line-length-limit] + arguments = [200]