doc: specify grammar of Deprecated convention

[dsnet]

This is a follow-up to #10909, which defined the convention as follows:

To signal that an identifier should not be used, add a paragraph to its doc comment that begins with Deprecated: followed by some information about the deprecation, and a recommendation on what to use instead, if applicable.

This specifies that the convention requires the addition of at least one paragraph, but fails to specify how subsequent paragraphs are to be treated (i.e., are they considered part of the deprecation notice or part of the "regular" documentation). The phrase "followed by some information" is ambiguous as to whether it applies only to the paragraph added or includes subsequent paragraphs.

This has come up with the implementation of tools trying to automatically surface deprecation notices to users during code review.

An example of this:

// ClearAllExtensions clears all extensions from m.
// This includes populated fields and unknown fields in the extension range.
//
// Deprecated: Use RangeExtensions instead to clear all populated extension fields:
//
//	proto.RangeExtensions(m, func(xt protoreflect.ExtensionType, _ interface{}) bool {
//		proto.ClearExtension(m, xt)
//		return true
//	})
//
// The example rewrite above does not clear unknown fields in the extension range,
// which is unlikely to matter in practice.
func ClearAllExtensions(m Message) { ... }

In this example, the intent of the deprecation notice is to include all subsequent paragraphs and code blocks as part of the deprecation notice. Should they be considered part of the deprecation message or not? At the present moment, the tool we have only surfaces:

Use RangeExtensions instead to clear all populated extension fields:

in code review since it only treats the single paragraph as the deprecation warning. The fact that the code block suggesting the alternative is not shown is unfortunate.

\cc @stapelberg @bsiegert @dmitshur @neild @FiloSottile

[dsnet]

I should note that the wiki has an example for rc4 as:

// Package rc4 implements the RC4 stream cipher.
//
// Deprecated: RC4 is cryptographically broken and should not be used
// except for compatibility with legacy systems.
//
// This package is frozen and no new functionality will be added.
package rc4

In that example, the second paragraph with "This package is frozen and no new functionality will be added." seems clearly related to the deprecation messaging.

[bcmills]

CC @ianthehat @matloob @jayconrod

[bcmills]

I think we should generally present only the paragraph containing the Deprecated tag — in the general case I think we we want something that fits nicely in an on-hover tooltip in an editor, that could be presented along with a link or prompt to view the full comment (CC @stamblerre).

So I would argue that the ClearAllExtensions comment should be rewritten, perhaps as:

// ClearAllExtensions clears all extensions from m.
// This includes populated fields and unknown fields in the extension range.
//
// Deprecated: Use ClearExtensions with RangeExtensions to clear only the
// populated extension fields without also clearing unknown fields.
//
// For example:
//
//	proto.RangeExtensions(m, func(xt protoreflect.ExtensionType, _ interface{}) bool {
//		proto.ClearExtension(m, xt)
//		return true
//	})

[dmitshur]

This specifies that the convention requires the addition of at least one paragraph, but fails to specify how subsequent paragraphs are to be treated (i.e., are they considered part of the deprecation notice or part of the "regular" documentation). The phrase "followed by some information" is ambiguous as to whether it applies only to the paragraph added or includes subsequent paragraphs.

I agree with this problem statement. We should resolve the ambiguity.

I'm not yet sure what resolution is better; I'll take more time to think about it. But I do think starting off with what @bcmills suggested above is safer, as it's more viable to expand it from 1 paragraph to multiple (if we find that it's worth doing), than the inverse.

We can gather how the deprecation notices are written in the wild (e.g., find instances in the module mirror) to help inform what solution is better. In other words, are there deprecation notices that really need to be multiple paragraphs and cannot be shortened into one paragraph?

In that example, the second paragraph with "This package is frozen and no new functionality will be added." seems clearly related to the deprecation messaging.

The intention of that paragraph may have been the opposite of how you perceived it.

It was added in commit 0e28dc19 by @FiloSottile, and based on that "The paragraph does not have to be the last paragraph in the doc comment." was added at the same time, it appears to intend to show there is allowed be additional unrelated text after the deprecation notice. Hopefully @FiloSottile can confirm or correct me if I misunderstood the intention (it really can go either way).

If the understanding above is accurate, perhaps the example can be improved by changing the "This package is frozen and no new functionality will be added." text to something completely different, to avoid this confusion and overlap with the frozen convention (a related, but not the same convention, documented at https://golang.org/wiki/Frozen).

[dsnet]

I agree that @bcmills' refactor is better in that it captures more information in the first paragraph. I've since modified our documentation to do likewise.

I'm in the process of migrating a widely used package and I got several users stating that they found the deprecation message unhelpful and wanted actual code examples, which I then added. It's unfortunate that the suggested code rewrite isn't currently surfaced in code review tooling if the definition is restricted to only the first paragraph (since code blocks cannot possibly be in the first paragraph).

[dmitshur]

Another way of resolving this is to continue to treat the deprecation convention as providing only a single bit of information: either the field, function, type, or package is deprecated, or it is not. I think that was the scope and intention of #10909.

This would mean tools could display the entire comment, and not try to extract only the "deprecation notice" segment from it (which becomes problematic because of the reasons described in the original issue).

I'm not sure if this is a better resolution, but it should be considered too.

[dmitshur]

Also /cc @dominikh who implemented a check that detects deprecated identifiers.

[dmitshur]

It doesn't seem that the path to resolution is known yet, as there is still ongoing discussion in this issue, so moving back to NeedsDecision state.

[dominikh]

Also /cc @dominikh who implemented a check that detects deprecated identifiers.

The addition of "The paragraph does not have to be the last paragraph in the doc comment." caught me off-guard. Staticcheck currently only checks the last paragraph in a comment.

Concerning this issue, I agree with @bcmills's comment (#38743 (comment)) – there are many contexts in which only a single paragraph can reasonably be displayed. For example in staticcheck, I would not want to output multiple lines of text and example code and whatnot. We print a single line containing "the deprecation message".

In that vein, I would say that "the deprecation message" consists of only the paragraph that is introduced by Deprecated: ; and that more information related to the deprecation may follow in additional paragraphs, but the deprecation message should be able to stand on its own – in particular it should end with a period.

[dsnet]

In that vein, I would say that "the deprecation message" consists of only the paragraph that is introduced by Deprecated: ; and that more information related to the deprecation may follow in additional paragraphs, but the deprecation message should be able to stand on its own – in particular it should end with a period.

I like this definition. It 1) clearly identifies all the information that is deprecation, 2) specially highlights the first paragraph as sort of the top-level summary and leaves it up to the tools whether to only show the first paragraph (which can still stand on its own) or all subsequent paragraphs.

[stapelberg]

I'm in the process of migrating a widely used package and I got several users stating that they found the deprecation message unhelpful and wanted actual code examples, which I then added. It's unfortunate that the suggested code rewrite isn't currently surfaced in code review tooling if the definition is restricted to only the first paragraph (since code blocks cannot possibly be in the first paragraph).

A pragmatic suggestion: include a link to a code example in the deprecation notice. Even if the tooling doesn’t make them clickable, it’s easy enough to copy&paste them, and when they’re short, they take up much less space than an embedded example.