Make .Is* discriminated union properties visible #16341

kerams · 2023-11-25T11:39:27Z

Implements fsharp/fslang-suggestions#222, attempts to finish #11394.

vzarytovskii · 2023-11-25T12:47:58Z

@kerams you're the MVP, thanks for reviving it! I remember I've hit some verification issues (ilverify tests iirc), and didn't have much time to investigate. Let me know if you'll hit them as well and will want some help looking into them.

src/Compiler/TypedTree/TcGlobals.fs

edgarfgp · 2023-11-25T15:00:45Z

It would be good to add some tests when using lowercase DU ie:

[<RequireQualifiedAccess>]
type X =
 | A
 | a

where we would have IsA and Isa

kerams · 2023-11-25T16:29:33Z

@psfinaki, how would I go about constraining which symbols can be renamed? Entities that are either compiler generated or implied should not offer the rename context menu option. Maybe I can return an empty result from GetRenameInfoAsync?

On second thought, maybe rename should be allowed here?

smoothdeveloper · 2023-11-25T16:55:52Z

@kerams, a shot in the dark but:

fsharp/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs

Line 167 in b4dc66c

yield FSharpInlineRenameLocation(document, textSpan)

https://github.com/dotnet/fsharp/blob/b4dc66c3d3cd4b45614d2b1b903890c9c846895a/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs#L102C16-L102C25

vzarytovskii · 2023-11-25T17:14:18Z

@kerams, a shot in the dark but:

fsharp/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs

Line 167 in b4dc66c

yield FSharpInlineRenameLocation(document, textSpan)

https://github.com/dotnet/fsharp/blob/b4dc66c3d3cd4b45614d2b1b903890c9c846895a/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs#L102C16-L102C25

That would work, probably, from the VS language service perspective, not sure if @kerams asked about universal service solution.

vzarytovskii · 2023-11-25T17:16:23Z

@psfinaki, how would I go about constraining which symbols can be renamed? Entities that are either compiler generated or implied should not offer the rename context menu option. Maybe I can return an empty result from GetRenameInfoAsync?

On second thought, maybe rename should be allowed here?

Thinking about it now. I'm a bit confused. You mean if we should allow renames on Is* members?

kerams · 2023-11-25T17:34:01Z

Yes. On the one hand, if you decide to rename a union case, it should be possible to rename Is properties too in order to get them in sync. On the other hand, renaming something that the compiler generated generated is fishy. Ideally a rename of either should rename both, but I am not sure how hard that would be.

I might just ignore this issue for now.

vzarytovskii · 2023-11-25T17:51:03Z

Yes. On the one hand, if you decide to rename a union case, it should be possible to rename Is properties too in order to get them in sync. On the other hand, renaming something that the compiler generated generated is fishy. Ideally a rename of either should rename both, but I am not sure how hard that would be.

I might just ignore this issue for now.

Yeah, it's a tough issue. I agree that we can ignore it for now.

smoothdeveloper · 2023-11-25T19:23:29Z

on the rename front, there is also the subtlety of renaming a property to the compiler generated name for an existing case.

I think the most important is just to prevent a rename on the call to .Is*, all considering the rest as pandora boxes.

kerams · 2023-11-26T09:39:25Z

One more thing I am unsure about. If assembly A is compiled with lang version preview and exposes a union, assembly B with a reference to A has access to that union's case testers even if it uses a lower language version.

Finally, these members are currently not surfaced without using the right language version, meaning we don't an error guiding you to update. You also don't get to use the members in 3rd party libraries, even if they have been recompiled, but without the approriate language version.

