Reverting #513

[manofstick]

As #513 didn't receive a proper code review (and subsequent clean up) it really was prematurely pulled into master. This pull request is restore prim_types.fs to a state that doesn't include those changes.

[msftclas]

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[manofstick]

NB. This will probably cause test breaks re: code gen due to the re-inserting of tail. IL instructions.

[dsyme]

Hi Paul, Thanks so much for preparing this, I know it's never easy to prepare a revert.

I'll now do a really proper code review of where we are, to the point where we can either press the button on the revert (and then resubmit with the tailcall fix included) or forge ahead with where we are plus the fix. Either way we'll get to the right place - I'm very sure we want this in in the end.

Unfortunately this will be early next week as I'm on vacation until then.

Thank you so much once again, it's the right thing to have this ready and give space for full review.

Best
Don

[manofstick]

Enjoy your holiday!

When you do get back to things....

In response to code review that you started, I do kind of have 'answers' for why things are like they are (that you commented upon). Not that I'm arguing that they shouldn't change, just letting you in on my thought processes as I made things the way they are.

• eliminate_tail_calls_xxx was deliberately jarring (i.e. use of underscore) so that it did stick out like a sore thumb. And what do you know, it was the cause of an issue!
• The type in the type abbreviation "type u = EqualsTypes.Int32" was really just to make some of the tuple lines palatable. The type isn't actually used, as it is just to form a valid typedefof<> statement, but needed to exist (i.e. can't use "_") because of type constraints
• "mos" namespace was just a common helper namespace; it didn't mean anything. But, I don't really like polluting the scope with with opening modules where I can avoid it, especially given the size of prim_types. Calling it a proper name like "EqualityAndComparersHelper" just becomes such a mouthful. "Helper" just becomes meaningless. But I really don't have a great idea here. (As a consumer of such things is, oh, that's just some module. Why did they call it that? Who knows, don't care, now what does this function do.... And because it is a separate namespace, my brain can then turn it off find it easy to parse... where as I find all just the prefixed "mkXXX" in the fsharp code base (and the similar naming conventions) really hard for my brain to ignore and cognitively draining... Horses for courses... Anyway...)
• /// on all types; I agree as an idea; but there are a lot of types that are very boilerplate, like ComparerTypes.Bool, ComparerTypes.SByte, ComparerTypes.Int16, which are really just single method implementations of IEssenceOfCompareTo. And the same goes for equals and hash. But that said, there definitely are places that could be improved with a triple slash.
• Something you didn't comment on, but you probably will at some stage -- what's with "Ensorcel"? Well I really couldn't come up with a good name here. "Invoke" would have probably been my first choice, but to me it is overused already. But in the magical tradition of "Invoking" or "Casting", and the somewhat magically creation of the comparison/equality/hash objects then Ensorcel it was. I kind of like it, but hey, if you can think of something else that suits then I'll change. Naming is hard.

[dsyme]

@manofstick Could you fix up the failing codegen tests please so we can accept this PR and move on to the code review? I see 11 failing tests including this one:

[00:31:36] +++ Optimizations\GenericComparison (Compare07.fs - NetFx40) +++

Thanks
Don

[manofstick]

Been a bit sick... still am... just updating to latest master... will check state of the world after that...

[manofstick]

Obviously that was wrong; Hmm. I'm pretty sure it had worked for me; although that was just a straight run of the ILCompare, not from the test suite... Must have done something wrong.

Anyway, I'll try again tonight...

[manofstick]

OK; well after doing the ci build locally I see that it leaves behind the il files as artifacts, which do include the path, so copied them over and trying again. I guess I'll find out tomorrow if this is the correct procedure.

[dsyme]

OK, I've finally pushed the button on this revert.

I did take another good look at the diff in prim-types.fs for #513 to see if we could just keep it after all, since I hate reverting it as much as anyone else. But I still I do have lots and lots of questions, we just need to go through it in a proper review.

@manofstick Could you submit a new PR with the basic work plus the combined fixes.

Here are some things I noticed on first review:

• Use a better name than Ensorcel, I don't know what that means. Maybe just Invoke or Apply.
• Use upper case names for all type variables, e.g. T not a
• Explain why -2 and +2 are used as return values. I didn't specifically see at fist where the -2 and +2 values are consumed. Now I see they just become Booleans in the LT/GT entry points. It would be good to document that more explicitly.
• Use a better name than mos
• Don't use class ... end, it's not needed
• Don't use type u = EqualsTypes.Int32, the name u doesn't help.
• Document the Function type and explain what it does. Give it a better name too.
• I'm not sure Activator.CreateInstance equals is better than a reflective get on a static field/property. Particularly because it needs the IGetType interface.
• For the line if comp.ComparerType.Equals ComparerType.ER perhaps just using a match would be better, calling Equals raises questions in my mind like is this doing boxing.
• I'm trying to understand why we're adding optimizations for Nullable in this - is that essential, were there specific scenarios that drove you to do that? I'm asking partly because prim-types.fs didn't previously have any specific functionality for Nullable
• The repetition of new inline assembly in these makes me nervous - is there any way we could factor this out into an inlined helper function shared with some other code paths in prim-types.fs??

                [<Struct; NoComparison; NoEquality>] type Unativeint = interface IEssenceOfCompareTo<unativeint> with member __.Ensorcel (_,x,y) = if (# "clt.un" x y : bool #) then (-1) else (# "cgt.un" x y : int #)

• IEssenceOfEquals would be better called IEqualsTrait (likewise others)
• Remove the commented-out code let inline mask (n:int) (m:int) = (# "and" n m : int #) etc.
• We need to discuss the portable47 issue - perhaps we're not even going to update that DLL in any case.

[manofstick]

@dsyme

No worries; I was actually surprised when it was merged without change... Anyway, I'll see how time treats me, but maybe this weekend or the next. Hopefully not too far in the future anyway.