x/tools/internal/refactor/inline: don't insert parens/braces unnecessarily

[adonovan]

The inliner uses the go/ast formatter, which automatically inserts parens only when they are required by precedence. However, there are two situations in which the inliner currently inserts brackets unnecessarily.

The first situation is that, when inlining a call to {return expr}, the inliner may replace the CallExpr by (expr) with extra parens. The reason these parens are sometimes needed is that the formatter can only apply its precedence algorithm when it sees the parent node of the new tree. However, the inliner currently works by formatting the "new" tree in isolation, then textually splicing it into the original file in place of the CallExpr, so the formatter does not get to see the context. The inliner will need to reproduce the computation as to whether these parens are required in the caller context.

The reason the inliner uses textual splicing is so that it does not reformat any more of the caller syntax than necessary, and the reason for that is that when formatting a tree that contains nodes from two files, at most one of them can have its comments correctly handled by the formatter because comments are not stored in the tree but in a position-keyed side table. The inliner chooses to preserve comments in the callee when formatting, since the callee's function body often contains comments, whereas the caller syntax (argument expressions) rarely does. However, when future strategies used by the inliner expand the scope of the refactoring (e.g. by replacing not just the CallExpr or its coextensive ExprStmt, but the enclosing BlockStmt), then this will become a bigger problem.

The second situation in which inlining inserts extra brackets is when replacing a call func(params){block}(args) by a block { var (params = args); body }. In this case the extra block may be necessary in general to avoid conflicts between names in the caller and callee blocks, but it should be easy to detect when these sets are disjoint and omit the brackets. The challenge is that omitting the brackets means replacing a single statement (e.g. ExprSmt) by not one statement (BlockStmt) but two or more (the parameter binding decl and then then rest of the body). So, we can't simply replace one tree with another but will need to replace some sequence of statements in a list with some other sequence of statements (e.g. in its parent BlockStmt). The tricky part is that this requires that we expand the scope of the "old" tree to an enclosing block (as ominously mentioned earlier), which has serious consequences for the fidelity of caller comment handling. I suspect it might be simplest to again use textual (not tree-based) processing: insert the BlockStmt, remember where its braces are, format the file, replace the braces with spaces, then format the file again.

So we see that fundamentally both of these problems are yet more consequence of #20744, which I think it is really time for us to fix.

@findleyr @timothy-king

[timothy-king]

I suspect it might be simplest to again use textual (not tree-based) processing: insert the BlockStmt, remember where its braces are, format the file, replace the braces with spaces, then format the file again.

Instead of trying to get all of this right in exactly 1 function, maybe it makes more sense to try to post-process from a clean state? If we know we have inlined a list of regions, re-parse maybe just those files, type check the package, find the relavant ast.Nodes and then have a generic clean up the inlining pass? It is more expensive, but we are only paying the price if inlining happens. It also should be really easy to get right in a non-hacky way. It is more expensive though.

I don't think this is necessarily a good idea. Just food for thought.

[findleyr]

@timothy-king that's an interesting way to approach the problem: given an lbrack and rbrack span, type check and determine if the block can be flattened.

However, my intuition is that we should avoid this type of post-processing. Doing so may have potentially significant performance implications, and tends to lead to subtle bugs due to the loss of information. We've been bitten by similar types of things in the past.

I'm not saying it's a bad idea, just warning that we tend to have problems composing operations in this way.

[adonovan]

maybe it makes more sense to try to post-process from a clean state?

Unfortunately that isn't possible. Inlining may result in the addition to the caller of imports of packages that are direct dependencies of the callee. When running under unitchecker, say, we won't have access to the type information for those packages. This is a serious constraint on the design that I should document. (It's also the reason we have to reinvent the constant folding part of go/types: because we can't use the latter directly.)

[gopherbot]

Change https://go.dev/cl/532098 mentions this issue: internal/refactor/inline: don't insert unnecessary parens

[adonovan]

Fun test case for brace deletion:

func main() {
	if true {
		goto label
	}
	f()
label:
}

func f() {
	x := 0
	print(x)
}

Naive (non)-solution:

func main() {
	if true {
		goto label // error: goto label jumps over declaration of x
	}
	x := 0
	print(x)
label:
}

[gopherbot]

Change https://go.dev/cl/532099 mentions this issue: internal/refactor/inline: elide redundant braces