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: inconsistently requires ParenExpr #43598

Open
dominikh opened this issue Jan 8, 2021 · 1 comment
Open

go/printer: inconsistently requires ParenExpr #43598

dominikh opened this issue Jan 8, 2021 · 1 comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dominikh
Copy link
Member

dominikh commented Jan 8, 2021

go/printer is inconsistent in handling the absence of ParenExpr. First, let me show an example that automatically inserts required parentheses, despite no ParenExpr existing in the AST:

// (- a (- b c))
&ast.BinaryExpr{
	X:  &ast.Ident{Name: "a"},
	Op: token.SUB,
	Y: &ast.BinaryExpr{
		X:  &ast.Ident{Name: "b"},
		Op: token.SUB,
		Y:  &ast.Ident{Name: "c"},
	},
}

prints as

a - (b - c)

despite the lack of a ParenExpr. I posit that this is correct behavior.

Now, consider this example:

&ast.IfStmt{
	Cond: &ast.BinaryExpr{
		X:  &ast.Ident{Name: "a"},
		Op: token.EQL,
		Y: &ast.CompositeLit{
			Type: &ast.Ident{Name: "b"},
		},
	},
	Body: &ast.BlockStmt{},
}

prints as

if a == b{} {
}

that is, it doesn't insert the necessary parentheses to make the printed form syntactically valid.

I am of the opinion that the second example should produce correct output, inserting parentheses as necessary. go/printer already has code that strips an existing ParenExpr if it is not required (i.e. it turns if (b) into if b), but leaves it intact otherwise. It should similarly be possible to insert missing parentheses, based on the same logic.

Users of gofmt can also run into this issue: the rewrite rule (a) -> a, which is documented as stripping unnecessary parentheses, incorrectly strips parentheses of the if (x == T{}) form.

IMHO, an AST should not require explicit parentheses to be valid, and go/printer should take care of turning a valid AST into valid code. I'd like to go over go/printer and fix all instances of this problem, thus making ParenExpr optional.

/cc @griesemer for his expert opinion

@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 8, 2021
@dmitshur dmitshur added this to the Backlog milestone Jan 8, 2021
@griesemer
Copy link
Contributor

I agree that go/printer should insert parentheses where needed to make the output valid.

@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label May 3, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants