SwensenSoftware / unquote

Write F# unit test assertions as quoted expressions, get step-by-step failure messages for free
http://www.swensensoftware.com/unquote
Apache License 2.0
285 stars 26 forks source link

[Decompiler][Patch]Failed around TupleGet pattern #77

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
I'm going to user your library in my project https://github.com/Kakadu/Falka/
Your tool have generatored worng code for
fun a b c ->
          match (a,b) with
          | (TNumber a,TOperator x) -> AExpr (x, ANumber a,c)
          | _ -> failwith "some bug here")

because you haven't investigate TupleGet pattern while decompilation,

Patch added.

Original issue reported on code.google.com by kakadu.hafanana on 1 Feb 2012 at 11:10

Attachments:

GoogleCodeExporter commented 9 years ago
Kakadu: thank you for the report and especially the supplied patch! Sorry it's 
taken me a little while to follow up on this, google code didn't seem to notify 
me and I only happened to look at the issue list right now and spot it. I will 
follow up on this and hopefully get a release for you in a timely manner.

Original comment by stephen....@gmail.com on 10 Feb 2012 at 7:42

GoogleCodeExporter commented 9 years ago

Original comment by stephen....@gmail.com on 10 Feb 2012 at 7:42

GoogleCodeExporter commented 9 years ago
Kakadu - can you give me a simpler, self-contained example of the code you are 
quoting, the decompiled result you are seeing, and the decompiled result you 
expect?

Please note that TupleGet is one of the more complicated, context driven 
quotation patterns which I have battled against mightily and created an 
abstraction around: 
http://code.google.com/p/unquote/source/browse/trunk/Unquote/ExtraPatterns.fs?r=
468#105

While I appreciate the patch you supplied, it is not as robust as the TupleLet 
pattern, and depends on the assumption of an implementation of the (?) dynamic 
operator.

Original comment by stephen....@gmail.com on 11 Apr 2012 at 1:44

GoogleCodeExporter commented 9 years ago
Try with this code.

> let f = <@ fun (a:int) (b:int) -> match a,b with | (1,_) -> 1  | _ -> b@> ;;
val f : Quotations.Expr<(int -> int -> int)> =
  Lambda (a,
        Lambda (b,
                Let (matchValue, NewTuple (a, b),
                     IfThenElse (Call (None,
                                       Boolean op_Equality[Int32](Int32, Int32),
                                       [TupleGet (matchValue, 0), Value (1)]),
                                 Value (1), b))))

> Unquote.Operators.decompile (f );;
val it : string =
  "fun a b -> let matchValue = (a, b) in if TupleGet (matchValue, 0) = 1 then 1 else b"

I dont expect to see TupleGet in the result. AFAIU it goes to the result in 
last pattern matching with wildcard. It seems that you are expecting correct 
pattern matching case in Decompilation.fs+227.

Original comment by kakadu.hafanana on 11 Apr 2012 at 7:50

GoogleCodeExporter commented 9 years ago
Perfect example: thank you!

Absolutely you shouldn't see TupleGet in the result: my goal is to approach 
100% decompiling to valid code (the decompiler falls back on F#'s quotation 
printing in cases it fails to consider, as you are seeing here).

But, do note that some expressions require very hard or near impossible 
"resugaring". In particular, the semantic forms of sequence expressions and 
matches are very different from their syntactic forms. So, in this example, the 
best I can do is probably

"fun a b -> let matchValue = (a, b) in if (let (t1,_) = matchValue in t1) = 1 
then 1 else b"

where I am falling back on using (let (t1,_) = matchValue in t1) instead of the 
dynamic approach you suggested, because, even though my version would be more 
verbose, I do not want to assume any outside dependencies such as the existing 
of a dynamic operator which works correctly on tuples.

Original comment by stephen....@gmail.com on 11 Apr 2012 at 2:31

GoogleCodeExporter commented 9 years ago
So we can close this bug.

Original comment by kakadu.hafanana on 11 Apr 2012 at 2:46

GoogleCodeExporter commented 9 years ago
Once I implement: yes. But I haven't done the work yet. I expect to fix this 
and get a release out including other bug fixes and enhancements by the end of 
the week.

Original comment by stephen....@gmail.com on 11 Apr 2012 at 3:10

GoogleCodeExporter commented 9 years ago
This issue was closed by revision r474.

Original comment by stephen....@gmail.com on 11 Apr 2012 at 7:10

GoogleCodeExporter commented 9 years ago

Original comment by stephen....@gmail.com on 11 Apr 2012 at 8:46