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

Compiler allows non-functions to be bound to active pattern names #17190

Open
brianrourkeboll opened this issue May 20, 2024 · 8 comments
Open
Assignees
Labels
Area-Compiler-Checking Type checking, attributes and all aspects of logic checking Bug help wanted Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Milestone

Comments

@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented May 20, 2024

There are a couple of ways in which it is possible to define "active patterns" that are (from the F# perspective) values, not functions. While the tooling treats them as active patterns, it is impossible to actually use them as patterns in any pattern-matching position.

Expected behavior

The compiler disallows defining a multi-case active pattern that is not a function:

let (|P|Q|) = Choice<unit, unit>.Choice1Of2 ()
// error FS1209: Active pattern '|P|Q|' is not a function

I would expect the same for single-case and partial active patterns.

Actual behavior

(|P|)

A value bound to what looks like a single-case active pattern name is actually compiled as a property with a getter (when a module-bound value):

let (|P|) = 3
// val (|P|) : int = 3

image

(|P|_|)

An option or value option value bound to a partial active pattern name is compiled to the exact same .NET method as it would be if it were defined as a function, but from the F# perspective it is a value (or a type function, even though you cannot give it explicit type arguments), not a function, although the tooling again treats it as an active pattern.

let (|P|_|) = None
// val (|P|_|) : 'a option

image

let (|P|_|) = ValueNone
// val (|P|_|) : 'a voption

image

Unfortunately, updating the compiler to disallow these scenarios would be a breaking change — someone could be passing values so defined around qua values (f (|P|), let r = (|P|) + (|Q|), etc.) — although I doubt anyone actually intentionally binds values to active pattern names like this in normal code.

@edgarfgp
Copy link
Contributor

A warning behind a lang feature like we have done in other cases might work here ?

@brianrourkeboll
Copy link
Contributor Author

brianrourkeboll commented May 20, 2024

A warning behind a lang feature like we have done in other cases might work here ?

Yeah, I think that would probably be the best we could do.

@brianrourkeboll
Copy link
Contributor Author

brianrourkeboll commented May 20, 2024

It turns out that members with active pattern names are also allowed (SharpLab) and are not treated as patterns at all by the tooling:

[<AutoOpen>]
type Foo =
    static member (|P|) = 3
    static member (|Q|_|) = 3
    static member (|R|_|) = None
    static member (|S|_|) = ValueNone
    static member (|T|U|) = 3

image

Anonymous records, too:

let x = {| (|P|) = 3 |}

(Parens are gone in tooling, though)

image

And constraint calls:

let inline f x = (^a : (member (|P|) : int) x)
f {| (|P|) = 3 |}

And lambdas:

let f : int -> int = fun (|P|) -> (|P|) + (|P|)

Or any other pattern-matching context:

match 3 with
| (|P|) -> (|P|) + (|P|)

Also unions:

[<RequireQualifiedAccess>]
type T =
    | (|P|) of int
    | (|Q|)

I guess it's a similar phenomenon to the one that allows you to bind operator names to values:

let y = {| (||) = 3 |}
match (+) with (|++|) -> 2 |++| 2
let f (>*^*<) x y = x >*^*< y
match (+) 3 with (~+) & (~-) -> +(-(2)) + -(9) // 20.

They're all just being lexed as IDENTs (or parenIdents), and then there's inconsistent treatment later on...

@abelbraaksma
Copy link
Contributor

Yes. from F#'s standpoint, these are just longident, and while they have special meaning in some scenarios, they are not illegal. Likewise, if you do have an active pattern, you can just use it as a HOF function argument, or in a function call.

I vaguely remember bringing this up ages ago, but I cannot remember the resolution at the time. We could (should?) issue a warning perhaps, instead of an error?

@abonie abonie added Bug Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code. Area-Compiler-Checking Type checking, attributes and all aspects of logic checking and removed Needs-Triage labels Jun 24, 2024
@edgarfgp
Copy link
Contributor

edgarfgp commented Jul 7, 2024

I think we should prevent users from using active patterns that are not functions.

@edgarfgp edgarfgp self-assigned this Aug 12, 2024
@edgarfgp
Copy link
Contributor

edgarfgp commented Aug 13, 2024

@vzarytovskii Would be ok to create a new language preview flag e.g. ImprovedActivePatternReporting and fix in multiple PR's some of the cases showed in this issue ?

@vzarytovskii
Copy link
Member

@vzarytovskii Would be ok to create a new language preview flag e.g. ImprovedActivePatternReporting and fix in multiple PR's some of the cases showed in this issue ?

I suppose, but I will still need to re-read the whole thread to freshen my memory

@edgarfgp
Copy link
Contributor

edgarfgp commented Aug 13, 2024

@vzarytovskii I think we can start with

let (|P|) = 3 // This compiles now. But should show `FS1209: Active pattern '|P|' is not a function` 

Why:

let (|P|) = 3 is not a function and can not even be used in match

Screenshot 2024-08-13 at 13 30 37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-Checking Type checking, attributes and all aspects of logic checking Bug help wanted Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Projects
Status: New
Development

No branches or pull requests

6 participants