proposal: cmd/gofmt: write official spec for gofmt #18790

Closed
dlsniper opened this Issue Jan 26, 2017 · 9 comments

Comments

Projects
None yet
7 participants
@dlsniper
Contributor

dlsniper commented Jan 26, 2017

Hello,

In light of #18782 I would like to resurface an older discussion about the specifications for gofmt being provided as official specifications.
Alternative implementations of the gofmt tool could be done a lot easier and would allow for having another tool to validate validate against (much like we have gc and gccgo).
Thank you for your consideration and time.

@bradfitz bradfitz changed the title from Official spec for gofmt to proposal: official spec for gofmt Jan 26, 2017

@bradfitz bradfitz added the Proposal label Jan 26, 2017

@bradfitz bradfitz added this to the Proposal milestone Jan 26, 2017

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Jan 26, 2017

Contributor

I fear that an exact specification of gofmt might be more complex than the actual source. There are many heuristic decisions in gofmt (for instance when and when not to break alignment of comments in a long sequence of key:value pairs of map composite literals, etc.) which are relatively easy to express programmatically, but rather problematic in prose.

That said, I agree that gofmt has grown organically and at this point could use a clean rewrite, which might in fact be that specification.

(I've been working on such a rewrite on and off; perhaps it's time to focus on it a bit more diligently.)

Contributor

griesemer commented Jan 26, 2017

I fear that an exact specification of gofmt might be more complex than the actual source. There are many heuristic decisions in gofmt (for instance when and when not to break alignment of comments in a long sequence of key:value pairs of map composite literals, etc.) which are relatively easy to express programmatically, but rather problematic in prose.

That said, I agree that gofmt has grown organically and at this point could use a clean rewrite, which might in fact be that specification.

(I've been working on such a rewrite on and off; perhaps it's time to focus on it a bit more diligently.)

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 26, 2017

Contributor

A spec seems unnecessarily rigid. Gofmt is not a compiler, and we expect gofmt output to drift slightly over time as we fix problems. The underlying failure in #18782 seems to be that the CI system reports a test failure if code is not gofmt'ed correctly. A spec will not fix that: gofmt output will still change in minor ways over time.

Contributor

rsc commented Jan 26, 2017

A spec seems unnecessarily rigid. Gofmt is not a compiler, and we expect gofmt output to drift slightly over time as we fix problems. The underlying failure in #18782 seems to be that the CI system reports a test failure if code is not gofmt'ed correctly. A spec will not fix that: gofmt output will still change in minor ways over time.

@dlsniper

This comment has been minimized.

Show comment
Hide comment
@dlsniper

dlsniper Jan 27, 2017

Contributor

@rsc maybe specifications is the wrong word here and I should have used: "set of rules".

And yes, many people run this against their repositories as part of their CI pipeline but if you don't touch a file you normally don't have a reason to format it so a break in CI like that will come from nowhere. I'm going to save the speech about the importance of gofmt for the Go community but if it continues to slowly drift, as you've said, we might just as well have the rules updated as well when that happen to understand the changes to it.

I've sadly tried many times to come up with the rules myself, by reading the current implementation, but this proved to be a rather hard task which lead nowhere. I definitely appreciate @griesemer's intention to refactor this into a cleaner implementation, maybe that's the best first step to do in this case.

Thank you.

Contributor

dlsniper commented Jan 27, 2017

@rsc maybe specifications is the wrong word here and I should have used: "set of rules".

And yes, many people run this against their repositories as part of their CI pipeline but if you don't touch a file you normally don't have a reason to format it so a break in CI like that will come from nowhere. I'm going to save the speech about the importance of gofmt for the Go community but if it continues to slowly drift, as you've said, we might just as well have the rules updated as well when that happen to understand the changes to it.

I've sadly tried many times to come up with the rules myself, by reading the current implementation, but this proved to be a rather hard task which lead nowhere. I definitely appreciate @griesemer's intention to refactor this into a cleaner implementation, maybe that's the best first step to do in this case.

Thank you.

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Jan 27, 2017

Member

@griesemer to clarify, are you talking about a clean rewrite of cmd/gofmt, go/printer or both?

Any timeline? Wondering if I should hold off working on #16371 to not conflict with it.

Member

mvdan commented Jan 27, 2017

@griesemer to clarify, are you talking about a clean rewrite of cmd/gofmt, go/printer or both?

Any timeline? Wondering if I should hold off working on #16371 to not conflict with it.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 27, 2017

Contributor

And yes, many people run this against their repositories as part of their CI pipeline but if you don't touch a file you normally don't have a reason to format it so a break in CI like that will come from nowhere.

That's exactly why a mis-formatted file should not break CI.

At Google, we use a presubmit check on the way into the repo, but if we adjust something in gofmt, our CI system doesn't break due to all the existing files. That's really untenable, and I would strongly suggest not doing that. Among other things, if gofmt changes slightly from say Go 1.7 to Go 1.8, it means your CI system can't support testing against both. (It's a good example of a forced atomic code repair.)

If you care a lot that files in the repo match a particular version of gofmt after being submitted, a better approach would be to have an automated process that checks in reformatted files as needed, without bothering people.

Contributor

rsc commented Jan 27, 2017

And yes, many people run this against their repositories as part of their CI pipeline but if you don't touch a file you normally don't have a reason to format it so a break in CI like that will come from nowhere.

That's exactly why a mis-formatted file should not break CI.

At Google, we use a presubmit check on the way into the repo, but if we adjust something in gofmt, our CI system doesn't break due to all the existing files. That's really untenable, and I would strongly suggest not doing that. Among other things, if gofmt changes slightly from say Go 1.7 to Go 1.8, it means your CI system can't support testing against both. (It's a good example of a forced atomic code repair.)

