Prevent the need for a version bump and fix a few test cases#1
Prevent the need for a version bump and fix a few test cases#1ashanbrown wants to merge 4 commits intopohly:import-aliasesfrom
Conversation
It was possible to use fmt.Print by importing "fmt" under a different name. Now a pattern can get matched against the full package path instead of the import name. This is also useful to ban items from packages that may get imported under various different names.
Previously, import renaming led to false negatives:
import fmt2 "fmt"
fmt2.Println("hello world")
There were also false positives for source code that didn't
use the fmt package and just looked like it did:
fmt := struct {
Println string
}{}
return fmt.Println
Now the default pattern uses the new import renaming support and
only matches when the actual "fmt" standard package provides the
forbidden symbol.
forbidigo/forbidigo.go
Outdated
| } | ||
|
|
||
| func DefaultPatterns() []string { | ||
| return []string{`^((?P<pkg>fmt)\.Print(|f|ln)|print|println)$`} |
There was a problem hiding this comment.
This didn't work for the case without a package name.
There was a problem hiding this comment.
Which Go code contains just plain print or println function calls? Do we really need to support that?
There was a problem hiding this comment.
print and println are built-in go functions (see https://go.dev/ref/spec) that provide an easy way to output log lines (but are generally bad news for production code)
There was a problem hiding this comment.
(I haven't mentioned it to this point but I have been wondering it would be useful to provide some way to differentiate between built-in functions and local redefinitions, but it didn't seem important enough to me to worry about for now)
There was a problem hiding this comment.
Interesting, I didn't know those functions exist - my introduction to Go skipped over them, obviously for good reasons 😁
I copied this over into the main PR.
| cfg := packages.Config{ | ||
| Mode: packages.NeedSyntax | packages.NeedName | packages.NeedFiles | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedDeps, | ||
| Mode: packages.NeedSyntax | packages.NeedName | packages.NeedFiles | packages.NeedTypes | packages.NeedTypesInfo, | ||
| Tests: *includeTests, |
There was a problem hiding this comment.
I didn't mean to remove this but which case do we need it for? I could imagine it being expensive.
There was a problem hiding this comment.
Remove it from forbidigo/forbidigo_test.go and parsing will fail with:
2022/12/05 09:06:46 internal error: package "fmt" without types was imported from "_/tmp/TestForbiddenIdentifiersit_finds_forbidden__renamed_identifiers1395216622/001"
| return l.RunWithTypes(fset, nil, nodes...) | ||
| } | ||
|
|
||
| func (l *Linter) RunWithTypes(fset *token.FileSet, typesInfo *types.Info, nodes ...ast.Node) ([]Issue, error) { |
There was a problem hiding this comment.
ashanbrown#22 then will add pkg *types.Package - do we then change the name again or interpret "WithTypes" as "with information from the types package"? I suspect you meant "with types info" here, in which case the function would have to become "RunWithTypesAndPkg" or something.
How about putting the args into a struct:
type RunConfig struct {
fset *token.FileSet // required
typesInfo *types.Info // optional, "pkg" rules won't work without it
pkg *types.Package // optional, "file" rules won't work without it
}
func (l *Linter) RunWithConfig(config RunConfig, nodes ...ast.Node) ([]Issue, error) {
...
There was a problem hiding this comment.
The advantage of the struct is that new optional fields can be added for future extensions without an API break.
|
That new interface sounds good to me. |
pohly
force-pushed
the
import-aliases
branch
from
9936bb4 to
f9243d1
Compare
pohly
force-pushed
the
import-aliases
branch
5 times, most recently
from
2621c02 to
cac3c7d
Compare
2 participants
Here are a few fixes. First, I went back to your proposal to use a new method for
Runto avoid having to bump this package to v2. Second, I've fixed up main/examples to work as expectedmake testpasses now.Thanks for getting us this far. I think we're almost there. I'd welcome the other minor style fixes I suggested on your MR before I merge this.