Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unnecessary parentheses analyzer trigger in chained function call #1362

Open
5 tasks done
joprice opened this issue Feb 23, 2025 · 2 comments
Open
5 tasks done

unnecessary parentheses analyzer trigger in chained function call #1362

joprice opened this issue Feb 23, 2025 · 2 comments
Labels

Comments

@joprice
Copy link

joprice commented Feb 23, 2025

Version

v0.77.2

Dotnet Info

.NET SDK:
Version: 8.0.403
Commit: c64aa40a71
Workload version: 8.0.400-manifests.e0880c5d
MSBuild version: 17.11.9+a69bbaaf5

Runtime Environment:
OS Name: Mac OS X
OS Version: 15.0
OS Platform: Darwin
RID: osx-arm64
Base Path: /usr/local/share/dotnet/sdk/8.0.403/

.NET workloads installed:
Configured to use loose manifests when installing new manifests.
There are no installed workloads to display.

Host:
Version: 9.0.0
Architecture: arm64
Commit: 9d5a6a9aa4

.NET SDKs installed:
6.0.427 [/usr/local/share/dotnet/sdk]
7.0.410 [/usr/local/share/dotnet/sdk]
8.0.303 [/usr/local/share/dotnet/sdk]
8.0.403 [/usr/local/share/dotnet/sdk]
9.0.101 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
Microsoft.AspNetCore.App 6.0.35 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.20 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.7 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.10 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 9.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 6.0.31 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.32 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.35 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.20 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.6 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.7 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.10 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 9.0.0-preview.6.24327.7 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 9.0.0-rc.2.24473.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 9.0.0 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

Steps to reproduce

Call a member function that takes at least one argument with parens that is followed by another function:

 bg.lighten(0.2).hexa ()

The Parentheses can be removed warning is triggered for the (0.2) argument, even thought applying the quick fix will result in the invalid expression

bg.lighten 0.2.hexa ()

Details

  • Expected behavior: on warning should be triggered when a function application in a chain of calls requires parens to disambiguate it
  • Actual behavior: a warning is issues for a valid expression, and an invalid quick fix is applied

Logs

No response

Checklist

  • I have looked through existing issues to make sure that this bug has not been reported before
  • I have provided a descriptive title for this issue
  • I have made sure that that this bug is reproducible on the latest version of the package
  • I have provided all the information needed to reproduce this bug as efficiently as possible
  • I or my company would be willing to contribute this fix
@joprice joprice added the bug label Feb 23, 2025
@TheAngryByrd
Copy link
Member

cc @brianrourkeboll

@brianrourkeboll
Copy link
Contributor

Hmm, this is a pretty obvious bug in hindsight: this case needs to come after this one 🤦‍♂️.

// Parens are never required around suffixed or infixed numeric literals, e.g.,
//
//     (1l).ToString()
//     (1uy).ToString()
//     (0b1).ToString()
//     (1e10).ToString()
//     (1.0).ToString()
| DotSafeNumericLiteral, _ -> false
// We can't remove parens when they're required for fluent calls:
//
//     x.M(y).N z
//     x.M(y).[z]
//     _.M(x)
//     (f x)[z]
//     (f(x))[z]
//     x.M(y)[z]
//     M(x).N <- y
| SynExpr.App _, SyntaxNode.SynExpr(SynExpr.App(argExpr = SynExpr.ArrayOrListComputed(isArray = false))) :: _ -> true

| _, SyntaxNode.SynExpr(SynExpr.App _) :: path
| _, SyntaxNode.SynExpr(OuterBinaryExpr expr (Dot, _)) :: SyntaxNode.SynExpr(SynExpr.App _) :: path when
    let rec appChainDependsOnDotOrPseudoDotPrecedence path =
        match path with
        | SyntaxNode.SynExpr(SynExpr.DotGet _) :: _
        | SyntaxNode.SynExpr(SynExpr.DotLambda _) :: _
        | SyntaxNode.SynExpr(SynExpr.DotIndexedGet _) :: _
        | SyntaxNode.SynExpr(SynExpr.Set _) :: _
        | SyntaxNode.SynExpr(SynExpr.DotSet _) :: _
        | SyntaxNode.SynExpr(SynExpr.DotIndexedSet _) :: _
        | SyntaxNode.SynExpr(SynExpr.DotNamedIndexedPropertySet _) :: _
        | SyntaxNode.SynExpr(SynExpr.App(argExpr = SynExpr.ArrayOrListComputed(isArray = false))) :: _ -> true
        | SyntaxNode.SynExpr(SynExpr.App _) :: path -> appChainDependsOnDotOrPseudoDotPrecedence path
        | _ -> false

    appChainDependsOnDotOrPseudoDotPrecedence path
    ->
    true

So I think this bug should technically only happen for fluent calls where the parenthesized argument is a suffixed numeric literal or a numeric literal with a decimal point.

I.e.:

bg.lighten(0.2).hexa () // Wrongly suggests removal.

but

let x = 0.2
bg.lighten(x).hexa () // Won't suggest removal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants