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: can print an invalid return statement depending on position info #32854

Closed
mvdan opened this issue Jun 29, 2019 · 9 comments

Comments

@mvdan
Copy link
Member

commented Jun 29, 2019

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

go version devel +2e0cd2aef5 Fri Jun 28 20:10:05 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

https://play.golang.org/p/oxljScGL89r

What did you expect to see?

A valid program, like:

func f() {
	return call()

}

What did you see instead?

The return call() being printed separately, which means two statements instead of one.

func f() {
	return
		call()

}

To make this more obvious, gofmt formats the last piece of code as:

func f() {
	return
	call()

}

I encountered this while writing a program to modify source code. Surprisingly, I ended up with code that wouldn't compile, and figured out it was because the printer wasn't doing the right thing.

I assume this is a small bug that can be fixed, such as not allowing a newline in this case. I'll investigate a bit. /cc @griesemer

@mvdan mvdan added the NeedsFix label Jun 29, 2019
@mvdan mvdan self-assigned this Jun 29, 2019
@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

I realise that this could qualify as "abusing" the syntax tree nodes. However, I don't see why go/printer should ever write incorrect or different code. The only alternative would be to write lots of code to always make sure that the positions are OK, which I'm not sure is possible.

@av86743

This comment has been minimized.

Copy link

commented Jun 29, 2019

Conceivably, your updated AST is not a valid one. Perhaps you can verify this using one of the AST library predicates/validators?

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

I'd disagree. Even if you print a syntax tree with an empty fileset, the printer should still print a valid program, even if the style ends up looking weird.

For example, other similar statements like go and defer don't have this same bug, so I think this is simply human error. The fix is also trivial - I'm finishing the test now.

@av86743

This comment has been minimized.

Copy link

commented Jun 29, 2019

So you are unable to verify whether your generated AST is valid?

@gopherbot

This comment has been minimized.

Copy link

commented Jun 29, 2019

Change https://golang.org/cl/184121 mentions this issue: go/printer: never print a newline before the returned results

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

Sorry, I don't know what you mean. The syntax tree is perfectly valid. The position information obtained via the fileset is a bit weird, but still, I don't think that should lead to the printer producing incorrect output and no error.

I also don't know what validators you're talking about. You can validate the syntax tree example I gave above, if that helps.

mvdan added a commit to mvdan/gogrep that referenced this issue Jun 29, 2019
See golang/go#32854 for the issue reported for
upstream.
@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

@mvdan I agree this is a bug. Unfortunately, the go/printer can produce incorrect formatting in some cases.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

I understand these edge cases are super rare, primarily because one needs to craft a syntax tree manually. It's impossible for gofmt to run into this bug, I think. In any case, I'm happy to send patches like this one if you're happy to review them.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

@mvdan I can review them but it's not a high priority. The shorter the better, of course :-)

@mvdan mvdan added this to the Go1.14 milestone Sep 19, 2019
@gopherbot gopherbot closed this in 211932b Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.