Skip to content

Commit

Permalink
Analyzer & code fix for ~IDE0047~ FS3583: remove unnecessary parenthe…
Browse files Browse the repository at this point in the history
…ses (#16079)

* Add analyzer & code fix for IDE0047

* Consistency

* Consolidate lambda branches

* Try-finally tweak

* Fix

* Fantomas

* Explain

* More Fantomas

* Pass in getTextAtRange for № literals

* Fantomas

* Structs

* Don't need that

* Spaces

* getTextAtRange → getSourceLineStr

* Use `getSourceLineStr` and enable handling sensitive indentation
  inside parens.

* Singleton

* ↓

* Better

* Streamline

* One more

* Don't need that here

* Simplify lambda traversal

* Add comment

* Make private

* Fix infix op tests

* Add some more tests

* Be (somewhat) more systematic about precedence, &c.

* Represent precedence for more kinds of expression than just symbolic
  infix operators.

* Handle exprs potentially confusable with type applications.

* Handle more numeric literal cases.

* Handle dangling nested exprs (`match`, `if`, &c.)

* Consolidate SynPat logic

* Comments

* Space after backticks

* Parens in interp hole

* Submodule

* Move extension method to common module

* Expose 	raverseAll internally for clarity

* Remove redundant arrow

* Move diagnostic creation logic into its own file

* We're still calling the logic directly inside of
  `DocumentDiagnosticAnalyzer` rather than exporting the type as an
  implementation of the `IFSharpUnnecessaryParenthesesDiagnosticAnalyzer`
  interface, since that interface does not exist upstream in Roslyn, and
  it seems like we would currently prefer not to go through the
  logistics of upstreaming it.

* Fantomas

* Remove comment

* Apply Fantomas to tests

* Include paren diagnostics in builder capacity

* Update surface area

* Remove unnecessary parens in DocumentDiagnosticAnalyzer tests

* Add in-memory cache like SimplifyNameDiagnosticAnalyzer's

* Update misleading comment

* Ish

* Might as well include the other example

* The prefix op rules are tricky…

* +Assorted other bits and bobs

* Use existing helper method

* More correct dangling expr logic

* A few more tests for dangling exprs

* Fix incomplete exprs

* Missed that

* Structness is immaterial here

* Fantomas

* Remove semi-misleading word

* Add example to doc comment

* Move ROS extensions to separate file in FCS

* Use FS3583 instead of IDE0047

* Add example to comment

* Fix find/replace in comments

* Do the faster check first

* Pretty sure we don't actually need that

* Upon further thought, I'm pretty sure we _don't_ need to memoize
  the checking for dangling right-hand expressions, because we never
  dive deeper than needed for the check at hand, and the check will
  be quite shallow unless there are vast amounts of, say, nested matches
  inside of an if-condition body, but even then, such a check is both
  necessary and will only happen once, e.g.:

  ```fsharp
  let f x =
    if
       (match x with
        | _ ->
            match x with
            | _ ->
                match x with
                | _ ->
                    match x with
                    | _ ->
                        match x with
                        | _ ->
                            if x then x else x) then x else x
  ```

  If any other if-expression were found, or any nested parenthesized
  expression, or any construct without an exposed right-hand expression,
  the dive would end.

* Fix (nonfunctional) copy/paste bug

* Add tests for unmatched parens

* Consolidate affixed infix tests for speed

* Fantomas

* Address a few more corner cases

* Add basic ServiceAnalysis tests

* Remove redundant assertions

* Move some framework-y code to the framework file

* Remove incorrect test

* This test purported to test FS1182, but the actual diagnostic that is
  generated from the example is FS1183.

* Don't need

* A few more tests

* Add a few attribute-related tests

* Add test for new operator introduced in #15923

* Vars instead of consts

* Add a few clarifying comments

* Update tests/FSharp.Compiler.Service.Tests/UnnecessaryParenthesesTests.fs

---------

Co-authored-by: Petr <[email protected]>
  • Loading branch information
brianrourkeboll and psfinaki authored Oct 31, 2023
1 parent acd147b commit 3983afc
Show file tree
Hide file tree
Showing 51 changed files with 3,498 additions and 123 deletions.
1 change: 1 addition & 0 deletions src/Compiler/FSComp.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1725,3 +1725,4 @@ featureUnmanagedConstraintCsharpInterop,"Interop between C#'s and F#'s unmanaged
3579,alwaysUseTypedStringInterpolation,"Interpolated string contains untyped identifiers. Adding typed format specifiers is recommended."
3580,tcUnexpectedFunTypeInUnionCaseField,"Unexpected function type in union case field definition. If you intend the field to be a function, consider wrapping the function signature with parens, e.g. | Case of a -> b into | Case of (a -> b)."
3582,tcInfoIfFunctionShadowsUnionCase,"This is a function definition that shadows a union case. If this is what you want, ignore or suppress this warning. If you want it to be a union case deconstruction, add parentheses."
3583,unnecessaryParentheses,"Parentheses can be removed."
2 changes: 2 additions & 0 deletions src/Compiler/FSharp.Compiler.Service.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@
<Compile Include="Facilities\CompilerLocation.fs" />
<Compile Include="Facilities\BuildGraph.fsi" />
<Compile Include="Facilities\BuildGraph.fs" />
<Compile Include="Utilities\ReadOnlySpan.fsi" />
<Compile Include="Utilities\ReadOnlySpan.fs" />
<FsLex Include="AbstractIL\illex.fsl">
<OtherFlags>--module FSharp.Compiler.AbstractIL.AsciiLexer --internal --open Internal.Utilities.Text.Lexing --open FSharp.Compiler.AbstractIL.AsciiParser --unicode --lexlib Internal.Utilities.Text.Lexing</OtherFlags>
<Link>AbstractIL\illex.fsl</Link>
Expand Down
Loading

0 comments on commit 3983afc

Please sign in to comment.