-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/cover: pragmas treated differently than in cmd/compile #22022
Comments
cc @gri and @mdempsky, who've worked on pragmas fro the compiler side. |
This is a cmd/cover bug. See also #18285 (which I expect led to the current cmd/cover implementation). It is a feature that compiler annotations are not required to appear in godoc output. |
@ianlancetaylor Thanks for clarifying. I'll work on a fix for cmd/cover. |
For what it's worth, I'm not a fan of how lax cmd/compile is in parsing pragmas, and I think it is unfortunate that it makes support in go/ast-based tooling more difficult. I'd be supportive of refining where it's valid to write pragmas. (So far, we've strived for strict backwards compatibility when reworking the lexer/parser.) |
I think some formal standard for pragmas would avoid such bugs in future. I see two main types of pragmas.
If position sensitive pragmas are always placed directly before associated code, then it's easier for AST tools to preserve their relative-positions. Without that, AST tools may end up misplacing them (#17315). Can we standardize along those lines? |
@Dhananjay92 The trick is that we generally do not want these comments to appear in godoc for exported functions, and we do not want to complicate godoc. |
Change https://golang.org/cl/69630 mentions this issue: |
What version of Go are you using (
go version
)?Go 1.9
Does this issue reproduce with the latest release?
Yes.
What did you do?
Suppose we have the following files:
This test passes normally, but fails with coverage enabled.
(NOTE: the field tracking experiment has to be enabled to turn on nointerface. This bug applies to function pragmas in general though; nointerface is just the easiest to observe).
Analysis
The problem is that cmd/compile and cmd/cover (and presumably other tools) treat pragmas differently.
In cmd/compile, pragmas are recognized by the lexer with minimal awareness of surrounding syntax. When lexer sees a pragma, it sets a bit in the parser. When a top-level definition is parsed, whichever pragma bits are set apply to that function. Pragma bits are cleared at the end of each top-level definition. See parser.go. Consequently, the compiler recognizes function pragmas even when there are comments and blank lines between the pragma and the definition it applies to.
In cmd/cover, pragmas are read from the syntax tree produced by go/parser. cmd/cover adds instrumentation to the source code. It removes most comments (since they are difficult to re-emit) but preserves pragma comments that were in the comment block for top-level definitions. Pragmas that were floating above a definition (as in the example above) are assumed not to apply to any specific definition and are moved to the top of the file. See cover.go. Here's the cover output for the example above:
Discussion
I'm not sure whether this should be considered a cmd/compile bug or a cmd/cover bug. It's surprising to me that the compiler recognizes function pragmas even when there are unrelated comments and blank lines between the pragma and the function definition. There aren't many public examples of pragmas used in this way. In cases where this happens, it seems like people are trying to avoid including the pragma in the documentation.
If this is a compiler bug, we should make the compiler consistent with cmd/cover, so that function pragmas must appear in the block of comments immediately above the definition. cmd/cover would not need to change. Existing examples of these floating pragmas would need to be fixed.
If this is considered a bug in cmd/cover, the comment processing functionality would need to be fixed so that function pragmas are attached to the following top-level definition. In general, any tool that rewrites source code while preserving semantics needs to watch out for this.
The text was updated successfully, but these errors were encountered: