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: Go2: arg tags for marking const, other properties; use in vet #33122

Closed
rcoreilly opened this issue Jul 15, 2019 · 4 comments
Closed
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Milestone

Comments

@rcoreilly
Copy link

This proposal is to adopt the concept of struct field tags for function args, to allow various properties to be marked, including const, so that tools such as vet could be used to find cases where a read-only value is being written.

In the discussion of the various const-like proposals, e,g., #27975 and #22876, the consistent feedback has been that const-poison is a major problem (I certainly suffered from this many times in C++) and that const-ness is one of multiple possible properties one might want to mark.

A potential conclusion is that const-ness must be optional, and a more open-ended syntax is needed to enable various properties to be marked. A logical approach within the existing Go toolkit is to adopt the open-ended tags for struct fields to function arguments:

func MyFunc(a *Astruct `const:"+"`, name string) {
    ...
}

vet could parse those arg tags, trace through all operations on an arg marked as const, and find any places where it is actually being written to. This would avoid const poison, have no performance overhead, enable const-ness to be asserted flexibly at any point, and support the debugging aspect of const-ness. This is analogous to how the race detector is used to find bugs, instead of somehow preventing races through the language itself.

This would obviously not solve the performance advantages of const-ness, such as the no-copy conversion between []byte and string, but perhaps the debugging aspect is the most important?

This would also work well with #33088, which allows const vs. non-const to be decorated at the calling site, and vet could also be used to flag violations where the & operator was used to call a non-const arg or vice-versa, or even without this proposal it could independently check whether an arg passed without the & operator was written to.

@gopherbot gopherbot added this to the Proposal milestone Jul 15, 2019
@ianlancetaylor ianlancetaylor added v2 An incompatible library change LanguageChange Suggested changes to the Go language labels Jul 15, 2019
@ianlancetaylor
Copy link
Contributor

Similar to #24889.

@rcoreilly
Copy link
Author

Indeed, essentially identical. I can see the objection of adding lots of "clutter". Just for completeness, it might be worth considering an alternative organization such as having the tags just preceding the function:

// MyFunc does some things
`a:"const" `name:"no-nil"`
func MyFunc(a *Astruct `const:"+"`, name string) {
    ...
}

this makes the tags fit in visually with the comments as a form of meta docs, and keeps the signature itself clean and easily seen.

A major drawback is that it requires the arg names to be typed twice and sync'd -- if you change one, you'll have to remember to change the other. Vet could presumably catch those errs though.

@beoran
Copy link

beoran commented Jul 22, 2019

#24889 was actually a good idea, the problem now is that the syntax for struct tags is not very good. #23637 proposes to fix this. I'd say that if the latter gets implemented, then we can reconsider the former issue and see if it isn't less cluttered now due to the better syntax.

@ianlancetaylor
Copy link
Contributor

Closing as dup of #24889.

@golang golang locked and limited conversation to collaborators Oct 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests

4 participants