-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/tools/go/analysis/passes/composite: check locally defined structs #54823
Comments
My main concern is that if you turned such a flag on, one would very frequently end up applying this analyzer to transitive dependencies. Those packages were not developed with this coding convention in mind and may contain unkeyed composite literals. What would one then do with these diagnostics? Updating transitive dependencies is unrealistic. My experience with this specific property is that you will see +100s of diagnostics if these are surfaced for transitive dependencies on a large code base. To have this on a flag, I think we need to know what we different drivers of the analyzers would do: bazel, go cmd, stand alone binary, gopls, etc. The x/tools/go/analysis/internal/checker package does filtering down to just have diagnostics from root actions and packages. This might be enough for this case. @adonovan for bazel thoughts. An alternative is to have composite have a factory function that returns a *Analyzer and the factor can configure whether it checks locals or not. The composite.Analyzer global can then created from this. This would require users to create their own single/multicheckers. So this would end up a mild improvement over "fork it". However, I would not be as concerned as a flag change, which is a visible behavior change from |
I don't think it is unconditionally desirable for this checker to treat local cases as an error, for a reason I discovered by coincidence this morning: within a package, the expression And as Tim points out, Go style has for a long time required field names for imported types but not required them for local types, so this change would go against years of convention. |
I am not sure I understand this problem. Can you give an example of such situation?
It is an interesting use case. But in my opinion, an uncommon one I would agree with these concerns if the default behavior was changed. But I suggest to add an option that will be used only when needed. However, it’s not a hill I’m willing to die on, and any result will suite me. |
I think Tim is pointing out that this analyzer will be very noisy for code that you do not own, since local unkeyed literals are an accepted style.
Gopls doesn't support customizing analyzers, but internally we've discussed exposing flag configuration at some point in the future.
This is something we do frequently in the standard library (or at least go/* libraries), and it is a style that I have grown to appreciate. My intuition is that this feature would not produce enough true positives to justify its noisiness, and therefore does not meet the bar for cmd/vet (though we could escalate this to the proposal committee for further discussion). Assuming that we don't want this enabled for vet, the question then becomes whether or not we want to support features that are disabled for most users. Normally for a change like this, we look for evidence that this new analysis will catch real bugs. I don't know how to prove that, in this case, since the bugs would manifest as a literal value switching fields. |
I'll try. Say you are the author of a package P which import Q (written by someone else) which imports R (written by someone else). Let's say you want to run this analyzer, let's call is strictcomposites, and say
If we did expose flag configuration in the future, do you know if the Diagnostics for
Good question. Having global Analyzer variables and no constructor functions makes customization like this tricky. Something similar comes up for users of passes/buildssa that want debug info enabled. But at the same time, is it worth the maintenance cost in each case? |
Suppressed: by default gopls only reports analysis diagnostics for packages containing opened files. |
To expand on @findleyr's comment a bit, let's say this was a flag and enabled. Then if someone jumped to a definition in another package in gopls, Diagnostics would start being displayed in those packages. This would be Q or R in my example. This makes me think we are going to have too many cases where a dependent package was written for I am still not particularly opposed to having a factory function that returns a composite Analyzer for a different configuration options. It is not obviously worth the maintenance cost though. |
For some reason, I thought it was already possible. My bad
Thanks for the explanation. I usually run analyzers in CI via
Is there any option to change this behavior? I couldn't find anything in the settings. After reading all the arguments, I can agree that this flag won't be very useful. So, I am closing this issue. |
Right now the
composite
analyzer doesn't report unkeyed locally defined composite literals. However, they also can lead to some obscure bugs (for example, after reordering of fields with the same type).So, I would like to suggest to add a new flag named
local
that will define whether to check locally defined structs. To preserve the current behavior, the default value will befalse
.The change itself is quite simple. But before creating a PR, I would like to hear someone else's thoughts on this.
The text was updated successfully, but these errors were encountered: