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.

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

Return "None" when doing string None, instead of empty string #891

Open
5 tasks done
abelbraaksma opened this issue Jun 28, 2020 · 23 comments
Open
5 tasks done

Return "None" when doing string None, instead of empty string #891

abelbraaksma opened this issue Jun 28, 2020 · 23 comments

Comments

@abelbraaksma
Copy link
Member

abelbraaksma commented Jun 28, 2020

Print "None" when value is None

I propose we normalize how option is treated when turning its value into a string, namely, string None should return "None". Currently it returns the empty string.

This would bring it more in line with this existing change (dotnet/fsharp#7693) which returns "ValueNone" for string ValueNone. It also brings it in line with FSI, which prints "None" to the output window when it is None. Conversely, when debugging, None will appear as null, even though this PR was done to mitigate this.

The existing way of approaching this problem in F# is:

Roll your own version to get the string value of an option. But this doesn't help with debugging. It's also not very trivial to re-implement a generic string function in such a way that it works differently for option<_>.

Pros and Cons

The advantages of making this adjustment to F# are:

  • Many newcomers to the language are very surprised to see null when they inspect a variable that is None. This would help the general debugging experience.
  • When printing an option, you get "Some ..." in the output, but never "None", this leads to surprises: "why is this not None?" Until it dawns on you after some headbanging.
  • Alignment with other discriminated unions, that always print the case-name and the value. Most notably, "ValueNone" for ValueOptions.

The disadvantages of making this adjustment to F# are :

  • It's a tiny change in behavior for string, and if there are libraries out there that rely on string None = "", these would fail
  • If an option is converted to an obj (boxed), the old behavior remains, as it is impossible to detect the difference between a null object reference and a null value that once was None. But this problem applies in other parts of the .NET ecosystem as well.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): XS (proto-type is ready, link to PR follows)

I remember earlier discussions on this and that there was no simple solution, but perhaps that was about None.ToString(), which raises an NRE. I didn't know that there was a type-safe syntax form, albeit compiler-specific, that makes this work. It is possible that in the past this syntax wasn't available, but now that it is, I suggest we make use of it.

I've also checked to fix None.ToString() and the like, but that's a wider problem and should go in its own suggestion.

Related suggestions:

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

@charlesroddie
Copy link

charlesroddie commented Jun 30, 2020

Four string functions, with four different behaviours for None!

string None // ""
None.ToString() // Null Reference Exception
sprintf "%A" None // "None"
sprintf "%O" None // "<null>"

They should all return "None".

@pblasucci
Copy link

I'm not so sure I agree. Though I do think there should be more consistency. In fact, I'd say two of the four operations you listed give surprising results (but the other two meet, at least my, expectations). Here's how I'd expect it to work:

string None // "None"
sprintf "%A" None // "None"

None.ToString() // Null Reference Exception
sprintf "%O" None // Null Reference Exception

@charlesroddie
Copy link

Expectations in what sense? Are you saying that NREs are good behaviour? Presumably if you read the compiler source code you would get to the point where the current behaviour for the first two are also expectations in the sense you would expect the output given the current implementation?

@pblasucci
Copy link

There are really two issues here:

  1. Should callers be aware that None is implemented as null?
  2. Should .ToString() and "%O" exhibit the same behaviour?

Working backwards, the second point seems very straight-forward to me. Yes -- the "%O" type specifier should exhibit the same behaviour as calling .ToString().

Now, as to the first point...

I think, everything else being equal, we'd all love for None to have a different implementation. But that's not going to happen. So, if we say "None shouldn't act like null", then it's not just .ToString() that needs to change. Every instance method on FSharpOption`1 needs to be specially handled by the compiler (.GetHashCode(), .Equals(), et cetera). And that hints at the other issue I have with all this... using the string operator or the Printf module can have F#-specific behaviour, because there's no expectation-of-use outside of F#. However, invoking an instance method on a (potentially null) variable is a fundamental thing in the broader .NET ecosystem. It'd be weird to have it work one way everywhere -- except for one method on one value and only when processed by one compiler.

@charlesroddie
Copy link

@pblasucci Yes for 2 and no for 1. The null issue is hard to fix, agreed, but not impossible! Working through what is needed here: #884

@pblasucci
Copy link

pblasucci commented Jun 30, 2020

@charlesroddie I'm willing to bet #884, as it currently stands, doesn't ever get approved (though I can imagine marking UseNullAsTrueValue being deprecated). Further, I can't see the BDFL signing off on any fundamental change to the implementation of FSharpOption`1. Even if the change could be done in a way that retains binary compatibility, it'd still be a change with an enormous "ripple effect". Too risky.

@abelbraaksma
Copy link
Member Author

abelbraaksma commented Jun 30, 2020

The benefit of null in DUs with UseNullAsTrueValue is that you don't get punished for cases that have this value. Currently, None will never end up on the heap, not even when you box it. It's just always null.

I can imagine a design where it gets erased entirely, but I can't imagine a design where it is replaced with a heap allocated object, it would be too detrimental to performance. The value type approach wouldn't help here either, if only because of compatibility (and we already have ValueOption).

For user defined types, maybe it can be deprecated, but we'll still need to support it for the foreseeable next decades.

Btw, on NRE, I don't think users should have to be bothered by it. We already have instance methods that don't throw, the only issue is with the ones inherited from obj. I'd love to fix that, F# shouldn't throw NREs on its own types (that don't even support null on any DU officially).

I file that under the "principle of least surprise".

@pblasucci
Copy link

I can imagine a design where it gets erased entirely, but I can't imagine a design where it is replaced with a heap allocated object

Yup. Definitely agree.

For user defined types, maybe it can be deprecated, but we'll still need to support it for the foreseeable next decades.

💯 Spot on.

We already have instance methods that don't throw

Can you provide an example? I'm struggling to think of one (where the target is null, but the invocation doesn't NRE).

