go-tools
go-tools copied to clipboard
dominikh
Exported package members without a comment do not trigger a warning
I used to see warnings in VSCode (I'm pretty sure I was using staticcheck), something among the lines of "Exported type should have comment or be unexported", but I noticed I stopped seeing them. I've googled a lot but haven't found a solution to this, so here I am, writing this issue.
Example code:
func Upload(regionID string) error {
// some code that really deserves a comment
}
Expected result: should show a warning "Exported type should have comment or be unexported" Actual result: no warning
Staticcheck does not flag missing comments, and never has. The tool that previously did so in VSCode was golint, which has been deprecated.
We don't intend to start flagging missing comments. I don't believe that it is a check that can be implemented in a way that satisfies our requirement for near-zero false positives; many identifiers are not worth documenting, and IMO golint has caused tremendous amounts of noise because of this check.
We do have optional, disabled by default checks that enforce proper comment style for when there already are comments, however (ST1000, ST1020-ST1022)
Thanks for your response. Is it possible that the rule for comments on every exported identifier will be added (as optional, of course)? I may be weird, but I liked golint and its yelling about missing comments.
Is it possible that the rule for comments on every exported identifier will be added (as optional, of course)?
It's unlikely. Even optional checks need to have low rates of false positives. Golint already tried to address this by, for example, whitelisting String methods without comments, but it's impossible to maintain an exhaustive list of such names. It's not a check I'd be comfortable having.
I understand. This makes sense after giving it some thought. I'll fall back to yelling at my teammates to comment code.
And thanks for creating staticcheck, it's really great!
@dominikh While I appreciate the effort to keep the false positive rate as low as possible, I don't agree that those are false positives.
We really want to use staticcheck to replace golint, and we thought staticcheck with all checks is a superset of golint, but this is the only exception we found (so far), and we don't want to bring back golint just for this check.
If we provide a PR to add new rules for this, and make them non-default, without any allowlist (we are totally fine with writing String implements fmt.Stringer. doc comments), will you be willing to accept it? Being "forced" to write some not-so-useful doc comments is still better than missing doc comments, IMHO.
We really want to use staticcheck to replace golint, and we thought staticcheck with all checks is a superset of golint, but this is the only exception we found (so far), and we don't want to bring back golint just for this check.
Staticcheck is not a superset of golint, and IIRC this isn't the only such instance.
If we provide a PR to add new rules for this
This isn't an issue of effort on my end, but of conviction. I think that these were the worst checks in golint and produced predominantly noise, and to a lesser degree useless comments. Even if the checks are disabled by default, too many people will accidentally enable them.
Being "forced" to write some not-so-useful doc comments is still better than missing doc comments, IMHO.
That is where we disagree.
I'm sorry that this is causing you additional work, but I'm unlikely to change my opinion on the matter.
On second thought I tend to agree with you. If the doc comment is only Foo implements Bar. when you are forced to write it, then not writing anything is not a big loss.
It may seem pointless to write code documentation for code that appears to be obvious but no code is obvious at all. I see code comments that just describe what the method name says or the code below it defines.
Documentation is great place to write down why this code exists in the first place, what is its purpose and why do we need it. Not having this check available is a missed opportunity in my eyes.
I agree that it should not be part of the default rules that are being applied but why not have it as an optional rule? :thinking:
but no code is obvious at all
I disagree. Plenty of code is obvious. A String() string method doesn't need a comment to explain that it implements the Stringer interface. The same goes for Error() string and the error interface. There are many such instances of boilerplate code that is obvious to anyone who has written Go for more than a day. Having to document these is an unnecessary nuisance, and failing CI because of it is a productivity killer.
but why not have it as an optional rule?
The primary reason is that people accidentally or in poor judgement enable all checks unconditionally, without being fully aware what the checks are, nor guarding against new checks being added in the future. In that sense, optional checks have to be "harmless enough if enabled accidentally". And since I so vehemently disagreed with this check existing in golint, it doesn't qualify as "harmless" to me. Yes, this is purely a judgement call.
This is your tool, I respect your decision on that and there are plenty of other linters that can enforce that type of policy.
Having worked with people who are new to Go or still learning I can only vouch that even seemingly simple interfaces could do with a comment. Just think of folks coming over from Java who have a very different understanding of how interfaces work. I only started to appreciate single-method interfaces many months into learning Go.
The code documentation does not need to exist to teach concepts of a programming language but I think saying boilerplate code does not require documentation is an excuse for not being able to come up with good documentation.
Boilerplate code by the way, should probably be code-generated and then indeed, you do not need documentation.
I just ran into a deploy issue because I naively had swapped golint out for staticcheck on the Go team's recommendation. Turns out it's not a superset (which is fine), but not only that I can't even enable this rule.
If the doc comment is only Foo implements Bar. when you are forced to write it, then not writing anything is not a big loss.
This is an extremely useful doc comment because it tells the user where they can go for more information (the interface that this method implements). This was exactly the case in the issue I mentioned: we deployed then a user wasn't sure about how a new feature should be used because the documentation didn't point them to what they needed. Of course, this also should have been caught in code review (oops), but that could be said of almost all static checks.
Please reconsider at least adding this as optional. I do not believe that even the "useless" comments several people have implemented are actually useless; having documentation with absolutely no description of an identifier, no matter how simple, is just bad looking and best practice is to document everything. Just because some people will document it badly doesn't mean the rest of us who try to write exhaustive documentation shouldn't be able to use the check.
having documentation with absolutely no description of an identifier, no matter how simple, is just bad looking
In that case you probably dread working with the Go standard library, which has plenty of trivial and undocumented identifiers.
Nevertheless, in light of all the golint refugees, I have reopened the issue and will reconsider it as an optional check.
There is also a good chance that this fits into #1102.
I am currently leaning towards adding a check for missing documentation, with the following constraints: it'll only be part of the planned codereview category of checks, and it will be opt-in.
Being in codereview means it will lead to no CI failures, only hints during code review of the kind "hey, should this be documented?", which can be dismissed easily. And the opt-in at least reduces (albeit not eliminates) the number of people who accidentally enable the check. And even if they do enable it accidentally, the phrasing of the diagnostic will be suggestive, not declarative, so people will be more likely to disagree with it and figure out how to disable it.
See #1102 for details on the codereview category and the envisioned UI.
Thank you for reconsidering this. Would it be possible to force it to cause a CI failure? General best practices are to document everything and I suspect you'll find many people who won't want to let any undocumented methods slip through (well documented methods, of course, remains an issue for code review). golint for example had the -set-exit-code (or something a long those lines) flag to enable this for what would otherwise be warnings.
Would it be possible to force it to cause a CI failure?
I cannot stop you from running the checks that are meant for code review in your CI, nor can I stop you from exiting non-zero from a script if it detects output from Staticcheck.
However, I will not make these first class features of Staticcheck. Adding a codereview check is a concession, accepting that most identifiers should be documented and that it's something worth bringing up in code review. But I still fundamentally disagree with the notion that "every exported identifier has to be documented", and in my experience it leads to worse documentation on average, across all users. Golint has lead to a swathe of bad documentation, trying to circumvent it.
One of my guiding principles in designing Staticcheck is to avoid making suggestions that lead to worse code.
General best practices are to document everything
No, general best practices are to write good documentation. Having to document everything is your interpretation of what good documentation is. I disagree with that interpretation. You may consider that a "disdain for good documentation", although I don't see why disagreeing with you on what good documentation is equals "disdain". Neither of us have the authority to define "good documentation" for everyone. We can merely have our personal opinions. We're allowed to disagree.
nor can I stop you from exiting non-zero from a script if it detects output from Staticcheck
I'd be worried about output detection being brittle, it feels like a hack for something that should just be supported (even if it's supported in a way that requires turning on a feature and throwing a flag, a sort of double safety device.
Having to document everything is your interpretation of what good documentation is
That is not what I said, in fact, I even implied that documenting everything did not lead to good documentation. It is best practice, none the less.
From practical perspective I think adding code-style/docs checks to staticcheck is not the best strategy, it will increase maintenance costs and blur the original focus of "finding actual bugs".
I see this reasonable only as a convenience/performance optimization.
Code style details are also a subject of team agreement (even in this issue there is a disagreement regarding what constitutes helpful docs). In my humble opinion, covering code-style topic would be a huge effort, mostly due to lack of detailed established community standard (which is in turn a great thing about Go's simplicity).
There are plenty of code-style targeted tools available (for example revive for this issue) that can help to enforce particular agreements with automation.
I really like that check, and it's very useful when introducing junior developers to a project, and in my experience it does more good than harm. My vote would be to leave it as a non-default check in non-aggressive staticcheck, but I wouldn't be too upset if it were moved into the aggressive part.
I see this was planned as part of the Staticcheck 2022.2 milestone, but its progress says 0%. So what's the status of this issue? Is it put on hold?
The 2022.2 release has been delayed for various personal reasons. This issue will likely still be looked at and decided upon as part of working on 2022.2 (which will most likely become 2023.1 instead), but I can't give you a timeline.