Subtraction of two chars, new conversions, and fixes for dynamic operator invocations and QuotationToExpression

[Happypig375]

fsharp/fslang-suggestions#1030

• Subtraction of two chars

fsharp/fslang-suggestions#1040

• Conversions of
  • char->decimal
  • string->nativeint
  • string->unativeint
  • decimal->nativeint
  • decimal->unativeint

Fixes for LanguagePrimitives.ExplicitDynamic

• Invocations of op_Implicit

  type A() = static member op_Implicit(_: A) = 1
  LanguagePrimitives.ExplicitDynamic<A, int> (A())

  Used to throw System.NotSupportedException: Dynamic invocation of op_Addition involving coercions is not supported.
  Now it invokes the op_Implicit instead.
• Invocations with types having more than one op_Explicit

  type B() = static member op_Explicit(_: B) = 1
             static member op_Explicit(_: B) = 1y
  LanguagePrimitives.ExplicitDynamic<B, int> (B())

  Used to throw System.Reflection.AmbiguousMatchException: Ambiguous match found. because the old implementation did a direct lookup of op_Explicit with parameter types, but reflection does not support looking up by overloaded return types directly.
  Now it invokes the B -> int overload of B.op_Explicit.

Fix for LanguagePrimitives.InequalityDynamic

• Wrong generic static cache parameter: OpEqualityInfo was used instead of OpInequalityInfo

Optimizations for type-to-same-type conversions

• Changed operation to no-op, e.g. byte 1uy used to emit a conv.u1 instruction, now it is a no-op because the types are the same, and just returning the input is fine.

Fix for dynamic invocations of checked operators

• Instead of calling the unchecked LanguagePrimitives.ExplicitDynamic, LanguagePrimitives.CheckedExplicitDynamic is added which performs dynamic checked operations

And various fixes for FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.QuotationToExpression

• <@ 1y + 1y @> (Used to throw System.InvalidOperationException: The binary operator Add is not defined for the types 'System.SByte' and 'System.SByte'.)
• <@ [] < [] @> (Used to throw System.InvalidOperationException: The binary operator LessThan is not defined for the types 'Microsoft.FSharp.Collections.FSharpList1[System.IComparable]' and 'Microsoft.FSharp.Collections.FSharpList1[System.IComparable]'.)
• <@ [|1|] = [|1|] @> (Used to translate to physical equality instead of structural equality like outside of quotations)
• <@ decimal LanguagePrimitives.GenericOne<nativeint> @> (Used to throw System.InvalidOperationException: No coercion operator is defined between types 'System.IntPtr' and 'System.Decimal'.)

[Happypig375]

RIP the CI

[vzarytovskii]

Yep, it seems CI is having a bad day today :(

[Happypig375]

Failing tests unrelated?

[dsyme]

See my comment here: fsharp/fslang-suggestions#1030 (comment)

[dsyme]

Note that some witness-passing code will need to be updated, check SubtractionDynamic and look for test cases related to witness passing

[Happypig375]

There is currently no test cases related to SubtractionDynamic. Are you referring to #6805 which is still open?

[dsyme]

Seach for this test:

    test "check CallWithWitnesses all operators)"      

and add a case for subtraction of chars

It exercises SubtractionDynamic under the hood

[Happypig375]

FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.EvaluateQuotation <@ 'a' + '\001' @>
|> printfn "%O"

fails with

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.InvalidOperationException: The binary operator Add is not defined for the types 'System.Char' and 'System.Char'.
  at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&)
  at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) <0x34197b0 + 0x000ce> in <filename unknown>:0 
   --- End of inner exception stack trace ---
  at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) <0x34197b0 + 0x000f6> in <filename unknown>:0 
  at System.Reflection.MethodBase.Invoke (System.Object obj, System.Object[] parameters) <0x3419060 + 0x00014> in <filename unknown>:0 
  at <StartupCode$WebFsc-Client>[email protected] (Microsoft.FSharp.Core.Unit unitVar) <0x3f469d0 + 0x00028> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvoke[T,TResult] (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt, TResult result1, Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] part2) <0x3f41748 + 0x0001a> in <filename unknown>:0 
  at <StartupCode$FSharp-Core>.$Async+Delay@1064[T].Invoke (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f415d8 + 0x00036> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.Invoke[T] (Microsoft.FSharp.Control.FSharpAsync`1[T] computation, Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f46220 + 0x000a0> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.Bind[T,TResult] (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt, Microsoft.FSharp.Control.FSharpAsync`1[T] part1, Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] part2) <0x3f46560 + 0x00068> in <filename unknown>:0 
  at WebFsc.Client.ScreenOut+Wrap@109-3[a].Invoke (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f46450 + 0x00014> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.Invoke[T] (Microsoft.FSharp.Control.FSharpAsync`1[T] computation, Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f46220 + 0x000a0> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvoke[T,TResult] (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt, TResult result1, Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] part2) <0x3f41748 + 0x00064> in <filename unknown>:0 
  at <StartupCode$FSharp-Core>.$Async+Delay@1064[T].Invoke (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f415d8 + 0x00036> in <filename unknown>:0 
  at <StartupCode$FSharp-Core>.$Async+Catch@1142[T].Invoke (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f411c0 + 0x00074> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.Invoke[T] (Microsoft.FSharp.Control.FSharpAsync`1[T] computation, Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f40e20 + 0x000a0> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.Bind[T,TResult] (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt, Microsoft.FSharp.Control.FSharpAsync`1[T] part1, Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] part2) <0x3f40ba0 + 0x00068> in <filename unknown>:0 
  at Elmish.Cmd+bind@52-3[a].Invoke (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3f40918 + 0x00014> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.Invoke[T] (Microsoft.FSharp.Control.FSharpAsync`1[T] computation, Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3a71130 + 0x000a0> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvoke[T,TResult] (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt, TResult result1, Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] part2) <0x3a6ab50 + 0x00064> in <filename unknown>:0 
  at <StartupCode$FSharp-Core>.$Async+Delay@1064[T].Invoke (Microsoft.FSharp.Control.AsyncActivation`1[T] ctxt) <0x3a6a960 + 0x00036> in <filename unknown>:0 
  at Microsoft.FSharp.Control.AsyncPrimitives+StartWithContinuations@915[T].Invoke (Microsoft.FSharp.Core.Unit unitVar0) <0x3a8d8d0 + 0x000b6> in <filename unknown>:0 
  at Microsoft.FSharp.Control.Trampoline.Execute (Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] firstAction) <0x3a6a1d8 + 0x0004a> in <filename unknown>:0 

. Looks like a fix will also be required.

[Happypig375]

Same for

FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.EvaluateQuotation <@ 1uy + 1uy @>

. Should fixing these be considered in-scope for this PR?

[Happypig375]

I'll handle them in this PR.

[Happypig375]

Ping

[dsyme]

I did a detailed scan and this looks good now. I will do one more detailed line by line review soon and then we can pull.

We will wait at least until dev17.0 --> main integration is done.

[vzarytovskii]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[KevinRansom]

/@Happypig375 ---

Thanks for this contribution , however, it seems to have remained untouched in quite a while. Do you have plans to return to it? or can it be closed?

Thanks

Kevin

[Happypig375]

@KevinRansom This PR has been waiting on the F# team for ages and you are telling me that it's my fault for leaving this untouched?

[Happypig375]

I will only touch this PR again if there is a guarantee of merging.

[vzarytovskii]

I will take care of merging main. Changes look good to me, I think we're good to merge. @dsyme may take another glance at it.

[dsyme]

Cool I'll do a review, thanks

[vzarytovskii]

@KevinRansom @dsyme this looks fine to me, I'm okay with merging it. Wdyt?

[Happypig375]

The year-long journey is finally over!

[dsyme]

@Happypig375 This is a truly vast amount of work on your part. It's really remarkable how much you have systematically tested this.

The PR adds complexity - which is why it took so very long to merge it - but the test suite captures why this is needed.

Some things that would have made it quicker

• Where possible only add tests. That proves that at least nothing has "stopped working" that was under test before. The rewrite of the test suite (e.g. in quotes) is very good, but I guess it would have been easier to review if it had only been additive.

• Always minimise the diff especially for anything complex. If necessary submit a pre-PR cleanup that adjusts things so the actual diff will be enitrely minimal.

[smoothdeveloper]

It indeed looks like tremendous work on test coverage.

I wonder if the tests under TestExpr wouldn't benefit from something similar as what was done for surface area, and also extracting the F# code input into files that can be edited with F# editor.

It must have been really hard to udpate the test and make it pass, to consider when adding such test will be required again.