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

Add "Make .Is* discriminated union properties visible from F#" #402

Merged
merged 4 commits into from
Jan 17, 2020

Conversation

Tarmil
Copy link
Contributor

@Tarmil Tarmil commented Oct 2, 2019

No description provided.

Copy link
Member

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

These are more suggestions than anything else

@cartermp
Copy link
Member

cartermp commented Oct 3, 2019

Also might be worth mentioning that existing DUs that define IsXX members won't compile today, so this is safe from that perspective.

Co-Authored-By: Phillip Carter <pcarter@fastmail.com>
@Tarmil
Copy link
Contributor Author

Tarmil commented Oct 3, 2019

Also might be worth mentioning that existing DUs that define IsXX members won't compile today, so this is safe from that perspective.

It is mentioned in Compatibility / Is this a breaking change?

Tarmil and others added 2 commits October 5, 2019 01:14
@Tarmil Tarmil changed the title Add "Make .Tag and .Is* discriminated union properties visible from F#" Add "Make .Is* discriminated union properties visible from F#" Oct 4, 2019
Copy link
Member

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Almost there. This will need a snippet describing compatibility with the existing Is* members you have access to on the Option<'T> and ValueOption<'T> types.


* If this is a change or extension to FSharp.Core, what happens when previous versions of the F# compiler encounter this construct?

It is not directly an extension to FSharp.Core, but it does apply to union types declared in FSharp.Core: `option`, `Result`, `Choice`. Previous versions of the F# compiler do not allow invoking the generated members on these types.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't true for Option<'T> or ValueOption<'T>. See here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option<'T> is easy: IsSome and IsNone are already visible, so there is no change.

For ValueOption<'T>, there are also visible IsSome and IsNone, in addition to the hidden IsValueSome and IsValueNone. So there are two possible routes:

  • make them visible (which makes them redundant with the existing IsSome/IsNone)
  • keep them hidden (which makes ValueOption<'T> unique among all union types for not having Is{{UnionCaseName}} properties)

I think I'm in favor of the former, for consistency. Plus it's probably simpler to implement than making an exception.

Copy link
Member

Choose a reason for hiding this comment

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

I assume we can't remove the existing IsSome/None, but adding IsValueSome/None would effectively make them aliases of the former, in which they would then probably be unique in our ecosystem.

It's a tough call. For parity you would like to have them, but different members that do the exact same thing brings confusion, and unless the tooltip would clearly describe that they are synonyms, I'm afraid programmers will have to research what the differences are and you'll end up answering questions similar to whether String or string has better performance / should be preferred / has different semantics etc.

Of course, the answers will be simple, they're equal, but I think it's better to prevent the questions rising in the first place.

But then there's the argument of parity, auto generated code, predictability, orthogonality etc. It's a tough call ;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the exact stance on obsoleting API is, but it could be acceptable to mark the existing ValueOption IsSome/IsNone properties [<Obsolete>] to reduce confusion.

Copy link
Member

Choose a reason for hiding this comment

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

@Tarmil I think your preference here is spot on: hiding IsValueSome/IsValueNone for ValueOption. Can you make a note of that in the RFC?

@abelbraaksma
Copy link
Member

I like the RFC, it's quite clear I think, but I do have a few general remarks / requests:

  • should we add a discussion thread for the RFC, as many other RFC's have, as opposed to just the original feature request?
  • what to do about exposure to null? That is, if Foo.IsCase is a property, x.IsCase could have x point to null. Not in F#, but if the property is exposed to, say, C# from F#. Do we consider that an acceptable NRE, or should we do something else? Would the property also work on DU's where null is a true value (like with IsNone)?
  • The texts talks about compiler-generated properties, but don't we already have them and don't they simply need to be exposed instead?
  • I'm a bit fuzzy about the section on Alternatives, sounds much to me like an unresolved issue: do we, or don't we expose Tag in some or more ways? And I think it is already exposed to C# anyway.
  • The proposal talks about nested types, but this feels a bit odd, considering that F# does not allow creation of nested types (but I don't have a better alternative).
  • There's mention that conflicts won't arise. I agree, but it might be good to mention that extension properties may exist, or other bindings with the same name and module/type (but not instance members). Current rules could potentially break such code, because extension members come last in resolution. Not a big thing and I don't know if it exists in the wild, but we might want to point it out.

@dsyme dsyme merged commit 31fae8a into fsharp:master Jan 17, 2020
@dsyme
Copy link
Contributor

dsyme commented Jan 17, 2020

The "natural" way of implementing this is to generate the Is/Tag properties earlier (into the TAST) rather than in IlxGen.fs. I don't think this technique causes any compat problems but this should be considered when reviewing the implementation.

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

Successfully merging this pull request may close these issues.

5 participants