jso0 / runsharp

Automatically exported from code.google.com/p/runsharp
MIT License
2 stars 0 forks source link

Short circuit boolean operators ( || and && ) do not generate IL that matches C# compiler IL #19

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Hi

I'm currently evaluating the possibility of using RunSharp to be a rule 
compiler for dynamic formulas entered by a user.  I've got most of the 
infrastructure (parsing, building AST etc.) sorted out and I am walking the AST 
tree using runsharp to generate CIL instructions.  Most of this seems to be 
working fine.  The IL generated by runsharp is very close/matches the IL 
generated for the equivalent formula typed in C# and generated by the C# 4.0 
compiler.  However, I'm got an anomaly with the short-circuit boolean operators 
where runsharp generates the code differently than the C# compiler.  Consider 
the following C# sharp rule:

        public static bool Rule4(HTANTRL.IOptionValueProvider options)
        {
             return ((((options.GetOptionValueAsInteger(1010) == 100) && (System.String.Compare(options.GetOptionValueAsString(1009), "B", System.StringComparison.OrdinalIgnoreCase) == 0)) || 
                      ((options.GetOptionValueAsInteger(1010) == 125) && (System.String.Compare(options.GetOptionValueAsString(1009), "S", System.StringComparison.OrdinalIgnoreCase) == 0))) || 
                      ((options.GetOptionValueAsInteger(1010) == 160) && (System.String.Compare(options.GetOptionValueAsString(1009), "S", System.StringComparison.OrdinalIgnoreCase) == 0))) || 
                      ((options.GetOptionValueAsInteger(1010) == 200) && (System.String.Compare(options.GetOptionValueAsString(1009), "S", System.StringComparison.OrdinalIgnoreCase) == 0));
        }

Notice the use of the && and || operators.  I then experiment and try to create 
the equivalent method by hand using runsharp using the following code:

        private void GenerateRule4(TypeGen typeGen)
        {
            CodeGen g = typeGen.Public.Static.Method(typeof(bool), "Rule04").Parameter(typeof(IOptionValueProvider), "options");
            {
                Operand op11 = g.Arg("options").Invoke("GetOptionValueAsInteger", Operand.FromObject(1010)).EQ(Operand.FromObject(100));
                Operand op12 = Static.Invoke(typeof(System.String), "Compare", g.Arg("options").Invoke("GetOptionValueAsString", Operand.FromObject(1009)), Operand.FromObject("-B"), Operand.FromObject(System.StringComparison.OrdinalIgnoreCase)).EQ(Operand.FromObject(0));

                Operand op21 = g.Arg("options").Invoke("GetOptionValueAsInteger", Operand.FromObject(1010)).EQ(Operand.FromObject(125));
                Operand op22 = Static.Invoke(typeof(System.String), "Compare", g.Arg("options").Invoke("GetOptionValueAsString", Operand.FromObject(1009)), Operand.FromObject("S"), Operand.FromObject(System.StringComparison.OrdinalIgnoreCase)).EQ(Operand.FromObject(0));

                Operand op31 = g.Arg("options").Invoke("GetOptionValueAsInteger", Operand.FromObject(1010)).EQ(Operand.FromObject(160));
                Operand op32 = Static.Invoke(typeof(System.String), "Compare", g.Arg("options").Invoke("GetOptionValueAsString", Operand.FromObject(1009)), Operand.FromObject("S"), Operand.FromObject(System.StringComparison.OrdinalIgnoreCase)).EQ(Operand.FromObject(0));

                Operand op41 = g.Arg("options").Invoke("GetOptionValueAsInteger", Operand.FromObject(1010)).EQ(Operand.FromObject(200));
                Operand op42 = Static.Invoke(typeof(System.String), "Compare", g.Arg("options").Invoke("GetOptionValueAsString", Operand.FromObject(1009)), Operand.FromObject("S"), Operand.FromObject(System.StringComparison.OrdinalIgnoreCase)).EQ(Operand.FromObject(0));

                g.Return((((op11 && op12) || (op21 && op22)) || (op31 && op32)) || (op41 && op42));
            }
        }

