Analyzer & code fix for ~IDE0047~ FS3583: remove unnecessary parentheses

[brianrourkeboll]

Resolves #16078.

I have an analyzer and code fix for IDE0047: remove unnecessary parentheses for expressions and patterns (adding it for type signatures can be done but will require updating SyntaxTraversal.Traverse to traverse types in many more places).

Extra.parens.2023-10-04.210059.mp4

I shoehorned the call to the analysis logic into DocumentDiagnosticAnalyzer.fs. This lets us avoid making any upstream changes to Roslyn that then need to flow back here, although eventually just hooking every new analyzer into DocumentDiagnosticAnalyzer seems like it would become unsustainable or at least undesirable, so we should probably come up with some kind of longer-term plan at some point.

I've added a fair number of obvious tests, but, due to the combinatorially-explosive nature of the problem at hand, they are by no means exhaustive. I'd be happy to hear suggestions for ways to improve them.

Top-level test summary

Questions

• The fix-all/bulk-fix functionality doesn't seem to trigger. Does anyone see an obvious reason why it wouldn't?

  • Edit: see Analyzer & code fix for ~IDE0047~ FS3583: remove unnecessary parentheses #16079 (comment).
  • This has been addressed in dab4696 by using a new FSxxxx diagnostic ID, FS3583, instead of IDE0047.