F# shouldn't throw NREs on its own types

Probably true... but how can you enforce that in the presence of reflection?

@charlesroddie
Copy link

charlesroddie commented Jun 30, 2020

If this is about None then user-defined types don't come into this. Note that a non-null representation would only be a single allocation on the heap as pointed out by @Tarmil in the other thread.

how can you enforce that in the presence of reflection?

Reflection breaks every assumption of the type system I believe so there is no way to make that safe and we should restrict to making non-reflection-using code work well and safely.

@abelbraaksma
Copy link
Member Author

Can you provide an example? I'm struggling to think of one (where the target is null, but the invocation doesn't NRE).

> Unchecked.defaultof<option<int>>.IsNone;;
val it : bool = true

> None.IsNone;;
val it : bool = true

I don't know if these are implemented as extension members or something, but certainly they "feel" as if they're on the type.

Also, GetTag(), but that's definitely implemented as an extension method.

Also just found that you cannot do myOption :> IEquatable<int option>, even though disassembly shows it's implemented (but I'm digressing).

@abelbraaksma
Copy link
Member Author

Note that a non-null representation would only be a single allocation on the heap

If I understand you correctly, than isn't that precisely what we try to prevent? None currently doesn't allocate.

@Tarmil
Copy link

Tarmil commented Jul 1, 2020

Note that a non-null representation would only be a single allocation on the heap

If I understand you correctly, than isn't that precisely what we try to prevent? None currently doesn't allocate.

A single allocation for all uses of None of a given type, not an allocation for every use of None. So it's very minor.

That being said, I'm not sure if testing equality against this static value (which is what pattern matching on option would compile to) would be as easily optimized by the JIT as a null check.

@pblasucci
Copy link

@abelbraaksma Thanks for the example. I checked and .IsNone/.IsSome as instance members are not implemented as an extension method. So it's yet another case of FSharpOption`1 being given special treatment. But this puts things in an even less-desirable state (for me at least). If the goal is consistency, then adding more "one offs" to FSharpOption`1 is actively working against that. ☹️

Aside: can we find examples of this behaviour for any other types (especially types defined outside of FSharp.Core)?

@abelbraaksma
Copy link
Member Author

abelbraaksma commented Jul 1, 2020

can we find examples of this behaviour for any other types

@pblasucci No, unless you make it an extension. Point style programming is susceptible to NREs.

. I checked and .IsNone/.IsSome as instance members are not implemented as an extension method

Interesting. But are they really instance methods in the assembly? I'll have to check.

Inside Core, special care is taken that strings, even when null, won't raise. But this is only true if you call the Core functions of String, or other functions that take or format a string (like %O).

That being said, I'm not sure if testing equality against this static value

@charlesroddie, I see now what you mean, one instance that's the same globally, like a module let.

That would mean you could use reference equality. But in compiled jitted code, that's a pointer dereference, and that's still going to be slower than a null check. In part because of predictability for the cpu and possibly other low level stuff.

(as another aside, wonder what happens if the address gets pinned and not released)

Though it's easy to code this up. I'll see if I can get some numbers (but this discussion digresses a bit from the simple proposal here, as even if we can proof there's no noticeable difference, it's unlikely to make it into the language anytime soon for binary compat reasons; also, any code that relies on None being null, like a lot of C#, but also any existing ngen'ed library, would fail).

@NinoFloris
Copy link

That would mean you could use reference equality. But in compiled jitted code, that's a pointer dereference, and that's still going to be slower than a null check. In part because of predictability for the cpu and possibly other low level stuff.

A proper static readonly gets jitted as a constant in .net core, so reference equality could be very fast

@abelbraaksma
Copy link
Member Author

@NinoFloris, good to know (so not in the garbage collectible heap, but heap nonetheless, right?), it'll help interpreting the next dive into binary assembly.

@NinoFloris
Copy link

Yeah I'm not exactly sure if it works for reference types :/

@abelbraaksma
Copy link
Member Author

I thought sharplab.io didn't support JIT for F# code?

To get the assembler code, a handy shortcut I use is with Benchmarkdotnet, which is smart enough to show only the relevant parts (sometimes it misses bits but it usually gets it right). Use the DisassemblyDiagnoser.

@Tarmil
Copy link

Tarmil commented Jul 3, 2020

Oh, I thought I had used sharplab this way before, but I guess not :/

@dsyme
Copy link
Collaborator

dsyme commented Apr 13, 2023

@abelbraaksma I'm marking this as approved in principle. I'll leave it up to @vzarytovskii and others to decide if the breaking change is acceptable.

I'm not entirely sure of the implementation. I gather you're proposing to reflect over the type variable in the case that it is no compile-time resolved? Note that

string (box None)

would still always return "" - because there is no practical way to distinguish box None from null.

@edgarfgp
Copy link
Member

edgarfgp commented Dec 2, 2024

@T-Gro Now that #1386 is been implemented. Would this also have priority ?

@T-Gro
Copy link

T-Gro commented Dec 2, 2024

I thinks its doable.
This is a real and not hypothetical (i.e. not degenerated use cases with reflection and/or evil types, but breaking for quite some normal code) breaking change and does have to be announced as such.
One thing that worries me is that I would not know how to handle easy optOut here - like we have with LanguageFeatures for compiler code.

This would likely be best achieved by when 'T : option<_> in the statically optimized FSharp.Core code for string. But I do not see a way of doing language version branching here.

(or any other easy means for people to optout temporarily if a mere SDK update breaks them and they need to gain some time to address necessary change)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants