Skip to content
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

Closed
ShoshinNikita opened this issue Sep 1, 2022 · 9 comments
Closed
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@ShoshinNikita
Copy link

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 be false.

The change itself is quite simple. But before creating a PR, I would like to hear someone else's thoughts on this.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 1, 2022
@gopherbot gopherbot added this to the Unreleased milestone Sep 1, 2022
@heschi heschi added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 2, 2022
@heschi
Copy link
Contributor

heschi commented Sep 2, 2022

@timothy-king
Copy link
Contributor

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.
@findleyr for gopls 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 cmd/vet.

@adonovan
Copy link
Member

adonovan commented Sep 2, 2022

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).

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 MyStruct{a, b, c} acts as a compile-time assertion that MyStruct has exactly three fields. Forcing it to use field names would remove this assertion, making it more likely that the addition of a new field to MyStruct would not be accompanied by a corresponding change to each MyStruct{...} literal within the package. You're right that the order-based struct literals are vulnerable to field reordering, but that's an infrequent occurrence, and one that usually results in a type error.

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.

@ShoshinNikita
Copy link
Author

one would very frequently end up applying this analyzer to transitive dependencies

I am not sure I understand this problem. Can you give an example of such situation?

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 MyStruct{a, b, c} acts as a compile-time assertion that MyStruct has exactly three fields.

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.

@findleyr
Copy link
Contributor

findleyr commented Sep 6, 2022

one would very frequently end up applying this analyzer to transitive dependencies

I am not sure I understand this problem. Can you give an example of such situation?

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.

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.

Gopls doesn't support customizing analyzers, but internally we've discussed exposing flag configuration at some point in the future.

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 MyStruct{a, b, c} acts as a compile-time assertion that MyStruct has exactly three fields.

It is an interesting use case. But in my opinion, an uncommon one

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.

@timothy-king
Copy link
Contributor

I am not sure I understand this problem. Can you give an example of such situation?

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 x/tools/go/analysis/passes/printf (the specifics do not matter beyond enabling Facts). You run these checkers from singlechecker, multichecker or go vet on P. The mechanics are such that you will end up applying strictcomposites to R. If R has MyStruct{a, b, c} internally, strictcomposites would produce a Diagnostic when applied. The question is then is driving running the analyzers going to print/display/report Diagnostics from (strictcomposites, R) or suppress them when it is run from P. If this does not happen, strictcomposites will be much too noisy to enable. It would complain about using standard libraries and many other valid usages. This is why I am asking others for what different drivers would choose to display.

Gopls doesn't support customizing analyzers, but internally we've discussed exposing flag configuration at some point in the future.

If we did expose flag configuration in the future, do you know if the Diagnostics for (strictcomposites, R) would be displayed or suppressed? Would it depend on what files are opened?

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.

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?

@findleyr
Copy link
Contributor

findleyr commented Sep 6, 2022

If we did expose flag configuration in the future, do you know if the Diagnostics for (strictcomposites, R) would be displayed or suppressed? Would it depend on what files are opened?

Suppressed: by default gopls only reports analysis diagnostics for packages containing opened files.

@timothy-king
Copy link
Contributor

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 composites, but not strictcomposites. This makes me really hesitant to have this behavior controlled by a flag.

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.

@ShoshinNikita
Copy link
Author

Gopls doesn't support customizing analyzers, but internally we've discussed exposing flag configuration at some point in the future.

For some reason, I thought it was already possible. My bad

I'll try. Say you are the author of a package...

Thanks for the explanation. I usually run analyzers in CI via golangci-lint, and it doesn't show diagnostics for std or third-party packages. And, as said before, VSCode shows diagnostics only for packages with opened files. So, I wasn't aware of this nuance of analyzer internals.

by default gopls only reports analysis diagnostics for packages containing opened files

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.

@ShoshinNikita ShoshinNikita closed this as not planned Won't fix, can't repro, duplicate, stale Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants