Skip to content
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.

Sign up for GitHub

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

[WIP] Fix #513 for recursive types #961

Closed
wants to merge 2 commits into from
Closed

[WIP] Fix #513 for recursive types #961

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9344c31
Merge remote-tracking branch 'refs/remotes/Microsoft/master'
Feb 3, 2016
c2dcf23
Fix recursive types for equality and comparison
manofstick Feb 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 58 additions & 11 deletions src/fsharp/FSharp.Core/prim-types.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1871,7 +1871,12 @@ namespace Microsoft.FSharp.Core
module mos =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a proper uppercase name for this module. What is "mos"?

type IGetType =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking through all this code I realize now that #513 really was not properly code reviewed at all. GitHub simply didn't show the diff for the critical file prim-types.fs and no comments or proper review has been done. That this happened is is a grave problem with relying on GitHub PRs for code review.

For example two basic things that indicate this code hasn't been reviewed:

  • We need proper /// comments on every type declaration in prim-types
  • Please don't use abc_def_hij style naming

I'm not sure what to do about this yet. We should really revert the commit and start again with a full code review, but I think that @KevinRansom didn't want to do that. I do understand that the testing is good and the performance results are real. But we need to be more considered here.

abstract Get : unit -> Type


let isEqualTypedef (t1:Type) (t2:Type) =
t1.IsGenericType
&& t2.IsGenericType
&& (t1.GetGenericTypeDefinition ()).Equals (t2.GetGenericTypeDefinition ())

let makeType (ct:Type) (def:Type) : Type =
def.MakeGenericType [|ct|]

Expand Down Expand Up @@ -2068,11 +2073,32 @@ namespace Microsoft.FSharp.Core
override __.Invoke (comp, x:'a, y:'a) =
phantom<'comp>.Ensorcel (comp, x, y)

let makeComparerInvoker (ty:Type) comp =
let wrapperTypeDef = typedefof<EssenceOfCompareWrapper<int,ComparerTypes.Int32>>
let wrapperType = wrapperTypeDef.MakeGenericType [| ty; comp |]
Activator.CreateInstance wrapperType
[<Sealed>]
type ComparerInvoker_StructuralComparable<'a when 'a : null>() =
inherit ComparerInvoker<'a>()

override this.Invoke (comp, x:'a, y:'a) =
match x with
| null ->
match y with
| null -> 0
| _ -> -1
| _ -> (unboxPrim x : IStructuralComparable).CompareTo (y, comp)

let makeComparerInvoker (ty:Type) (comp:Type) =
let wrapperType =
if mos.isEqualTypedef comp typeof<ComparerTypes.RefType.StructuralComparable<Tuple<int,int>>> then
// This is required because recursive types do an extensive recursive function calls
// for comparison, and the constrained types building blocks model doesn't actually
// allow the IL instruction tail on constrained calls. This short circuits the implementation
// with an ComparerInvoker that casts so as to avoid that situation.
let wrapperTypeDef = typedefof<ComparerInvoker_StructuralComparable<_>>
wrapperTypeDef.MakeGenericType [| ty |]
else
let wrapperTypeDef = typedefof<EssenceOfCompareWrapper<int,ComparerTypes.Int32>>
wrapperTypeDef.MakeGenericType [| ty; comp |]
Activator.CreateInstance wrapperType

type t = ComparerTypes.Int32
type Function<'relation, 'a>() =
static let essenceType : Type =
Expand Down Expand Up @@ -2118,7 +2144,7 @@ namespace Microsoft.FSharp.Core
match info.ComparerType with
| ComparerType.ER -> eliminate_tail_call_int (GenericSpecializeCompareTo.Function<EquivalenceRelation,_>.Invoker.Invoke (comp, x, y))
| ComparerType.PER_gt
| ComparerType.PER_lt -> eliminate_tail_call_int (GenericComparisonForInequality comp x y)
| ComparerType.PER_lt -> GenericComparisonForInequality comp x y
| _ -> raise (Exception "invalid logic")
| c when obj.ReferenceEquals (c, Comparer<'T>.Default) ->
eliminate_tail_call_int (Comparer<'T>.Default.Compare (x, y))
Expand Down Expand Up @@ -2720,9 +2746,30 @@ namespace Microsoft.FSharp.Core
override __.Invoke (comp, x:'a, y:'a) =
phantom<'eq>.Ensorcel (comp, x, y)

let makeEqualsWrapper (ty:Type) comp =
let wrapperTypeDef = typedefof<EssenceOfEqualsWrapper<int,EqualsTypes.Int32>>
let wrapperType = wrapperTypeDef.MakeGenericType [| ty; comp |]
[<Sealed>]
type EqualsInvoker_StructuralEquatable<'a when 'a : null>() =
inherit EqualsInvoker<'a>()

override this.Invoke (comp, x:'a, y:'a) =
match x with
| null ->
match y with
| null -> true
| _ -> false
| _ -> (unboxPrim x : IStructuralEquatable).Equals (y, comp)

let makeEqualsWrapper (ty:Type) (comp:Type) =
let wrapperType =
if mos.isEqualTypedef comp typeof<CommonEqualityTypes.RefType.StructuralEquatable<Tuple<int,int>>> then
// This is required because recursive types do an extensive recursive function calls
// for equality, and the constrained types building blocks model doesn't actually
// allow the IL instruction tail on constrained calls. This short circuits the implementation
// with an EqualsInvoker that casts so as to avoid that situation.
let wrapperTypeDef = typedefof<EqualsInvoker_StructuralEquatable<_>>
wrapperTypeDef.MakeGenericType [| ty |]
else
let wrapperTypeDef = typedefof<EssenceOfEqualsWrapper<int,EqualsTypes.Int32>>
wrapperTypeDef.MakeGenericType [| ty; comp |]
Activator.CreateInstance wrapperType

type u = EqualsTypes.Int32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use abbreviations like this

Expand Down Expand Up @@ -2762,7 +2809,7 @@ namespace Microsoft.FSharp.Core
// The compiler optimizer is aware of this function (see use of generic_equality_per_inner_vref in opt.fs)
// and devirtualizes calls to it based on "T".
let GenericEqualityIntrinsic (x : 'T) (y : 'T) : bool =
eliminate_tail_call_bool (GenericSpecializeEquals.Function<PartialEquivalenceRelation,_>.Invoker.Invoke (fsEqualityComparerNoHashingPER, x, y))
GenericSpecializeEquals.Function<PartialEquivalenceRelation,_>.Invoker.Invoke (fsEqualityComparerNoHashingPER, x, y)

/// Implements generic equality between two values, with ER semantics for NaN (so equality on two NaN values returns true)
//
Expand All @@ -2787,7 +2834,7 @@ namespace Microsoft.FSharp.Core
| :? IEqualityComparerInfo as info ->
match info.Info with
| EqualityComparerInfo.ER -> eliminate_tail_call_bool (GenericEqualityERIntrinsic x y)
| EqualityComparerInfo.PER -> eliminate_tail_call_bool (GenericEqualityIntrinsic x y)
| EqualityComparerInfo.PER -> GenericEqualityIntrinsic x y
| _ -> raise (Exception "invalid logic")
| c when obj.ReferenceEquals (c, EqualityComparer<'T>.Default) ->
eliminate_tail_call_bool (EqualityComparer<'T>.Default.Equals (x, y))
Expand Down