-
Notifications
You must be signed in to change notification settings - Fork 21
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.
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
'unit' should implement IEquatable<unit>
and IComparable<unit>
#1345
Comments
I believe "null" is always emitted as a unit value. To my knowledge there is not even a singleton unit value in FSharp.Core. In a way Unit should be labelled with UseNullAsTrueValue This means the interfaces are not supported in any real sense. Can your IEquatable code work in this situation? Or does it have to be written to tolerate UseNullAsTrueValue types. Thanks |
@dsyme is right, For instance, calling any of the methods you dumped above will lead to an NRE:
And if we look at how this compiles, we see that it is always
Yes. But if you would ever pass
It will always boil down to: That said, I don't see a real downside of doing this. While these methods will never be callable in practice, if there are actual use-cases, I'm not against doing so. It's a benign change. But if that's enough motivation? |
Implementation of this would presumably be (since it would be breaking to make |
That would likely break compatibility with all the existing compiled F# libraries and existing compiler versions that people use. |
In what way would it break compatibility? Is there any code that relies on the implementation of |
Libraries compiled before such a change will still use null though. So you could take a unit returned from an existing library, try to call Equals on it, and get a NRE. |
That would happen at the moment so no break is caused. I wouldn't expect returning |
I meant Equals from IEquatable, which unit doesn't implement at the moment. My point is, if unit implements interfaces, then you'll expect to be able to use them, but unit returned from older libraries will fail. |
@charlesroddie, at a minimum, every single public function, method, interface that exposes Here's an example where you can see that using
Yes. I have such code in my helper libraries used in unit testing. And I wouldn't be surprised if such code exists in F# (i.e. in A trivial example is the following, and it is easy to see how this could help with log messages or debug output in unit tests: /// Print var info based on the type
let DebugInfo (a: 'T) =
match box a with
| null when typeof<'T> = typeof<Unit> -> "Unit type"
| null when typeof<'T> = typeof<option<int>> -> "None value of type int"
| null when typeof<'T> = typeof<option<string>> -> "None value of type string" // of course, this would be generalized
| _ -> "Unknown" Alternatively, a null-safe
Not really. I've written code along the lines of above quite often and while it can take a moment before you realize how certain types are implemented and that it requires some creativity in coding dynamic type detection, it is not all too uncommon. Another consideration, and a strong reason why back in the pre-F# 1.0 days it was decided that Btw, fwiw, That said, I still don't see any harm in adding these interfaces to |
Sorry took a while to reinvestigate this. The use case here is minor. I encountered it when using mapped signals, something like So the main reason is logical consistency for The use of |
I propose that
unit
should implementIEquatable<unit>
andIComparable<unit>
.Existing:
Implementation:
Pros and Cons
The advantages of making this adjustment to F# are
IEquatable<'a>
.where 'a :> IEquatable<'a>
instead ofwhere 'a : equality
. I am doing a spike to investigating usingIEquatable<'T>
almost everywhere and the absence of this feature made this awkward.IEquatable<unit>
rather than a more complex equality function.The disadvantages of making this adjustment to F# are ...
Extra information
Estimated cost (XS, S, M, L, XL, XXL):
XS
Related suggestions: (put links to related suggestions here)
Affidavit (please submit!)
Please tick these items by placing a cross in the box:
Please tick all that apply:
For Readers
If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.
The text was updated successfully, but these errors were encountered: