IsUnionCaseTester throwing an error

[ncave]

2 issues with IsUnionCaseTester:

• Seems to be throwing an error, instead of returning false on non-matching values.
• Seems to be missing a case where an union case tester method is neither IsProperty nor IsMethod (but it is IsPropertyGetterMethod).

Perhaps it's missing the | V v -> ... case, maybe something like this?

member _.IsUnionCaseTester =
    checkIsResolved()
    match d with
    | P p -> p.IsUnionCaseTester
    | M m -> m.IsUnionCaseTester
    | V v -> v.IsPropertyGetterMethod && v.LogicalName.StartsWith("get_Is") // or something like it
    | E _ | C _ -> false // fixed to return boolean

Related to #16341
Tested on .NET 8.0, with <LangVersion>preview</LangVersion>.

[edgarfgp]

Would this be fixed in F# 9 ?

[vzarytovskii]

We probably should, yes.

[abonie]

@ncave Could you please share code that repros this?

[abonie]

I mean specifically how does this case looks like

• Seems to be missing a case where an union case tester method is neither IsProperty nor IsMethod (but it is IsPropertyGetterMethod).

[ncave]

@abonie Sure, here it is:

type HTMLAttr =
    | Href of string
    | Custom

let a = Custom
let b = a.IsCustom
printfn $"IsCustom = {b}"

Here, when inspecting the AST for the generated union case tester members get_IsCustom and get_IsHref, we can see the following:

• FSharpMemberOrFunctionOrValue.IsProperty = false
• FSharpMemberOrFunctionOrValue.IsMethod = false
• FSharpMemberOrFunctionOrValue.IsFunction = true
• FSharpMemberOrFunctionOrValue.IsPropertyGetterMethod = true
• FSharpMemberOrFunctionOrValue.IsUnionCaseTester = throws an exception, not good...

Based on these, see the OP above for a proposed fix in FSharpMemberOrFunctionOrValue.IsUnionCaseTester.

[ncave]

@abonie In other words, instead of what it is right now:

member _.IsUnionCaseTester =
    checkIsResolved()
    match d with
    | P p -> p.IsUnionCaseTester
    | M m -> m.IsUnionCaseTester
    | E _ | C _ | V _ -> invalidOp "the value or member is not a property"

should be something like this:

member _.IsUnionCaseTester =
    checkIsResolved()
    match d with
    | P p -> p.IsUnionCaseTester
    | M m -> m.IsUnionCaseTester
    | V v -> v.IsPropertyGetterMethod && v.LogicalName.StartsWith("get_Is") // added missing case
    | E _ | C _ -> false // fixed to return boolean

[abonie]

@ncave sorry I got a bit sidetracked last couple of days. I guess I wanted to know what F# code you are inspecting that results in IsProperty = false, IsMethod = false, IsFunction = true and IsPropertyGetterMethod = false. But you're saying you are looking at the AST from the IL that was generated for get_IsCustom, is that correct?

[ncave]

@abonie Yes, that is correct, see above for an example F# code, and the AST for the generated union case tester member get_IsCustom.

[abonie]

Right @ncave, sorry for being a little clueless here, but I can't figure out how you're grabbing the get_IsCustom symbol that makes this combination of IsMethod = false and IsPropertyGetterMethod = true happen.

Here's what I am trying in our unit tests to capture this, based on the snippet you shared:

[<Fact>]
let ``IsUnionCaseTester test`` () =
    FSharp """
module Lib

type HTMLAttr =
    | Href of string
    | Custom

let a = Custom
let b = a.IsCustom
printfn $"IsCustom = {b}"
let c = a.get_IsCustom
"""
    |> withLangVersionPreview
    |> typecheckResults
    |> fun results ->
        let isCustomSymbolUse = results.GetSymbolUseAtLocation(11, 22, "let c = a.get_IsCustom", [ "get_IsCustom" ]).Value
        match isCustomSymbolUse.Symbol with
        | :? FSharpMemberOrFunctionOrValue as mfv ->
            Assert.True(false, $"""IsProperty = {mfv.IsProperty}
IsMethod = {mfv.IsMethod}
IsFunction = {mfv.IsFunction}
IsPropertyGetterMethod = {mfv.IsPropertyGetterMethod}
IsValue = {mfv.IsValue}""")
        | _ -> failwith "Expected FSharpMemberOrFunctionOrValue"

