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

proposal: cmd/vet: relax "composites" check to module scope #43864

Open
muirdm opened this issue Jan 23, 2021 · 3 comments
Open

proposal: cmd/vet: relax "composites" check to module scope #43864

muirdm opened this issue Jan 23, 2021 · 3 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal
Milestone

Comments

@muirdm
Copy link

muirdm commented Jan 23, 2021

I propose we relax the "composites" vet check to allow implicit field name use if the struct literal and struct type are within the same module.

Currently the "composites" vet check allows implicit struct field names if the struct type and struct literal are in the same package. When adding a new struct field you can certainly update uses in the same package and avoid errors. Now that modules provide a formal grouping of packages we can further relax the "composites" check to allow implicit field use within the same module because you similarly can update all struct uses throughout the module when adding a new struct field.

This came up when discussing whether gopls should suppress implicit field completions that would fail vet.

To implement this change the analyzer would need to be able to map a types.Package to a module name. I don't know exactly how this would work.

/cc @mvdan

@seankhliao seankhliao changed the title cmd/vet: relax "composites" check to module scope proposal: cmd/vet: relax "composites" check to module scope Jan 23, 2021
@gopherbot gopherbot added this to the Proposal milestone Jan 23, 2021
@mvdan
Copy link
Member

mvdan commented Jan 23, 2021

The rationale here is that, since a module is versioned and released as a single entity, the backwards compatibility that this vet check enhances - by allowing adding or reordering fields in a struct type - is less important for packages in the same module. Using a composite literal defined in a package which belongs in a different module would still require explicit keys.

I personally prefer to be explicit more often than not with composite literals, but I also find it reasonable that in some cases where the type belongs to the same module, the check might be too pedantic given that vet is meant to be used and followed by everyone.

@mvdan
Copy link
Member

mvdan commented Jan 24, 2021

To implement this change the analyzer would need to be able to map a types.Package to a module name. I don't know exactly how this would work.

Something to add there - if go vet is used in GOPATH mode, or with an ad-hoc package that does not belong to any module, the current behavior would not change.

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Jan 26, 2021
@timothy-king
Copy link
Contributor

To implement this change the analyzer would need to be able to map a types.Package to a module name. I don't know exactly how this would work.

I am not currently aware of a good path from the info available in an analysis.Pass to get the module name. If this was available, one would need to communicate the module names between packages in the same module. At the moment, this means writing Facts. Likely a package fact with the module name. Checking whether the names of the modules match seems straightforward after this. Still need the first part solved though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal
Projects
Status: Incoming
Development

No branches or pull requests

4 participants