dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.9k stars 782 forks source link

Improve C# -> F# lambda conversion codefix #15499

Open psfinaki opened 1 year ago

psfinaki commented 1 year ago

Currently codefix is activated for this code:

let incAll = List.map (n => n + 1)

But not for this code:

let getPositives = List.filter (n => n > 0)

Not a bug, but would be useful.

nih0n commented 1 year ago

Hey, done some research about this, the bug is due the > character in the lambda body, for some reason it breaks something in the TryRangeOfParenEnclosingOpEqualsGreaterUsage during the SyntaxTraversal.Traverse process but I couldn't trace where exactly

psfinaki commented 1 year ago

Hi @nih0n! Thanks for taking a look into this. :)

Indeed, it seems like a problem with the syntax traversal. Actually, I am quite sure the filtering we are doing in this function has a flaw. By then, we have identified that we're in the parenthesized statement and so we are looking what's inside. With n => n + 1, the parser thinks we are trying to somehow apply n to n + 1 whereas with n => n > 0, it thinks we are trying to apply the whole n => n to 0 (basically it thinks we are comparing the former to the latter hence the operator in question is > not =>).

In syntax traversal terms, we are dealing with SynExpr.App. It has a part containing a functor funcExpr and a part containing an argument argExpr. And here, most probably in the code above, we should actually look at argExpr instead of funcExpr (and then within argExpr probably to the funcExpr again). Since we want both things working, it will probably mean just adding another pattern matching case to the expression.

Does it make sense? Do you want to give it a stab? At least this should be quite easy to test and debug now - the latest main contains ConvertCSharpLambdaToFSharpLambdaTests for unit testing of this stuff.

brianrourkeboll commented 1 month ago

With n => n + 1, the parser thinks we are trying to somehow apply n to n + 1 whereas with n => n > 0, it thinks we are trying to apply the whole n => n to 0 (basically it thinks we are comparing the former to the latter hence the operator in question is > not =>).

It's because => and > have the same precedence, while + has higher precedence than => (and all are left-associative). So n => n > 0 and n => n + 1 will have different ASTs.

psfinaki commented 1 month ago

Yeah. I've learned something about op precedence since then as well 👀