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

go/printer: incorrect *Config.Format results with +build comments and the SourcePos mode #64976

Open
cixel opened this issue Jan 5, 2024 · 2 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@cixel
Copy link
Contributor

cixel commented Jan 5, 2024

What version of Go are you using (go version)?

8db1310 (tip, as of time of writing)

What did you do?

Here is an example reproducer: https://go.dev/play/p/sphL42iaX_a

I think it's a pretty obscure issue which requires a few things:

  1. the SourcePos mode enabled
  2. one or more // +build constraints, and no //go:build comments
  3. a change in the AST before the first //+build comment which offsets position such that the printer has to emit a //line directive

As far as I can tell, here's what's going wrong:

  • the call to printer.*Config.Fprint will fully print the given node (a File) and then call fixGoBuildLines to consolidate build directives and translate "plus builds" to the current //go:build format
  • when printing comments, build constraints are special-cased: the current position of the writer is recorded to p.plusBuild, then the comment is written with p.writeString
  • writeString checks if SourcePos is enabled and, if necessary, emits a line directive before writing its arg string to the output buffer

After printing, fixGoBuildLines turns //+build comments into //go:build by iterating the positions in p.plusBuild and parsing the constraints at each:

for _, pos := range p.plusBuild {
	y, err := constraint.Parse(p.commentTextAt(pos))
	if err != nil {
		x = nil
		break
	}
    ...

Because of SourcePos, the offsets recorded to p.plusBuild can incorrectly point to a //line comment, rather than a valid build constraint, so parsing fails, the loop exits, and subsequent plusBuild lines are ignored and the inserted block will be incomplete/incorrect.

What did you expect to see?

  • //+build constraints should have been preserved and/or translated to //go:build
  • I'm not sure if there's any value in a line directive on the build constraint comment -- perhaps that can be omitted.

What did you see instead?

In the reproducer, one of the build constraints is omitted and a seemingly unnecessary line directive is printed ts the top of the file.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 9, 2024
@dmitshur dmitshur added this to the Backlog milestone Jan 9, 2024
@dmitshur
Copy link
Contributor

dmitshur commented Jan 9, 2024

CC @griesemer, @mvdan.

@griesemer griesemer self-assigned this Sep 18, 2024
@griesemer griesemer modified the milestones: Backlog, Go1.24 Sep 18, 2024
@griesemer
Copy link
Contributor

Assigning to me and for 1.24 to make it visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants