-
Notifications
You must be signed in to change notification settings - Fork 15
match imports and variables against their canonical name #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| linters: | ||
| enable: | ||
| - prealloc | ||
| - nakedret |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| repos: | ||
| - repo: https://github.com/golangci/golangci-lint | ||
| rev: v1.44.2 | ||
| rev: v1.50.1 | ||
| hooks: | ||
| - id: golangci-lint |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,12 @@ | ||
| SHELL=bash | ||
|
|
||
| setup: | ||
| pre-commit install | ||
|
|
||
| test: | ||
| cd examples && diff <(sed 's|CURDIR|$(CURDIR)|' expected_results.txt) <(go run .. 2>&1 | sed '/^go: downloading/d') | ||
|
|
||
| lint: | ||
| pre-commit run --all-files | ||
|
|
||
| .PHONY: lint test |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,20 +16,121 @@ If no patterns are specified, the default pattern of `^(fmt\.Print.*|print|print | |
| functions (and whole files), that are identifies as Godoc examples (https://blog.golang.org/examples) are excluded from | ||
| checking. | ||
|
|
||
| A larger set of interesting patterns might include: | ||
| By default, patterns get matched against the actual expression as it appears in | ||
| the source code. The effect is that ``^fmt\.Print.*$` will not match when that | ||
| package gets imported with `import fmt2 "fmt"` and then the function gets | ||
| called with `fmt2.Print`. | ||
|
|
||
| This makes it hard to match packages that may get imported under a variety of | ||
| different names, for example because there is no established convention or the | ||
| name is so generic that import aliases have to be used. To solve this, | ||
| forbidigo also supports a more advanced mode where it uses type information to | ||
| identify what an expression references. This needs to be enabled through the | ||
| `analyze_types` command line parameter. Beware this may have a performance | ||
| impact because additional information is required for the analysis. | ||
|
|
||
| Replacing the literal source code works for items in a package as in the | ||
| `fmt2.Print` example above and also for struct fields and methods. For those, | ||
| `<package name>.<type name>.<field or method name>` replaces the source code | ||
| text. `<package name>` is what the package declares in its `package` statement, | ||
| which may be different from last part of the import path: | ||
|
|
||
| import "example.com/some/pkg" // pkg uses `package somepkg` | ||
| s := somepkg.SomeStruct{} | ||
| s.SomeMethod() // -> somepkg.SomeStruct.SomeMethod | ||
|
|
||
| Pointers are treated like the type they point to: | ||
|
|
||
| var cf *spew.ConfigState = ... | ||
| cf.Dump() // -> spew.ConfigState.Dump | ||
|
|
||
| When a type is an alias for a type in some other package, the name of that | ||
| other package will be used. | ||
|
|
||
| An imported identifier gets replaced as if it had been imported without `import .` | ||
| *and* also gets matched literally, so in this example both `^ginkgo.FIt$` | ||
| and `^FIt$` would catch the usage of `FIt`: | ||
|
|
||
| import . "github.com/onsi/ginkgo/v2" | ||
| FIt(...) // -> ginkgo.FIt, FIt | ||
|
|
||
| Beware that looking up the package name has limitations. When a struct embeds | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One other limitation I'm wondering about is what happens with type aliases.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's mentioned:
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yeah, nm
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although that isn't identified as a caveat (I was looking in the section below when I made this comment). Are we saying that we can't match types that are aliases by their own name?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's the effect. I'm undecided whether that is a good thing or a bad thing. The good side of it is that a pattern "xyz.Something" triggers also when some other package "abc" has an alias for it. Hmm, I need to check whether that works also when the name is different. The bad side is that the user needs to know when a type in a package like "abc" is an alias, because then "abc.Something" will not match.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It works as described:
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect a pattern that matches a type to also match that aliases of that type, but not the other way around. To, me, this points to multiple expansions as we walk up the alias chain, similar to the two expansions we do for identifies (with and without the package name).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With "pattern that matches a type", you mean Or did you mean The current situation is that That A rule for My preference would be to move ahead with the PR as it stands because the current behavior is documented correctly and wouldn't change, even if we later add support for matching the alias (and only the alias). An issue for this would be good as reminder.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right. I'm ok with moving forward since this behavior doesn't affect the default path, but I think not being able to match the alias by name is definitely unexpected. Unless you look at the underlying code, you'd have no reason to know if a type is an alias or not.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. => #23 |
||
| some other type, references to the inherited fields or methods get resolved | ||
| with the outer struct as type: | ||
|
|
||
| package foo | ||
|
|
||
| type InnerStruct { | ||
| SomeField int | ||
| } | ||
|
|
||
| func (i innerStruct) SomeMethod() {} | ||
|
|
||
| type OuterStruct { | ||
| InnerStruct | ||
| } | ||
|
|
||
| * `^fmt\.Print.*$` -- forbid use of Print statements because they are likely just for debugging | ||
| * `^fmt\.Errorf$` -- forbid Errorf in favor of using github.com/pkg/errors | ||
| * `^ginkgo\.F[A-Z].*$` -- forbid ginkgo focused commands (used for debug issues) | ||
| * `^spew\.Dump$` -- forbid dumping detailed data to stdout | ||
| * `^fmt\.Errorf(# please use github\.com/pkg/errors)?$` -- forbid Errorf, with a custom message | ||
| s := OuterStruct{} | ||
| s.SomeMethod() // -> foo.OuterStruct.SomeMethod | ||
| i := s.SomeField // -> foo.OuterStruct.SomeField | ||
|
|
||
| When a method gets called via some interface, that invocation also only | ||
| gets resolved to the interface, not the underlying implementation: | ||
|
|
||
| // innerStruct as above | ||
|
|
||
| type myInterface interface { | ||
| SomeMethod() | ||
| } | ||
|
|
||
| var i myInterface = InnerStruct{} | ||
| i.SomeMethod() // -> foo.myInterface.SomeMethod | ||
|
|
||
| Using the package name is simple, but the name is not necessarily unique. For | ||
| more advanced cases, it is possible to specify more complex patterns. Such | ||
| patterns are strings that contain JSON or YAML for a struct. | ||
|
|
||
| The full pattern struct has the following fields: | ||
|
|
||
| * `msg`: an additional comment that gets added to the error message when a | ||
| pattern matches. | ||
| * `pattern`: the regular expression that matches the source code or expanded | ||
| expression, depending on the global flag. | ||
| * `package`: a regular expression for the full package import path. The package | ||
| path includes the package version if the package has a version >= 2. This is | ||
| only supported when `analyze_types` is enabled. | ||
|
|
||
| To distinguish such patterns from traditional regular expression patterns, the | ||
| encoding must start with a `{` or contain line breaks. When using just JSON | ||
| encoding, backslashes must get quoted inside strings. When using YAML, this | ||
| isn't necessary. The following pattern strings are equivalent: | ||
|
|
||
| {p: "^fmt\\.Println$", msg: "do not write to stdout"} | ||
|
|
||
| {p: ^fmt\.Println$, | ||
| msg: do not write to stdout, | ||
| } | ||
|
|
||
| {p: ^fmt\.Println$, msg: do not write to stdout} | ||
|
|
||
| p: ^fmt\.Println$ | ||
| msg: do not write to stdout | ||
|
|
||
| A larger set of interesting patterns might include: | ||
|
|
||
| Note that the linter has no knowledge of what packages were actually imported, so aliased imports will match these patterns. | ||
| -* `^fmt\.Print.*$` -- forbid use of Print statements because they are likely just for debugging | ||
| -* `^fmt\.Errorf$` -- forbid Errorf in favor of using github.com/pkg/errors | ||
| -* `^ginkgo\.F[A-Z].*$` -- forbid ginkgo focused commands (used for debug issues) | ||
| -* `^spew\.Dump$` -- forbid dumping detailed data to stdout | ||
| -* `^spew.ConfigState\.Dump$` -- also forbid it via a `ConfigState` | ||
| -* `^fmt\.Errorf(# please use github\.com/pkg/errors)?$` -- forbid Errorf, with a custom message | ||
| -* `{p: ^fmt\.Errorf$, msg: please use github.com/pkg/errors}` -- the same with separate msg field | ||
|
|
||
| ### Flags | ||
| - **-set_exit_status** (default false) - Set exit status to 1 if any issues are found. | ||
| - **-exclude_godoc_examples** (default true) - Controls whether godoc examples are identified and excluded | ||
| - **-tests** (default true) - Controls whether tests are included | ||
| - **-analyze_types** (default false) - Replace literal source code before matching | ||
|
|
||
| ## Purpose | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.