Perhaps it would be best to always surface the members in metadata, but add a usage check in CheckExpressions? That would address both issues in this comment. (Older compilers will likely see these properties either way (see #11394 (comment)), and existing libraries will still need to be recompiled in order to expose union case testers)

T-Gro · 2023-11-27T09:36:32Z

One more thing I am unsure about. If assembly A is compiled with lang version preview and exposes a union, assembly B with a reference to A has access to that union's case testers even if it uses a lower language version.

Finally, these members are currently not surfaced without using the right language version, meaning we don't an error guiding you to update. You also don't get to use the members in 3rd party libraries, even if they have been recompiled, but without the approriate language version.

Perhaps it would be best to always surface the members in metadata, but add a usage check in CheckExpressions? That would address both issues in this comment. (Older compilers will likely see these properties either way (see #11394 (comment)), and existing libraries will still need to be recompiled in order to expose union case testers)

I personally believe that the language check should be only done at the producer of the DU.
The consumer should accept it and be able to consume it, even if the consumer is on an older language version.

After all, the 'producer' could write similar tester properties by hand as of now already.

kerams · 2023-11-27T10:08:05Z

After all, the 'producer' could write similar tester properties by hand as of now already.

Similar, yes, but not identical.

Assemblies emit IL for these members already, so there is no run-time difference. We're only talking about metadata. Imagine you are consuming a third party assembly whose author updates the SDK, but doesn't want to bump the language version. If the feature check is on the producer side, you wouldn't be able to use case testers from that assembly in your code, even if you use 'preview'. At the same time, whether or not the case testers are visible outwardly makes no difference to the author, because they exist regardless.

src/Compiler/Checking/AugmentTypeDefinitions.fs

T-Gro · 2023-11-27T11:38:36Z

After all, the 'producer' could write similar tester properties by hand as of now already.

Similar, yes, but not identical.

Assemblies emit IL for these members already, so there is no run-time difference. We're only talking about metadata. Imagine you are consuming a third party assembly whose author updates the SDK, but doesn't want to bump the language version. If the feature check is on the producer side, you wouldn't be able to use case testers from that assembly in your code, even if you use 'preview'. At the same time, whether or not the case testers are visible outwardly makes no difference to the author, because they exist regardless.

The code owned by the producer might already be providing similar checks (maybe with some additional features/logic) and wouldn't want to expose the automatically created getters to keep the recommended API layer clean (for F# consumers at least). Keeping autocomplete results clean might be one aspect to it.

The producer should be able to control the augmentation via attributes as is possible with existing augmentations.
Deferring the languagefeature check to the consumer would lead to a situation of the producer being out of control.

I do realize this depends on one's opinion on 'adding to the offered F# API with hopefully harmless properties'. From the runtime perspective, it does not matter of course. But considering the tooling aspect with autocomplete and API discovery, I do see a difference.

src/Compiler/Checking/infos.fs

tests/FSharp.Compiler.ComponentTests/Language/DiscriminatedUnionTests.fs

psfinaki · 2023-11-27T15:43:42Z

On the rename front, I'd say there are two premises here:

Renaming DU fields is a fundamental tooling capability, hence should be possible
Any tooling action, if offered, shouldn't break the code / break the code even further

I haven't deeply looked into the implementation yet, however that's what I'd probably consider an ideal tooling behavior.

Given the code:

type Contact =
    | Email of address: string
    | Phone of countryCode: int * number: string

type Person = { name: string; contact: Contact }

let canSendEmailTo person =
    person.contact.IsEmail

Renaming Email to EmailAddress in the DU works and also renames IsEmail to IsEmailAddress
Renaming IsEmail to IsEmailAddress produces some kind of popup like "hey, this is a generated thing, please rename the original field and this will be renamed as well"

Now, since I don't expect 2) to be a common scenario and it's hard to implement, I'd be fine if that just wasn't offered for renaming. If that's offered but doesn't do anything - well, we can live with it. That still feels better to me than having that working in a way that it would automatically rename the original field everywhere.

That's just my perspective. Maybe @0101 you have some ideas.

On that note, if you @kerams save renaming files in this PR for a followup, reviewers will appreciate it :)

And thanks for your work.

vzarytovskii

That's great, thanks!

auduchinok · 2023-12-07T13:26:46Z

@kerams Could you please add some tests for intersection with interface members? Sorry if I've missed them in the test
data.

type I =
    abstract IsA: bool

type U =
    | A
    | B

    member this.IsA = false

    interface I with
        member this.IsA = this.IsA

type I =
    abstract IsA: bool

type U =
    | A
    | B

    interface I with
        member this.IsA = this.IsA

type I =
    abstract IsA: bool

type U =
    | A
    | B

    member this.IsA = 1

    interface I with
        member this.IsA = this.IsA

src/Compiler/CodeGen/EraseUnions.fs

psfinaki · 2023-12-06T17:30:06Z

src/Compiler/Checking/NicePrint.fs

@@ -1933,6 +1933,9 @@ module TastDefinitionPrinting =
        let props =
            GetImmediateIntrinsicPropInfosOfType (None, ad) g amap m ty
            |> List.filter (fun pinfo -> shouldShow pinfo.ArbitraryValRef)
+            // Filter out 'IsA' properties which are implied by the union cases since they don't need to be displayed


Just for my education - what are some examples of the printed outputs here? As in, are we sure it doesn't ever make sense to show these?

Any DU properties you define.

psfinaki · 2023-12-06T17:33:36Z

src/Compiler/Checking/CheckExpressions.fsi

@@ -578,6 +578,10 @@ val PublishTypeDefn: cenv: TcFileState -> env: TcEnv -> mspec: Tycon -> unit
 /// Publish a value definition to the module/namespace type accumulator.
 val PublishValueDefn: cenv: TcFileState -> env: TcEnv -> declKind: DeclKind -> vspec: Val -> unit

+/// Publish a value definition to the module/namespace type accumulator.


Probably should be decopypasted :)

psfinaki · 2023-12-07T12:36:19Z

src/Compiler/Checking/CheckDeclarations.fs

+// Build augmentation declarations
+//------------------------------------------------------------------------- 
+
+module AddAugmentationDeclarations =


It would be nice to separate this to a separate file so that CheckDeclarations keeps at least somewhat manageable size.

I mean I know it was there so you don't have to :) Just since you've looked into that, you might know if it is easy to do or not.

psfinaki · 2023-12-07T12:45:44Z

tests/FSharp.Compiler.ComponentTests/Language/DiscriminatedUnionTests.fs

+        |> shouldSucceed
+
+    [<FSharp.Test.FactForNETCOREAPP>]
+    let ``Lowercase Is* discriminated union properties are visible, proper values are returned`` () =


Can you maybe also add a sanity check test that "isABC" doesn't work?

psfinaki · 2023-12-07T12:48:49Z

tests/FSharp.Compiler.ComponentTests/Language/DiscriminatedUnionTests.fs

+type Foo = | Foo of string | ``Mars Bar``
+let foo = Foo.Foo "hi"
+if not foo.IsFoo then failwith "Should be Foo"
+if foo.``IsMars Bar`` then failwith "Should not be ``Mars Bar``"


Interesting. Thinking about it - this is probably a viable approach, but just curious - how hard would it be to rather have foo.Is``Mars Bar`` or to ban such elements from having Is property altogether?

Is``Mars Bar`` is not a valid identifier.

tests/FSharp.Compiler.ComponentTests/Language/DiscriminatedUnionTests.fs

psfinaki · 2023-12-07T14:04:37Z

Damn my review came too late :)

chkn · 2023-12-07T15:06:20Z

@kerams Oh this is great! Thanks for bringing this puppy home

T-Gro · 2023-12-14T13:33:08Z

tests/FSharp.Compiler.ComponentTests/Language/DiscriminatedUnionTests.fs

+        |> withLangVersionPreview
+        |> typecheck
+        |> shouldFail
+        |> withErrorMessage "The type 'ValueOption<_>' does not define the field, constructor or member 'IsValueNone'. Maybe you want one of the following:


@kerams :

I am having a problem with this test in a different PR and I cannot get my head around it - how is it ensured, that the generation is prevented for ValueOption?
Afterall, it does NOT have DefaultAugmentation(false).

What am I missing?
I tried to scan this PR's diff for any kind of logical condition that would skip IsValueNone and IsValueSome for ValueOption, but I do not see it.

While the case testers exist, they aren't exposed in prim-types.fsi.

And this PR does not add them automatically to anything in prim-types unless specified, or how does it work?
i.e. what condition is in place to make sure that ValueOption does NOT get them and a user-defined DU gets them?

Or is it done based on exposing/not exposing in a signature file?
Meaning that all users of signature files (not many, I admit) will only benefit from this feature if they manually add the testers to .fsi?

(this might be relevant for docs)

Yes, if there's a signature file, case testers need to be exposed manually. The RFC mentions it briefly.

Thanks, got it.

For some reason, I have it keep coming in the nullness PR (e.g. ValueOption does come up with the IsValueNone tester), not sure what is so different over there.

vzarytovskii and others added 13 commits August 10, 2022 20:12

Merge?

3e7a522

Fantomas

ba0a744

Fix naming

a7e8f23

fixes

808d5b4

Translations + tests fixes

1c176fa

Fixed tests

de5dd73

Merge remote-tracking branch 'upstream/main' into expose-du-props

7c0f4dd

Fix code formatting

453b79b

Fix the build

fd23df7

Merge

d861caf

Add backticks test

c46df5e

Remove unused code

c03e3cf

Format

13891f0

kerams requested a review from a team as a code owner November 25, 2023 11:39

kerams added 2 commits November 25, 2023 13:58

Fix test

de1b8a1

Fix surface area

2b18f4f

kerams commented Nov 25, 2023

View reviewed changes

src/Compiler/TypedTree/TcGlobals.fs Show resolved Hide resolved

Add more tests

a504fa5

T-Gro reviewed Nov 27, 2023

View reviewed changes

src/Compiler/Checking/AugmentTypeDefinitions.fs Outdated Show resolved Hide resolved

T-Gro reviewed Nov 27, 2023

View reviewed changes

src/Compiler/Checking/infos.fs Show resolved Hide resolved

T-Gro reviewed Nov 27, 2023

View reviewed changes

tests/FSharp.Compiler.ComponentTests/Language/DiscriminatedUnionTests.fs Show resolved Hide resolved

kerams added 2 commits November 29, 2023 20:55

Revert rename

c5f8ee0

Add more tests

3323fd7

kerams requested a review from T-Gro December 1, 2023 12:49

Merge branch 'main' into props

aadca65

vzarytovskii enabled auto-merge (squash) December 7, 2023 13:14

vzarytovskii approved these changes Dec 7, 2023

View reviewed changes

T-Gro approved these changes Dec 7, 2023

View reviewed changes

vzarytovskii merged commit 9123c41 into dotnet:main Dec 7, 2023
26 checks passed

psfinaki reviewed Dec 7, 2023

View reviewed changes

kerams deleted the props branch December 7, 2023 14:10

kerams mentioned this pull request Dec 13, 2023

Latest Fable fails to compile Fable.React fable-compiler/Fable#3658

Closed

T-Gro reviewed Dec 14, 2023

View reviewed changes

allisonchou mentioned this pull request Dec 15, 2023

[Automated] PRs inserted in VS build main-34415.28 #16445

Closed

T-Gro mentioned this pull request Jan 22, 2024

Align DU case augmentation with previous behavior in EraseUnions #16571

Merged

ncave mentioned this pull request Jun 12, 2024

IsUnionCaseTester throwing an error #17301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make .Is* discriminated union properties visible #16341

Make .Is* discriminated union properties visible #16341

kerams commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023

edgarfgp commented Nov 25, 2023

kerams commented Nov 25, 2023 •

edited

Loading

smoothdeveloper commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023

kerams commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023 •

edited

Loading

smoothdeveloper commented Nov 25, 2023

kerams commented Nov 26, 2023 •

edited

Loading

T-Gro commented Nov 27, 2023

kerams commented Nov 27, 2023 •

edited

Loading

T-Gro commented Nov 27, 2023

psfinaki commented Nov 27, 2023

vzarytovskii left a comment

auduchinok commented Dec 7, 2023 •

edited

Loading

psfinaki Dec 6, 2023

kerams Dec 7, 2023

psfinaki Dec 6, 2023

psfinaki Dec 7, 2023

psfinaki Dec 7, 2023

psfinaki Dec 7, 2023

kerams Dec 7, 2023

psfinaki commented Dec 7, 2023

chkn commented Dec 7, 2023

T-Gro Dec 14, 2023

kerams Dec 14, 2023

T-Gro Dec 15, 2023

T-Gro Dec 15, 2023

kerams Dec 15, 2023

T-Gro Dec 20, 2023

Make .Is* discriminated union properties visible #16341

Make .Is* discriminated union properties visible #16341

Conversation

kerams commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023

edgarfgp commented Nov 25, 2023

kerams commented Nov 25, 2023 • edited Loading

smoothdeveloper commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023

kerams commented Nov 25, 2023

vzarytovskii commented Nov 25, 2023 • edited Loading

smoothdeveloper commented Nov 25, 2023

kerams commented Nov 26, 2023 • edited Loading

T-Gro commented Nov 27, 2023

kerams commented Nov 27, 2023 • edited Loading

T-Gro commented Nov 27, 2023

psfinaki commented Nov 27, 2023

vzarytovskii left a comment

Choose a reason for hiding this comment

auduchinok commented Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psfinaki commented Dec 7, 2023

chkn commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerams commented Nov 25, 2023 •

edited

Loading

vzarytovskii commented Nov 25, 2023 •

edited

Loading

kerams commented Nov 26, 2023 •

edited

Loading

kerams commented Nov 27, 2023 •

edited

Loading

auduchinok commented Dec 7, 2023 •

edited

Loading