If you care a lot that files in the repo match a particular version of gofmt after being submitted, a better approach would be to have an automated process that checks in reformatted files as needed, without bothering people.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Jan 27, 2017

Contributor

@mvdan The compiler (cmd/compile) already generates a new syntax tree during parsing (cmd/compile/internal/syntax) and that package also contains a printer. That printer is very basic at the moment, but there are hooks to make it more complete (hooking up comments with nodes so that they are printed at the right places). That said, you should not stop on #16371; rewriting gofmt is not a priority at this point. If it happens it happens, but it will have to be as close to the existing gofmt as we can make it (for a start at least), and everything that we fix in gofmt will serve as guideline for any future implementation.

Contributor

griesemer commented Jan 27, 2017

@mvdan The compiler (cmd/compile) already generates a new syntax tree during parsing (cmd/compile/internal/syntax) and that package also contains a printer. That printer is very basic at the moment, but there are hooks to make it more complete (hooking up comments with nodes so that they are printed at the right places). That said, you should not stop on #16371; rewriting gofmt is not a priority at this point. If it happens it happens, but it will have to be as close to the existing gofmt as we can make it (for a start at least), and everything that we fix in gofmt will serve as guideline for any future implementation.

@rsc rsc changed the title from proposal: official spec for gofmt to proposal: cmd/gofmt: write official spec for gofmt Jan 30, 2017

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 30, 2017

Contributor

For the reasons above, we're declining this proposal. The CI problem should be solved other ways (and isn't solved by a spec unless it is completely frozen).

Contributor

rsc commented Jan 30, 2017

For the reasons above, we're declining this proposal. The CI problem should be solved other ways (and isn't solved by a spec unless it is completely frozen).

@phayes

This comment has been minimized.

Show comment
Hide comment
@phayes

phayes Nov 9, 2017

I'd like to offer an alternative use-case for having an official, human-readable spec for gofmt.

We are looking at using Go to built certified voting systems that conform with the VVSG (VOLUNTARY VOTING SYSTEM GUIDELINES). Despite the name, these guidelines are anything but voluntary in most US States.

Section 5.2.3 of VVSG 1.1 is as follows:

Application logic shall adhere to a published, credible set of coding rules,
conventions or standards (herein simply called the “coding standard”) that
enhance the workmanship, security, integrity, testability, and maintainability of
applications. Coding standards that are excessively specialized or simply
inadequate may be rejected on the grounds that they do not enhance one or more
of workmanship, security, integrity, testability, and maintainability.

Coding standards shall be considered published if and only if they appear in a
publicly available book, magazine, journal, or new media with analogous
circulation and availability, or if they are publicly available on the Internet.

...

Coding standards shall be considered credible if and only if at least two different
organizations with no ties to the creator of the rules or to the manufacturer
seeking conformity assessment, and which are not themselves voting equipment
manufacturers, independently decided to adopt them and made active use of
them at some time within the three years before conformity assessment was first
sought.

As it currently stands, the lack of a gofmt specification seems to be the only thing in our way to using Go as a VVSG compliant programming language. I'm sure that the VVSG isn't the only standard for critical software that requires documented, human-readable specifications for formatting standards.

Note that I think we could gloss over some of the more complicated logic that gofmt performs with generic phrases such as "Formatted code MAY opt to align comments in order to improve legibility" (for example).

phayes commented Nov 9, 2017

I'd like to offer an alternative use-case for having an official, human-readable spec for gofmt.

We are looking at using Go to built certified voting systems that conform with the VVSG (VOLUNTARY VOTING SYSTEM GUIDELINES). Despite the name, these guidelines are anything but voluntary in most US States.

Section 5.2.3 of VVSG 1.1 is as follows:

Application logic shall adhere to a published, credible set of coding rules,
conventions or standards (herein simply called the “coding standard”) that
enhance the workmanship, security, integrity, testability, and maintainability of
applications. Coding standards that are excessively specialized or simply
inadequate may be rejected on the grounds that they do not enhance one or more
of workmanship, security, integrity, testability, and maintainability.

Coding standards shall be considered published if and only if they appear in a
publicly available book, magazine, journal, or new media with analogous
circulation and availability, or if they are publicly available on the Internet.

...

Coding standards shall be considered credible if and only if at least two different
organizations with no ties to the creator of the rules or to the manufacturer
seeking conformity assessment, and which are not themselves voting equipment
manufacturers, independently decided to adopt them and made active use of
them at some time within the three years before conformity assessment was first
sought.

As it currently stands, the lack of a gofmt specification seems to be the only thing in our way to using Go as a VVSG compliant programming language. I'm sure that the VVSG isn't the only standard for critical software that requires documented, human-readable specifications for formatting standards.

Note that I think we could gloss over some of the more complicated logic that gofmt performs with generic phrases such as "Formatted code MAY opt to align comments in order to improve legibility" (for example).

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Nov 9, 2017

Contributor

@phayes I don't think that gofmt is a set of coding rules such as you describe. I think that what you are looking for is more like https://golang.org/wiki/CodeReviewComments .

Of course, I could be wrong about that, but I still don't see a spec for gofmt as appropriate. Sorry.

Contributor

ianlancetaylor commented Nov 9, 2017

@phayes I don't think that gofmt is a set of coding rules such as you describe. I think that what you are looking for is more like https://golang.org/wiki/CodeReviewComments .

Of course, I could be wrong about that, but I still don't see a spec for gofmt as appropriate. Sorry.

@golang golang locked and limited conversation to collaborators Nov 10, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.