-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
forbidigo: better support for configuring complex rules #3612
Conversation
pkg/golinters/forbidigo.go
Outdated
@@ -40,14 +41,20 @@ func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter { | |||
}, | |||
} | |||
|
|||
loadMode := goanalysis.LoadModeSyntax | |||
if settings != nil && settings.ExpandExpressions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we also need to set the analyzer flag called analyze_types
when we see this set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which analyzer flag?
goanalysis uses different "levels", not individual flags: https://pkg.go.dev/github.com/golangci/golangci-lint/pkg/golinters/goanalysis#LoadMode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we were using forbidigo as an analyzer, per the new guidelines for linters. For an analyzer, we'd set the analyze_types
flag at https://github.com/ashanbrown/forbidigo/blob/master/pkg/analyzer/analyzer.go#L55. That said, it looks like we've set up forbidigo to automatically expand types when TypesInfo
is provided. I think I hadn't quite realized we were using that as a flag, rather than just information. In retrospect, my preference would have been to explicitly add a new analyzeTypes
option at https://github.com/ashanbrown/forbidigo/blob/master/forbidigo/forbidigo.go#L65 (similar for what we did for the analyzer mode).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using types, your linter is now considered a slow linter.
#3639
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ldez That makes sense, and we had anticipated this issue, although I didn't realize that it was formally checked in golangci-lint. The linter can work without types, but there is an opt-in behavior that allows us to use types to match more things. Is there any precedent for a linter being both fast and slow? I don't suppose we could have both fast and slow versions of the same linter? I'd imagine they would have to have separate names and configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is type analysis triggered by the line at https://github.com/golangci/golangci-lint/pull/3612/files#diff-b647b23fb0e6073b7f161d88a8cd16af6dd3e90de1d6a81006d2d9a4ee416707R424? In this PR, it appears that setting is conditionally executed based on the configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal was to not change behavior unless the user explicitly asks for it in the config. It looks like there are two places where that is relevant:
- GetAllSupportedLinterConfigs: covered by this PR
- NewForbidigo: covered by this PR
The already merged commit unconditionally reconfigures forbidigo to enable type analysis. There's a certain risk that this breaks users because existing patterns get interpreted differently. I missed earlier that the PR wasn't just the automatic bump of the forbidigo package.
I think we should merge this PR here soon because it is required to finish the forbidigo update to 1.5.x properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I'm not keeping up with you guys 😢
So forbidigo 1.5.x is new, and the upgrade of it in #3639 is something that I really only saw today because it's also new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR makes forbidigo a "slow" linter, without actually enabling the feature which uses the additional information (no call of OptionAnalyzeTypes(true)
.
So my statement above still stands: this PR is needed to finish the forbidigo update. I just need to add OptionAnalyzeTypes(true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it.
208ecbc
to
bd4c77e
Compare
FYI, I've bumped |
bd4c77e
to
aa112f6
Compare
.golangci.reference.yml
Outdated
# Optionally put comments at the end of the regex, surrounded by `(# )?` | ||
# Escape any special characters. | ||
# Optional message that gets included in error reports. | ||
- pattern: ^fmt\.Print.*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we ended up calling this "p" at https://github.com/golangci/golangci-lint/pull/3612/files#diff-cb1fbdbfccd3fd0d8fe9d9006138fbcd1afcc3a701b37180c6fe50ed0c5c390fR332.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, and I did the same for ForbidigoPattern
in linter_settings.go
. I've fixed the reference file to match.
It's worth calling out that we can use pattern
here if p
is considered too brief: we can map that in linter_settings.go
:
// ForbidigoPattern corresponds to forbidigo.pattern and adds
// mapstructure support. The YAML field names must match what
// forbidigo expects.
type ForbidigoPattern struct {
// patternString gets populated when the config contains a string as
// entry in ForbidigoSettings.Forbid[] because ForbidigoPattern
// implements encoding.TextUnmarshaler and the reader uses the
// mapstructure.TextUnmarshallerHookFunc as decoder hook.
//
// If the entry is a map, then the other fields are set as usual
// by mapstructure.
patternString string
Pattern string `yaml:"p" mapstructure:"pattern"` // <=====
Package string `yaml:"pkg,omitempty" mapstructure:"pkg,omitempty"`
Msg string `yaml:"msg,omitempty" mapstructure:"msg,omitempty"`
}
I myself think golangci-lint should follow the choice made for upstream forbidigo and use p
, too.
pkg/lint/lintersdb/manager.go
Outdated
WithURL("https://github.com/ashanbrown/forbidigo") | ||
if forbidigoCfg != nil && forbidigoCfg.AnalyzeTypes { | ||
// Need more information for the analysis. | ||
l = l.WithLoadForGoAnalysis() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetAllSupportedLinterConfigs
is called in different situations, the linter config will not be available in all those situations, so please remove this and don't try to create an asymetrical configuration between WithLoadForGoAnalysis
and LoadModeTypesInfo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see how not setting IsSlow
in GetAllSupportedLinterConfigs
when called without config is wrong (or at least potentially misleading).
But is it also useful in NewForbidigo
to use LoadModeTypesInfo
whether it's needed or not? Can something break when combining WithLoadForGoAnalysis
with LoadModeSyntax
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's not related to isSlow
but to loadMode
.
if the 2 load modes are not synced, some unexpected behavior will appear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I've pushed an update which always uses the more expensive mode. In the long run I'd expect users to switch to it anyway. It's just not the default because of some risk of breaking certain patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back to the main point of this discussion: @ldez, you had asked that we not be asymmetric in our use of WithLoadForGoAnalysis
in one place and not the other. Could we simply report that we need the requirements of a fast linter except when this "experimental" setting is set in the config file? When we see the experimental config, we'd add the extra load requirements. I realize this is still asymmetric, although at least it would no longer call WithLoadForGoAnalysis
. Is the concern that by reporting a linter as fast, say in a help message, and later making it slow, we would be confusing the user, or is there something that would actually break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go test -v -run TestSourcesFromTestdata/forbidigo ./test
doesn't pass when GetAllSupportedLinterConfigs
doesn't use WithLoadForGoAnalysis
for forbidigo, so it has technical implications.
If we absolutely need a "fast forbidigo" (not convinced yet myself, but usage patterns are different and I don't want to make forbidigo less useful for its original author!) and a "throrough forbidigo" (a strong yes from me for Kubernetes, because import aliases make the current source-code based rules unreliable), then we could register forbidigo twice:
- Once with the current configuration and mode of operation (no changes).
- Again with a different name (working title "forbidigo-analyze-types", better suggestions welcome) and the new configuration without the
AnalyzeTypes
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's LoadModeTypesInfo
that is causing us to be considered a slow linter. For example, durationCheck uses this load mode and is still considered fast. I think it's the requirement to load dependencies that would make this a slow linter. WithLoadForGoAnalysis
does this:
lc.LoadMode |= packages.NeedImports | packages.NeedDeps | packages.NeedExportFile | packages.NeedTypesSizes
This was added at #1844. Perhaps @ldez can fill us on as to why we can't just set IsSlow
based on the presence of select LoadMode flags.
I'd personally be happy with the multiple-linter configs but that had sounded like a non-starter to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goanalysis.LoadModeTypesInfo
required WithLoadForGoAnalysis()
.
A linter with two "speed states" doesn't seem right.
"slow" or "fast" don't really describe the speed of a linter, it's just a best-effort state.
The "slow" linters share a state for the slow part, so if you run only one linter you will pay the cost but if your run several linters the cost will be shared.
Duplicating the linter doesn't seem like the right idea too.
I think it's not really important if your linter is flagged as "slow" or "fast".
But if it's important to you, you have to make choice inside the linter: use types or not.
I don't want to create unneeded complexity inside the configuration, I also prefer maintainability over possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to register it with WithLoadForGoAnalysis
unconditionally and then decide in NewForbidigo
based on the actual setting? At least the tests are passing that way.
Would it make the linter run faster? I'm not so sure.
For Kubernetes, golangci-lint run --disable-all --enable forbidigo -v
with that approach uses LoadModeSyntax
:
INFO Execution took 8.982228038s
That's after cleaning the golangci-lint cache, but with a warm Go build cache.
Without this hack it's 9.556500684s - a bit slower, but not much.
With an empty Go cache (go clean --cache
), the difference is 56.58711488s (with that approach) vs. 57.399810283s (without).
For comparison, with forbidigo master (985efcc) it's 9.639382845s and 57.23967985s.
These numbers are so close together, I don't see a relevant difference. It probably comes down to noise - otherwise master would have to be the fastest.
@ashanbrown: can you give a specific example where you want to use "fast" analysis? Does this PR really make a significant difference?
aa112f6
to
8a2f7e7
Compare
pkg/lint/lintersdb/manager.go
Outdated
WithURL("https://github.com/ashanbrown/forbidigo") | ||
if forbidigoCfg != nil && forbidigoCfg.AnalyzeTypes { | ||
// Need more information for the analysis. | ||
l = l.WithLoadForGoAnalysis() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetAllSupportedLinterConfigs
is called in different situations, the linter config will not be available in all those situations, so please remove this and don't try to create an asymetrical configuration between WithLoadForGoAnalysis
and LoadModeTypesInfo
.
176e87c
to
a5cb733
Compare
My latest push triggered some linter warnings. I've fixed those: diff --git a/pkg/golinters/forbidigo.go b/pkg/golinters/forbidigo.go
index cad20b4e..b46c8698 100644
--- a/pkg/golinters/forbidigo.go
+++ b/pkg/golinters/forbidigo.go
@@ -16,6 +16,7 @@ import (
const forbidigoName = "forbidigo"
+//nolint:dupl
func NewForbidigo(settings *config.ForbidigoSettings) *goanalysis.Linter {
var mu sync.Mutex
var resIssues []goanalysis.Issue
@@ -59,9 +60,7 @@ func runForbidigo(pass *analysis.Pass, settings *config.ForbidigoSettings) ([]go
forbidigo.OptionExcludeGodocExamples(settings.ExcludeGodocExamples),
// disable "//permit" directives so only "//nolint" directives matters within golangci-lint
forbidigo.OptionIgnorePermitDirectives(true),
- }
- if settings != nil && settings.AnalyzeTypes {
- options = append(options, forbidigo.OptionAnalyzeTypes(true))
+ forbidigo.OptionAnalyzeTypes(settings.AnalyzeTypes),
}
// Convert patterns back to strings because that is what NewLinter It looks like |
Looking forward to this getting merged! Once it is, please update the Schema stored here: https://github.com/SchemaStore/schemastore/blob/e321c0e165562a845e3d05b28f4ce2054bc11d7e/src/schemas/json/golangci-lint.json#L911-L913 |
@pohly Could you let us know where this PR is at? I'm still not excited about this never being a fast linter (although I think that change went in inadvertently with the dependency bump), but I figure it's better that we push it out and get feedback, given that there seems to be interest in the new features. Thanks. |
@ashanbrown: I think @ldez is just waiting for your okay to merge it as it is now. You were the one who had concerns and it wouldn't be fair to just ignore that. The discussion in #3612 (comment) stopped with me asking about some specific example where I can try out myself what the actual difference is. If you feel that this is not necessary, then we can also just go ahead and merge. |
Got it @pohly. I can approve this. As I mentioned, I don't think this causes any performance regression vs the previous release. I don't quite understand what your benchmarks you showed are trying to cover. Once we call |
forbidigo 1.4.0 introduced a new configuration mechanism with multiple fields per rule. It still supports a single string, but allowing structs as alternative to such strings makes the configuration easier to read and write. A new global flag is needed to enable the more advanced analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
forbidigo 1.4.0 introduced a new configuration mechanism with multiple fields per rule. It still supports a single string, but allowing structs as alternative to such strings makes the configuration easier to read and write.
A new global flag is needed to enable the more advanced analysis.