(ignore the fact that this is not a good test, using this for exploration purposes atm)
This for example gives me IsMethod = true and IsPropertyGetterMethod = true. If I instead change the one line to grab a different symbol

let isCustomSymbolUse = results.GetSymbolUseAtLocation(11, 5, "let c = a.get_IsCustom", [ "c" ]).Value

then I get both IsMethod and IsPropertyGetterMethod = false

Trying to understand what's going on before deciding if the ad hoc bugfix is appropriate

[ncave]

@abonie Perhaps try inspecting the IsCustom symbol at this line let b = a.IsCustom.

[abonie]

I've started with that (it's IsMethod = false, IsPropertyGetterMethod = false). Is it possible for you to share the code that runs into IsMethod = false, IsPropertyGetterMethod = true? I mean both code that's being inspected and the code that does inspecting

[ncave]

@abonie Yes, while it is true that when using the symbol look up (GetSymbolUseAtLocation), you get the correct values, I would still argue that the correct behavior for FSharpMemberOrFunctionOrValue.IsUnionCaseTester is to return false when not matching the input, instead of throwing an exception. It's a boolean property, it shouldn't throw.

e.g. this code prints the correct values (but please see below for more):

    let isCustomSymbolUse =
        typeCheckResults.GetSymbolUseAtLocation(8, 18, inputLines.[7], [ "IsCustom" ])
    match isCustomSymbolUse with
    | Some v ->
        match v.Symbol with
        | :? FSharpMemberOrFunctionOrValue as mfv ->
            printfn $"""
--- when using GetSymbolUseAtLocation ---
{mfv.CompiledName}:
    IsProperty = {mfv.IsProperty}
    IsUnionCaseTester = {mfv.IsUnionCaseTester}
    IsMethod = {mfv.IsMethod}
    IsFunction = {mfv.IsFunction}
    IsPropertyGetterMethod = {mfv.IsPropertyGetterMethod}
    IsValue = {mfv.IsValue}"""
        | _ -> printfn "Expected FSharpMemberOrFunctionOrValue"
    | _ -> printfn "Expected Symbol"

will print the following:

--- when using GetSymbolUseAtLocation ---
get_IsCustom:
    IsProperty = True
    IsUnionCaseTester = True
    IsMethod = False
    IsFunction = False
    IsPropertyGetterMethod = False
    IsValue = False

But, if you enumerate the declarations in the AST, you can see a different story.
e.g. this code prints the incorrect values (which is what this PR is about):

    for impl_file in projectResults.AssemblyContents.ImplementationFiles do
        for file_decl in impl_file.Declarations do
            match file_decl with
            | FSharpImplementationFileDeclaration.Entity (ent, ent_decls) ->
                for ent_decl in ent_decls do
                    match ent_decl with
                    | FSharpImplementationFileDeclaration.MemberOrFunctionOrValue (mfv, args, body) ->
                        if mfv.CompiledName.StartsWith("get_Is") then
                            printfn $"""
--- when enumerating declarations ---
{mfv.CompiledName}:
    IsProperty = {mfv.IsProperty}
    IsMethod = {mfv.IsMethod}
    IsFunction = {mfv.IsFunction}
    IsPropertyGetterMethod = {mfv.IsPropertyGetterMethod}
    IsValue = {mfv.IsValue}"""
                    | _ -> ()
            | _ -> ()

will print this:

--- when enumerating declarations ---
get_IsHref:
    IsProperty = False
    IsMethod = False
    IsFunction = True
    IsPropertyGetterMethod = True
    IsValue = False

--- when enumerating declarations ---
get_IsCustom:
    IsProperty = False
    IsMethod = False
    IsFunction = True
    IsPropertyGetterMethod = True
    IsValue = False

And if you try to use mfv.IsUnionCaseTester, it will throw an exception.

[abonie]

Thanks @ncave, I can reproduce this now. I suspect this is a bug in these properties (and not in the IsUnionCaseTester) - it doesn't make sense that it would be not a method but a property getter method. I am looking into it.

I would still argue that the correct behavior for FSharpMemberOrFunctionOrValue.IsUnionCaseTester is to return false when not matching the input, instead of throwing an exception. It's a boolean property, it shouldn't throw.

I agree with this. It seems to me there are two separate issues here: throwing instead of returning false and the weird "not a method" corner case.