SynExpr.shouldBeParenthesizedInContext subtleties

[brianrourkeboll]

I'm logging this here so that I don't forget about it. You may have an opinion on this, @nojaf.

The SynExpr.shouldBeParenthesizedInContext API added in #16461 seems likely to be useful in a variety of analyzers, code fixes, and perhaps other dealings with the AST — see, e.g., ionide/FsAutoComplete#1252 and ionide/FsAutoComplete#1253.

Right now, the API simply returns a Boolean value that ostensibly indicates whether, yes, parentheses are required around a given expression in a given context, or no, parentheses are not required:

fsharp/src/Compiler/Service/SynExpr.fsi

Lines 14 to 15 in ee46275

	val shouldBeParenthesizedInContext:
	getSourceLineStr: (int -> string) -> path: SyntaxVisitorPath -> expr: SynExpr -> bool

Unfortunately, however, a false value does not always strictly mean that parentheses are not required. As things now stand, false may sometimes mean "parentheses are not required so long as some whitespace adjustments are made."

Whitespace

There are a number of constructs that the F# parser allows when parenthesized that would be disallowed or parsed differently without parentheses, sometimes dependent on outer syntactic context, other times not. Some examples include:

• Multiple symbol-based (non-alphanumeric) constructs, originally separated only by a parenthesis, that would be parsed as a single symbolic operator if the parenthesis were removed but whitespace were not added in the middle.
• Constructs containing funky indentation that is allowed when parenthesized but would not be allowed at all (offsides would be violated, etc.) without parentheses.
• Constructs containing indentation that would be valid without parentheses, but that would not be valid in context, due to an outer offsides line, etc., if the parentheses were removed in place without, e.g., shifting the inner construct leftward or rightward.
• Etc.

Such cases are currently handled by the "remove unnecessary parentheses" code fix itself, e.g., trimming and shifting:

fsharp/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs

Lines 187 to 191 in ee46275

	match sourceText with
	| TrailingOpen context.Span -> txt[1 .. txt.Length - 2].TrimEnd()
	| Trim trim & NoShift -> trim txt[1 .. txt.Length - 2]
	| Trim trim & ShiftLeft spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n" + String(' ', spaces), "\n"))
	| Trim trim & ShiftRight spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n", "\n" + String(' ', spaces)))

and potential insertion of spaces before and after:

fsharp/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs

Lines 241 to 246 in ee46275

	match adjusted with
	| WouldTurnInfixIntoPrefix -> ValueNone
	| ShouldPutSpaceBefore & ShouldPutSpaceAfter -> ValueSome(" " + adjusted + " ")
	| ShouldPutSpaceBefore -> ValueSome(" " + adjusted)
	| ShouldPutSpaceAfter -> ValueSome(adjusted + " ")
	| adjusted -> ValueSome adjusted

This works well enough for this particular code fix, and it allows the code fix to work in more cases that developers might expect it to work in, but this approach has two main problems:

1. In other scenarios where SynExpr.shouldBeParenthesizedInContext could be useful — as in the example code fixes linked above — the API consumer may not be aware of such nuances and would likely not want to reimplement the logic to handle them in any case.
2. I have not been perfectly consistent in how such corner cases are treated. There are some cases where SynExpr.shouldBeParenthesizedInContext returns true when minor whitespace adjustments could make parentheses optional, and there are other cases where it returns false that are later double-checked in the code fix itself (I should move that one out of the code fix and into SynExpr.shouldBeParenthesizedInContext).

Options

Here are some things we could do about this:

1. Be more conservative and make false always mean "parentheses are not required and may be removed without any whitespace adjustments." This could also include exposing the whitespace analysis as a separate API.

2. Return a more nuanced value, perhaps something like:

   type Parenthesization =
       | Required
       | Optional of shift : Shift option * before : TrimOrPad option * after : TrimOrPad option
       | Forbidden
   
   and TrimOrPad =
       | Trim of charsToTrim : int
       | AddSpace
   
   and Shift =
       | ShiftLinesLeft of spaces : int
       | ShiftLinesRight of spaces : int

   Consumers of the SynExpr.shouldBeParenthesizedInContext API could then choose whether they care only about absolute optionality or are interested in optionality that is conditional on additional whitespace adjustments.

3. Do nothing. The SynExpr.shouldBeParenthesizedInContext API is "good enough" most of the time as it is, especially when used with code that doesn't deviate from reasonable code formatting norms, i.e., code that follows standard indentation practices, puts spaces around infix operators, etc.

[nojaf]

@brianrourkeboll great write-up! The Parenthesization type sounds very useful for codefix scenarios.