Refactor parens API

[brianrourkeboll]

Description

A refactoring of some of #16079, making use of the new ParsedInput API exposed in #16462. The underlying parenthesization logic is essentially unchanged (it was just extracted into separate SynExpr and SynPat modules), and the existing test suite for the unnecessary parens analyzer passes.

Changes

• Remove UnnecessaryParentheses.getUnnecessaryParentheses.
• Expose SynExpr.shouldBeParenthesizedInContext.
• Expose SynPat.shouldBeParenthesizedInContext.

This refactoring is meant to make the parenthesization API more composable (and perhaps inspire other future APIs to be likewise).

Now that ParsedInput.fold is available, we no longer need a bespoke, single-use API like UnnecessaryParentheses.getUnnecessaryParentheses. The unnecessary parentheses analyzer can now simply call ParsedInput.fold to traverse the entire untyped AST and use the functions on the SynExpr and SynPat modules to interrogate syntax nodes as appropriate, gathering diagnostic ranges along the way.

Questions

• SynPat.shouldBeParenthesizedInContext does not currently take getSourceLineStr like SynExpr.shouldBeParenthesizedInContext, because the offsides rules for patterns are currently a bit more restrictive than they are for expressions. That is, for expressions, it's currently possible for code with "sensitive" indentation to compile when wrapped in parentheses but fail to compile when not, whereas for patterns, such could does not compile even when wrapped in parentheses. If the offsides rules for patterns were relaxed in the future, it's possible that we'd then need getSourceLineStr for SynPat.shouldBeParenthesizedInContext as well. Should we require it as a parameter now, even though it is not currently (and may never be) needed, to avoid needing to make a breaking change if it does become necessary?

• Should we consider using a delegate for getSourceLineStr here?

  type GetSourceLineString = delegate of sourceLine: int -> ReadOnlySpan<char>

Checklist

• Test cases added.
• Existing parens test suite passes.
• Release notes entry updated.

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
vsintegration/src	docs/release-notes/.VisualStudio/17.10.md

[psfinaki]

Thanks for bringing the announced refactoring :)

[psfinaki]

As for your questions, I think this is not the area where we should be overly futureproof nor I see the need for extra abstractions and delegates. I'm fine with the current version :)

[brianrourkeboll]

@psfinaki

As for your questions, I think this is not the area where we should be overly futureproof nor I see the need for extra abstractions and delegates. I'm fine with the current version :)

Yeah, the delegate would specifically be so that it could return a ReadOnlySpan<char> instead of a string, which could eventually let us eliminate a string allocation per line when that function is called (for multiline parenthesized expressions). But that isn't really a problem in non-pathological code anyway, and it wouldn't be hard or painful to make that change later, either.