Skip to content

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

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

Open
Tracked by #15408
psfinaki opened this issue Jun 27, 2023 · 4 comments
Open
Tracked by #15408

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

psfinaki opened this issue Jun 27, 2023 · 4 comments
Labels
Milestone

Comments

@psfinaki
Copy link
Member

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
Copy link
Contributor

nih0n commented Jul 1, 2023

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
Copy link
Member Author

psfinaki commented Jul 3, 2023

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.

@0101 0101 removed the Needs-Triage label Jul 17, 2023
@brianrourkeboll
Copy link
Contributor

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
Copy link
Member Author

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

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

No branches or pull requests

4 participants