fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Improve C# -> F# lambda conversion codefix

Open psfinaki opened this issue 2 years ago • 3 comments

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.

psfinaki avatar Jun 27 '23 14:06 psfinaki

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

fernandoomorifaria avatar Jul 01 '23 04:07 fernandoomorifaria

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.

psfinaki avatar Jul 03 '23 18:07 psfinaki

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.

brianrourkeboll avatar Aug 28 '24 20:08 brianrourkeboll

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

psfinaki avatar Aug 29 '24 11:08 psfinaki