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: go/format: simplify old directive comments #68061

Open
jimmyfrasche opened this issue Jun 19, 2024 · 8 comments
Open

proposal: go/format: simplify old directive comments #68061

jimmyfrasche opened this issue Jun 19, 2024 · 8 comments
Labels
Milestone

Comments

@jimmyfrasche
Copy link
Member

Proposal Details

#51082 standardized Go directive comments. #37974 extended the syntax to allow for third party directives.

This left three old directives with custom formats: line, export, and extern.

#38785 agreed to treat //export and //go:export as synonyms, though this did not go through the proposal process, was never implemented, and did not include line and extern.

We should revive and expand this to all three:

For line (and similarly for export and extern)

  1. Accept both //line and //go:line in go1.N
  2. Have gofmt rewrite //line to //go:line when the module language version >= go1.N
@jimmyfrasche jimmyfrasche added this to the Proposal milestone Jun 19, 2024
@jimmyfrasche
Copy link
Member Author

cc @griesemer @adonovan @findleyr @lfolger from the related #68021

@adonovan
Copy link
Member

adonovan commented Jun 19, 2024

Though I would like to live in the world where the proposed conversion process has completed, it does seem like quite a lot of effort for a marginal improvement, versus living with a few legacy Namespace="" directives.

@jimmyfrasche
Copy link
Member Author

While that's the motivation that led me here, the simplification goes further into having a single simple rule without any special cases to remember. That makes things easier to learn and easier to use. Use covers a lot: writing a directive, using the iterator in the related issue, or adding the pattern to a syntax highlighter. It is minor and perhaps it is more trouble than its worth but may as well weigh that before locking it in further.

@lfolger
Copy link
Contributor

lfolger commented Jun 19, 2024

Maybe I'm missing something but unless you fully deprecate the old forms (which seems very costly), you still have to special case and remember them, don't you?

@jimmyfrasche
Copy link
Member Author

(I'm going to stick with line for discussion but everything applies to the other two as well.)

Making line and go:line synonyms allows two simplifications in tooling

  • a parsed line can be reported as being in the go namespace since it's the same as go:line
  • rendering a parsed line directive back to string doesn't need to handle the special case and can output go:line regardless of how it was originally written

Though minor those simplifications mean no one using go/ast to examine directives needs to worry about the special case. It's handled transparently.

Having gofmt rewrite line to go:line means that instances in the wild will die out so after some years effectively only go:line would exist and only the general directive format would need to be learned.

The code for handling line would still be there but it would be an implementation detail that essentially no one would need to worry about in practice. Even if the old form was entirely removed in newer versions of Go the code would still need to be there to handle compiling old codebases with a -lang from before the change, so it wouldn't really be possible to 100% remove it anyway.

@lfolger
Copy link
Contributor

lfolger commented Jun 20, 2024

This makes sense but only if #68021 is accepted because otherwise the only way to retrieve line directives is to actually parse the raw comments. Or are you suggesting to also change the values of the raw comments?

@jimmyfrasche
Copy link
Member Author

Part 2 of the proposal is to have gofmt rewrite X → go:X (for modules with a language version after they become synonymous). It already moves comment directives so it's not entirely unprecedented and these directives are defined by Go so it is safe to rewrite them.

I think it's important to settle this matter first as #68021 locks the format into the compatibility agreement more tightly than it is now. If the iterator goes in and then later it's decided to do this it would be unfortunate if for compatibility reasons //go:line had to be reported as the empty namespace or, worse, the namespace depended on how it was authored even though they mean the same thing.

@apparentlymart
Copy link

I agree that it would be nice to live in a world where this were consistent, and share the concern that it seems somewhat disruptive to get there.

I find myself considering a variation of this proposal that might be less disruptive:

  1. Support the go:-prefixed and unprefixed forms as synonyms as currently proposed, but...

  2. ...make the tools treat the unprefixed forms as the normalized form, to avoid churning existing codebases that already use the old form.

    (In particular, gofmt would rewrite //go:line to just //line, rather than the other way around.)

This variant means that anyone hand-writing code still doesn't need to remember the rule -- they can just always write a go: prefix and let the formatter "correct" it if it's one of the legacy cases -- but the formatter would not unilaterally rewrite all of the existing code in older codebases.

I think this would also help avoid mistakes where someone uses a new toolchain to work on a codebase that's intended to still work on older Go versions and ends up with the comments rewritten into a shape that would not be accepted (possibly silently ignored?) by other toolchain versions. (Though I understand that the rule about using the version in go.mod was intended to mitigate that, and would probably do so well enough if this were the only drawback.)

Of course, it does mean that the exceptions must remain exceptional forever. This alternative does not lead to a future where the unprefixed forms might become invalid altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants