cc @matloob @timothy-king @guodongli-google @zpavlinovic

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.

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.

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.

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.

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?

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.

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.

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.