When I look at the IL generated by runsharp using Reflector, I notice that the 
short-circuit boolean operators have been trandslated into if ? true : false 
statements and not the equivalent IL generated by the compiler.  

I might be missing something, but I can't seem to figure out what I'm doing 
wrong.  Is this a limitation in runsharp or am I using the API incorrectly.

Thanks for a great library.

What steps will reproduce the problem?
1.
2.
3.

What is the expected output? What do you see instead?

What version of the product are you using? On what operating system?

Please provide any additional information below.

Original issue reported on code.google.com by carel.l...@gmail.com on 20 Aug 2010 at 6:57

GoogleCodeExporter commented 9 years ago
Hi,

From what I see in reflector, the last line translates exactly to:

(op11 ? op12 : false) || (op21 ? op22 : false) ...

It might seem wrong, but 'A ? B : false' is in fact equivalent to A && B (also 
'A ? true : B' is equivalent to 'A || B', they are even implemented this way - 
see Operand.LogicalAnd/Or). See the following table (I hope it's easy enough to 
understand - the values in parentheses are unused in the particular 
calculation, square brackets mark the result - the point is that the 
short-circuiting works exactly as with && :)

   A  B  A && B   A ? B : 0
   0  0 [0]  (0)  0 -(0)>[0]
   0  1 [0]  (1)  0 -(1)>[0]
   1  0  1 ->[0]  1 >[0]
   1  1  1 ->[1]  1 >[1]

I believe that the code appearing in reflector is different only because the 
ordering of the instructions is a bit off for the && implementation. I can try 
to fix it, but I'm giving it the lowest possible priority :)

Also, looking at the code you have provided - are you aware that there is an 
implicit conversion operator for most primitive types, so that you can save 
typing Operand.FromObject(...), as well as proper implementations of most 
operators?

Your code could be rewritten as follows (if only for the sake of readability):

        private void GenerateRule4(TypeGen typeGen)
        {
            CodeGen g = typeGen.Public.Static.Method(typeof(bool), "Rule04").Parameter(typeof(IOptionValueProvider), "options");
            {
                Operand options = g.Arg("options");

                Operand op11 = options.Invoke("GetOptionValueAsInteger", 1010) == 100;
                Operand op12 = Static.Invoke(typeof(string), "Compare", options.Invoke("GetOptionValueAsString", 1009), "-B", StringComparison.OrdinalIgnoreCase) == 0;

                Operand op21 = options.Invoke("GetOptionValueAsInteger", 1010) == 125;
                Operand op22 = Static.Invoke(typeof(string), "Compare", options.Invoke("GetOptionValueAsString", 1009), "S", StringComparison.OrdinalIgnoreCase) == 0;

                Operand op31 = options.Invoke("GetOptionValueAsInteger", 1010) == 160;
                Operand op32 = Static.Invoke(typeof(string), "Compare", options.Invoke("GetOptionValueAsString", 1009), "S", StringComparison.OrdinalIgnoreCase) == 0;

                Operand op41 = options.Invoke("GetOptionValueAsInteger", 1010) == 200;
                Operand op42 = Static.Invoke(typeof(string), "Compare", options.Invoke("GetOptionValueAsString", 1009), "S", StringComparison.OrdinalIgnoreCase) == 0;

                g.Return((((op11 && op12) || (op21 && op22)) || (op31 && op32)) || (op41 && op42));
            }
        }

Hope this helps,
Stefan

Original comment by StefanSi...@gmail.com on 20 Aug 2010 at 8:38

GoogleCodeExporter commented 9 years ago
Thanks for the quick reply Stefan

I also figured that the logic seemed to be correctly translated.  Was not sure 
though whether the IL was correct and my limited experience with IL as this 
point caused me to ask the question.  As long as it is logically equivalent and 
just as performant as the C# compiler code, I'm happy.

W.r.t the implicit operator - I'm already using that.  I pasted an old sample, 
but thanks for the tip.   I still have a few cases out of the 30000 rules being 
translated not generating correctly IL, but I have yet to determine whether it 
is me not traversing the AST correctly or whether it is related to runsharp.  
I'll let you know if I discover any further anomalies.

Thanks again for a great library.
Carel

Original comment by carel.l...@gmail.com on 20 Aug 2010 at 8:54