I suspect I'm going to advocate for go vet -composites=false ./... in projects I contribute to (or possibly some smattering of -compositewhitelist application -- although I think the sheer existence of that flag hints at why the whole feature is questionable).

But since the whole section is "strongly recommended" and not a "must", this is just a comment and not necessarily a request for change.

Well, I do want this to be a "must" for our Go repos. Otherwise the entire "share a YAML template across all Go repos" goes out the window.

The "strongly recommended" is in terms of this being a generic doc for any and all Go devs. It's meant to be a "must" for PL, for the sake of consistency and automation.

I think it's fine if you want to do something else for projects that you alone develop, but we have to find some common ground on repositories where there are multiple developers. That's not just necessary for CI; humans will run into trouble if they try to write code in exactly opposite ways.

Another option is to partly or entirely give up on vet, but I personally oppose that unless you're suggesting to raise an issue upstream to remove the "composites" analyzer. If we're not trying to fix this upstream, and we're doing the opposite of what the vast majority of devs follow, we'll make our Go repositories pretty hostile to Go developers or contributors :)

That's just my two cents. I think we have to figure out this disagreement, because otherwise we'll end up with Go code being maintained in two incompatible styles.

Alright, fine, here we go then.

"exactly opposite ways" strikes me as quite hyperbolic. So does "two incompatible styles": this is really not that big of an issue -- or it wouldn't be, if go vet didn't make it an issue.

I do think the "composites" rule in go vet is sufficiently bizarre that, if I cared what go vet did at all, I would be happy to campaign in the upstream that it should be removed, yes. It seems significantly more controversial and significantly less useful than any of the other go vet rules. Every other go vet rule looks for actual bugs; this one rule does not. There is absolutely no bug-detection power in the composites rule.

I think there's a reason that golang syntax allows unnamed composite constructions, and I think the go vet tool overreaches significantly by deciding to poo-poo that entire language feature.

The explicitly-named-fields-in-composites rule makes some code ridiculously verbose for no reason. It is not uncommon to have a constructor function func(foo Foo, bar Bar, baz Baz) Zot and for its body to contain Zot{foo, bar, baz}. I see no joy at all in extending every such thing to Zot{foo: foo, bar: bar, baz: baz}. It's absolute line noise. Could I survive this? Yes. (I wrote Java for some years!) Do I want to have this verbosity forced on me, unconditionally? Not really.

It's been argued that the requirement of explicitly named fields in composite initializers creates more compile time safety, but I think this is objectively untrue. It provides different compiler assistance. Explicitly named fields in composite initializers means one can add fields to the struct definition without needing to update composite initializers. This is sometimes the exact opposite of what I want. In some cases it is good to be able to add fields without requiring a code change in other reference sites. In other cases, it is desirable to have the compiler forcibly inform me that I need to update other reference sites. Use of unnamed composite initializers produces the latter case. Neither of these behaviors is always the desired one, but in my opinion, the latter scenario (aka, "please, compiler, tell me what I need to update when I make changes") predominates. (If it was possible to use named initializers and annotate that the compiler should reject this composite if any fields remain uninitialized, perhaps I would regard this differently; but there is no such ability.)

The sheer existence of the -compositewhitelist flag seems to be a tacit admission that this rule is not universally applicable, and that one may have good reason to disable it in specific cases. (Either that, or it's simply a tacit admission of how very controversial this rule has been to users of go vet over time.) I see this kind of conditional applicability as an indication that the rule in question simply may not be a good thing to enforce as a rule at all.

I consider each of the above four five paragraphs a reason that the go vet composites rule is significantly questionable. Taken together, it's very questionable indeed.

Explicitly named fields in composites are often a good choice. I just absolutely don't buy that it's unconditional, and I think it's very silly that go vet hangs itself up on this. I would've never noticed any friction from go vet and never had a single jot of irritation from the invocation of go vet on repos I've experienced it on... except for this rule about composites.

As far as making this a shared YAML template: I think as desirable as that may be, there's also a limit of the feasibility of it. (ISTM there's a natural "law" of sorts: the more rules a template enforces, the harder it is to adopt and apply to existing projects; and if the only way to adopt something incrementally is by forking it, then that is how it will be done.)

It's usually my reaction to a situation like that to just accept it and jump to the natural end state, which in this case is to assume there's a core of recommendations, in practice it is always forked from, and that the best we are likely to do is periodically expend effort to re-anneal as many projects as possible as close to the core recommendation as is time-cost-effective.

