`SynExpr.shouldBeParenthesizedInContext` subtleties
- Dominant language
- F#
- Stars
- 4.3k
- Forks
- 876
- Avg merge
- 4d 11h
- Merged PRs (30d)
- 131
Description
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., https://github.com/fsharp/FsAutoComplete/pull/1252 and https://github.com/fsharp/FsAutoComplete/pull/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:
https://github.com/dotnet/fsharp/blob/ee46275f9f302cc03dbcdf691e444282474856e4/src/Compiler/Service/SynExpr.fsi#L14-L15
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:
https://github.com/dotnet/fsharp/blob/ee46275f9f302cc03dbcdf691e444282474856e4/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs#L187-L191
and potential insertion of spaces before and after:
https://github.com/dotnet/fsharp/blob/ee46275f9f302cc03dbcdf691e444282474856e4/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs#L241-L246
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](https://github.com/dotnet/fsharp/blob/ee46275f9f302cc03dbcdf691e444282474856e4/vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs#L225-L239) (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:
```fsharp
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.
Contributor guide
Assessment
This issue has not been assessed yet.