• I assume I need to add IFSharpUnnecessaryParenthesesDiagnosticAnalyzer and FSharpIDEDiagnosticIds.RemoveUnnecessaryParentheses to Roslyn. Might that also be why the bulk-fix functionality is not working for this (because Roslyn is not aware that IDE0047 is supported for F#)? What are the logistics for getting that into Roslyn so that it is available to use here? Edit: it sounds like we don't actually want to do this, at least right now, but I at least moved the diagnostic creation logic into its own file in a2c9fe2.

• When merged, should the analyzer be enabled by default in VS? It is for C#/VB, and I think it should be for F#, too (assuming the perf is acceptable). Either way, should it have a VS setting/be toggleable? As of right now, it will be enabled by default and will not be toggleable.

• The current implementation independently traverses the entire untyped syntax tree using a creative call to the code underlying SyntaxTraversal.Traverse. Is this OK, or even the right way or place at all? In theory the diagnostics could be emitted in CheckExpressions.fs, CheckPatterns.fs, etc., during the type-checking pass, although it's debatable whether that's the right place to emit an "IDE" diagnostic. Or is there another way to avoid/amortize/consolidate full AST traversals across analyzers (if that's even desirable—I can see not wanting one misbehaving analyzer to be able to block others)?

• I made a couple small updates to SyntaxTraversal.Traverse to handle some (seemingly) obvious omissions, namely:

  • Traverse the full patterns in the parsedData of the first SynExpr.Lambda in a lambda sequence instead of the simple pats of each lambda in the sequence.
  • Traverse the full body of the first SynExpr.Lambda in a lambda sequence instead of the body of each lambda in the sequence.
  • Traverse a record pattern's field patterns.
  • Traverse a quote pattern's expression.

  Is this OK?

• The only public API addition is

  val getUnnecessaryParentheses: getSourceLineStr: (int -> string) -> parsedInput: ParsedInput -> Async<range seq>

  Is this the signature we want?

  I can think of other scenarios where we might additionally want to expose another function along the lines of

  val parenthesesNeededBetween: getSourceLineStr: (int -> string) -> node: SyntaxNode -> path: SyntaxNode list -> bool

• It's not currently possible to apply the code fix to types (signatures or annotations, e.g., x : (int) → x : int), since SyntaxTraversal.Traverse does not traverse most SynType uses (and it explicitly skips SynType.Paren nodes altogether anyway). While certainly less pressing than expressions and patterns, would it ever make sense to handle types? (Edit: this should probably be done in one or more separate PRs.)

• It would be nice if we could assert that the AST never changed (at all? meaningfully?) upon application of the code fix in response to generated inputs, perhaps using something like FsCheck (compare https://blog.emillon.org/posts/2020-08-03-fuzzing-ocamlformat-with-afl-and-crowbar.html). If we did this, we'd need to take care to avoid any potential flakiness (or not run it on every commit, etc.). We could alternatively at least make such an assertion for the existing (non-generated) tests.

• I'm not currently handling the ML infix operators (mod, land…), since all (except for mod) now emit warnings. Should I? Edit: it seems reasonable not to handle them, since the default behavior will simply be not to suggest that parens can be removed even if they theoretically could, which I suspect is for the best, since I don't think many F# programmers without an OCaml background would know their precedence off the top of their head.

When ready

• Update surface area.

Remaining work

• Add tests that call the new FCS API directly.
• Consolidate some framework-y test code.
• Clean up/organize tests.

Potential followups

• Add fix-all tests.
• Add AST-checking tests(?). (I have a version that does this, and it does provide some amount of safety, but it surfaces a few mostly superficial representational annoyances that may or may not be worth spending the time to deal with.)

[nojaf]

Add a prefix: string option (or trivia) field to the SynConst cases for numeric literals to indicate the presence of prefixed +, -.

When it comes to strings and other constants, there actually are a lot of scenarios where the value in the untyped AST doesn't reflect what the user wrote. The way we deal with this in Fantomas is to get the original text from ISourceText (using #15979).

It's not currently possible to apply the code fix to types (signatures or annotations, e.g., x : (int) → x : int), since SyntaxTraversal.Traverse does not traverse most SynType uses

I'm in favour of extending SyntaxTraversal.Traverse to cover more ground. A lot of FCS consumers would benefit from that. (Recent example)

Nice work @brianrourkeboll!

[brianrourkeboll]

@nojaf

Add a prefix: string option (or trivia) field to the SynConst cases for numeric literals to indicate the presence of prefixed +, -.

When it comes to strings and other constants, there actually are a lot of scenarios where the value in the untyped AST doesn't reflect what the user wrote. The way we deal with this in Fantomas is to get the original text from ISourceText (using #15979).

Hmm yes, I tried the following and it does work, although it pollutes the signature/callsite a bit for what seems like a rather small and uncommon thing (e.g., parens are not needed in id ~~~(1y) but are in id ~~~(-0b11111111y), which is identical in the AST except for the range). We don't need it for interpolated and verbatim strings, since we have SynExpr.InterpolatedString and SynStringKind.

val getUnnecessaryParentheses : getTextAtRange: (range -> string) -> parsedInput: ParsedInput -> Async<range seq>

let getTextAtRange m = sourceText.ToString(RoslynHelpers.FSharpRangeToTextSpan(sourceText, m))
let! unnecessaryParentheses = UnnecessaryParentheses.getUnnecessaryParentheses getTextAtRange parseResults.ParseTree

Do you think it's worth the addition? We could pass in the Roslyn SourceText or F# ISourceText instead, although we don't actually need any other functionality from them—for this use case, we really only need getCharAtPos : pos -> char...

[nojaf]

I think having it as val getUnnecessaryParentheses: sourceText: ISourceText -> parsedInput: ParsedInput -> Async<range seq> will be most convenient for most users. Roslyn SourceText will probably be too tied to the VS side of things. It would be a shame if FSAC can't benefit from this work.

Overall it is a trade-off for sure. Passing in ISourceText seems reasonable to me.

[brianrourkeboll]

I think having it as val getUnnecessaryParentheses: sourceText: ISourceText -> parsedInput: ParsedInput -> Async<range seq> will be most convenient for most users. Roslyn SourceText will probably be too tied to the VS side of things. It would be a shame if FSAC can't benefit from this work.

Overall it is a trade-off for sure. Passing in ISourceText seems reasonable to me.

Hmm, I'm not totally against using ISourceText instead of getSubtext: (range -> string), but

1. I'm just noticing 🤦 that other public APIs in ServiceAnalysis.fs take getSourceLineStr: (int -> string) functions¹
2. It seems to me that the most compelling reasons to pass in an interface rather than a function are:
   a. If we foresee that the consuming function may someday need to use other APIs exposed by the interface
   b. If we want to do something that we can't use a first-class function for, like return a ReadOnlySpan<char> instead of a string to avoid allocations (and since we don't actually need a string)

As for (1), getSourceLineStr: (int -> string) would work fine, although the real requirement is the ability to match against an arbitrary span of subtext rather than anything to do with source lines.

As for (2a), I don't really foresee that this particular function (getUnnecessaryParentheses) will ever need to use any of the APIs that ISourceText currently exposes other than GetLineString (as a roundabout way to get a specific character or span of characters to compare against) or just GetSubTextFromRange (after #15979):

type ISourceText =
    abstract Item: index: int -> char with get
    abstract GetLineString: lineIndex: int -> string
    abstract GetLineCount: unit -> int
    abstract GetLastCharacterPosition: unit -> int * int
    abstract GetSubTextString: start: int * length: int -> string
    abstract SubTextEquals: target: string * startIndex: int -> bool
    abstract Length: int
    abstract ContentEquals: sourceText: ISourceText -> bool
    abstract CopyTo: sourceIndex: int * destination: char[] * destinationIndex: int * count: int -> unit

With getSubtext: (range -> string), a consumer (e.g., FSAC) that already had an ISourceText could just pass in its GetSubTextFromRange implementation directly:

let! unnecessaryParentheses =
    UnnecessaryParentheses.getUnnecessaryParentheses
        sourceText.GetSubTextFromRange
        parseResults.ParseTree

As for (2b), if we ever wanted to reap that benefit (although I realize that an ISourceText's lines are normally cached) we'd either need to have ISourceText.GetSubTextFromRange return a ReadOnlySpan<char> instead of a string (even if the implementation still used strings under the hood for now), or else perhaps make a single-method interface like:

type IGetSubtext =
    abstract GetSubtext: range: range -> ReadOnlySpan<char>

A caller with an ISourceText could then do something like:

let getTextFromRange =
    { new IGetSubtext with
        member _.GetSubtext range =
            (sourceText.GetSubTextFromRange range).AsSpan() }

let! unnecessaryParentheses =
    UnnecessaryParentheses.getUnnecessaryParentheses
        getTextFromRange
        parseResults.ParseTree

…All of that to say: I vote that we either:

• Just use something like getSubtext: (range -> string) (or I guess maybe getSourceLineStr: (int -> string) if we prefer making it a bit easier on the caller), or
• Update Add GetSubTextFromRange to ISourceText #15979 to make ISourceText.GetSubTextFromRange return a ReadOnlySpan<char> and use that

Footnotes

1. E.g.

   val getUnusedOpens:
       checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) -> Async<range list>
   val getSimplifiableNames:
       checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) -> Async<seq<SimplifiableRange>>
   

   ↩