(If there were a way to perform that annealing mechanically, or even monitor it -- tolerating divergences while tracking and long-term minimizing them -- I would find that quite appealing. But it seems that to do this when the format of exchange is YAML, is... well, I don't know. I will only observe that I have yet to see this done.)

So I'm not entirely bullish on the idea of one totally uniform template in the first place. It's good to try, but I don't know if it's worth losing much sleep over variations.

And if I was bullish on trying to force one totally uniform template: then I would say including go vet in the template makes adoption more uphill, and may be unwise if I wanted that uniform template to succeed.

Before I reply, I just want to reiterate that I personally don't mind what policies you follow on your own projects. Projects which have multiple developers, and where other people will also maintain the code in the long run, are an entirely different matter. I personally am much more strict on my personal projects with extra tools and checks, but in general with shared code I only expect or suggest the standard stuff.

if go vet didn't make it an issue.
if I cared what go vet did at all

go vet is a pretty much universally followed tool. Developing Go code while consciously not following its advice feels like consciously not following gofmt. I would again encourage you to file bugs if you think part of its design is flawed, but the tool is optional in the same way that following gofmt is optional.

Every other go vet rule looks for actual bugs; this one rule does not.

The tool is documented to be about finding real or potential bugs. Like you mention later, not being explicit across package boundaries can result in code that is harder to understand, or compilation errors if the types gain a new field, which is a backwards compatible change - as long as you follow this rule.

I think there's a reason that golang syntax allows unnamed composite constructions, and I think the go vet tool overreaches significantly by deciding to poo-poo that entire language feature.

Unkeyed composite literals are allowed if the type is defined in the same package. If the feature was entirely banned by vet, why would the feature exist?

Zot{foo: foo, bar: bar, baz: baz}

A constructor would normally be in the same package that defines the type, so it should not be mandatory here. As for the other cases, I mostly see key-value pairs like Name: "foo" or Err: fmt.Errorf(...); the simple Foo: foo repetition is very rare unless you're writing already-verbose boilerplate.

If it was possible to use named initializers and annotate that the compiler should reject this composite if any fields remain uninitialized, perhaps I would regard this differently; but there is no such ability.

I agree, and that could exist as a static analysis tool at a similar level as vet.

Neither of these behaviors is always the desired one, but in my opinion, the latter scenario (aka, "please, compiler, tell me what I need to update when I make changes") predominates.

This is what I meant by "exactly opposite ways" of developing Go. Look at https://golang.org/doc/go1compat#expectations:

Struct literals. For the addition of features in later point releases, it may be necessary to add fields to exported structs in the API. Code that uses unkeyed struct literals (such as pkg.T{3, "x"}) to create values of these types would fail to compile after such a change. However, code that uses keyed literals (pkg.T{A: 3, B: "x"}) will continue to compile after such a change. We will update such data structures in a way that allows keyed struct literals to remain compatible, although unkeyed literals may fail to compile. (There are also more intricate cases involving nested data structures or interfaces, but they have the same resolution.) We therefore recommend that composite literals whose type is defined in a separate package should use the keyed notation.

I realise these are rules for Go's compatibility itself, and they say "recommend", but I want to make it clear that out of the two options you list, pretty much all existing Go code follows this document and vet. And since IPLD mainly produces libraries, I don't think it's a good idea to have a different notion of backwards compatibility - even if it's just for "can we add a field to a struct without bumping the major version?".

The sheer existence of the -compositewhitelist flag seems to be a tacit admission that this rule is not universally applicable

I think you're reading too much into these internal flags; I maintained vet for a couple of years, and I've seen many people use vet, and I've never seen anyone use those flags. There are also other flags like -shadowstrict and -unusedfuncs, and that does not mean the checks are of worse quality. If the check runs by default, it means it's generally believed to have no false positives and should be followed.

So I'm not entirely bullish on the idea of one totally uniform template in the first place. It's good to try, but I don't know if it's worth losing much sleep over variations.

The first version is indeed just copying YAML files. There are some YAML template engines but they're all... either too simple, or too insane. I have future plans to make this all more composable with Cue, but that's a story for another day. The first version is indeed just having workflows for the "must" basics. If someone wants extras, they can create another workflow, or manually maintain their modified template. But it's not intended that they would choose a subset of the "must" category; otherwise this doc would be wrong.

I would say including go vet in the template makes adoption more uphill, and may be unwise if I wanted that uniform template to succeed.

I honestly hope that go vet isn't as controversial throughout the rest of PL as it is in IPLD :)

For me, the value of forbidding composite literals is not so much about adding more fields in the future, but about rearranging fields. Struct fields might be reordered to improve memory alignment or just for clarity / documentation purposes. Consider the following example:

type Employee struct {
	Age int16
	Salary int32
}

From an external package, you could now initialize a 35 year old employee making a 100k as Employee{35, 100000}. Now every update of this package comes with a risk: If Age and Salary are reordered, you'll now have a 100000 year old employee making 35 bucks.

Now one could argue that this is a backwards-compatibility breaking change and semver should make sure that changes like these only happen in between major releases. As go vet is a de facto standard, I'm not sure though if most developers would categorize this as a breaking change though, so you can't really trust on version numbers to save you here. Furthermore, just forcing myself to never use unkeyed composite literals, although slightly increasing the verbosity of constructor functions, makes sure that I never have to worry about this entire bug class again. Sounds like a worthwhile tradeoff to me.

In other cases, it is desirable to have the compiler forcibly inform me that I need to update other reference sites. Use of unnamed composite initializers produces the latter case.

FYI, there's a linter for that: https://github.com/mbilski/exhaustivestruct. I don't think I'd want a linter like that in most of my projects, but I can see how it might be useful for some special cases.
As @mvdan mentioned, in a future iteration the YAML workflow will allow projects to add additional steps to the standard workflow. For now, the recommended setup would be a separate GH actions workflow.

Thinking outloud a bit, in terms of what one might propose to upstream vet: @warpfork would it be reasonable to you if the restriction on unkeyed literals was extended from "OK for types defined in the same package" to "OK for types defined in the same module"?

In theory, that has a lower amount of danger compared to types defined in different modules, because a module is versioned and released as a single unit. And, in general, one does run go test ./... across an entire module after each change.

One potential downside is large modules, like monorepos, where backwards compatibility across packages within the same module still matters a lot. That's the main reason I think such a vet change might not be a good idea. But one could make the argument that those large modules would still run go test ./... and catch broken code, assuming decent test coverage. And perhaps there could be a flag like "make the composite check more aggressive" that they could opt into.

The idea above is now a proposal: golang/go#43864

@warpfork I hope I was clear with my points above. We can continue this discussion in the future if you want - for now, see the issue above for an actionable item upstream.

I'll merge this, as it's got two approvals and we should move forward. I do assume we'll treat the strong recommendations as a "must", as per the above :)