search

dadhi / FastExpressionCompiler

Fast Compiler for C# Expression Trees and the lightweight LightExpression alternative. Diagnostic and code generation tools for the expressions.
MIT License
1.16k stars 82 forks source link

Add (back?) support for Rethrow expression #382

Closed exyi closed 10 months ago

exyi commented 10 months ago

Linq.Expressions supports rethrow in catch blocks (C# throw; statement) by setting the Operand to null.

This is causing exception in FEC as of version 4.0.0, but it somehow worked in previous versions. I have no idea why, as I couldn't find any handling of rethrows in the older version, but upgrading FEC from 3.4.0-preview-01 to 4.0.0 makes our tests fail. If you'd like to know which commit did it, I can bisect it, I figured it would be easier to just go add support for it. I guess it somehow worked only by accident :shrug:, it seems that we don't run into the rethrow instruction in our unit tests, we only "needed" it to compile somehow.

So this patch adds proper support for rethrows, including some unit tests. I also added Rethrow function to LightExpression and support to ToCSharpString method

dadhi commented 10 months ago

Oh, great for finding and more for the complete PR. I will review it shortly. @exyi Kudos!

exyi commented 10 months ago

And no hurry, enjoy the holidays! :D

dadhi commented 10 months ago

@exyi I have added your tests to the Run method (this how FEC runs the tests, I probably need to add CONTRIBUTING.md for the matter). Now some tests are failing, probably because the ToExpression should be adapted the same way as ToCSharpString. Would you do that?

exyi commented 10 months ago

Yes, I think it should be fixed. I have some other tests failing locally, I guess it's a platform issue :shrug: (FastExpressionCompiler.LightExpression.IssueTests.Issue346_Is_it_possible_to_implement_ref_local_variables should use ldind.r4 instead of ldind.i4?)

dadhi commented 10 months ago

@exyi Thanks

(FastExpressionCompiler.LightExpression.IssueTests.Issue346_Is_it_possible_to_implement_ref_local_variables should use ldind.r4 instead of ldind.i4?)

Yes, you're right. I have added DefineConstant PRINTIL into the AssertOpCodes so that completely disabled the assert. Will be fixing it.