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

Pattern discard allowed for union case that takes no data #13851

Closed
NinoFloris opened this issue Sep 6, 2022 · 10 comments · Fixed by #14055
Closed

Pattern discard allowed for union case that takes no data #13851

NinoFloris opened this issue Sep 6, 2022 · 10 comments · Fixed by #14055
Labels
Area-Compiler-PatternMatching pattern compilation, active patterns, performance, codegen Bug good first issue help wanted
Milestone

Comments

@NinoFloris
Copy link
Contributor

Match discards aren't type checked against case

    type X = X 

    let test() =
        match X with
        | X _ -> () // No error X doesn't have arguments
@NinoFloris NinoFloris added the Bug label Sep 6, 2022
@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 6, 2022

Duplicate of #10204 and #1592, which are not considered bugs, according to @dsyme.
Should probably be a lang suggestion instead?

@edgarfgp
Copy link
Contributor

edgarfgp commented Sep 6, 2022

It always triggers me that when refactoring a DU my code still still compilers where I’m using _ in pattern matching 😟. An error, warning might help here ?

@dsyme
Copy link
Contributor

dsyme commented Sep 7, 2022

Things have changed a bit since 2016, and we now are more inclined to emit new informationals and warnings, protected by a /langversion switch.

So I'd definitely be ok with emitting at least an informational, and possibly a warning for this case.

@NinoFloris
Copy link
Contributor Author

Was this originally missed or was this intended as a source compatibility feature? (being able to drop data in select scenarios)

Do informational diags pop up in IDEs? I wouldn't mind a warning tbh.

@vzarytovskii vzarytovskii added this to the Backlog milestone Sep 9, 2022
@vzarytovskii vzarytovskii moved this to Not Planned in F# Compiler and Tooling Sep 9, 2022
@vzarytovskii vzarytovskii added Area-Compiler-Checking Type checking, attributes and all aspects of logic checking good first issue help wanted labels Sep 9, 2022
@dsyme dsyme added Area-Compiler-PatternMatching pattern compilation, active patterns, performance, codegen and removed Area-Compiler-Checking Type checking, attributes and all aspects of logic checking labels Sep 23, 2022
@dsyme
Copy link
Contributor

dsyme commented Sep 23, 2022

Was this originally missed or was this intended as a source compatibility feature? (being able to drop data in select scenarios)

It crept in somewhere along the way

Do informational diags pop up in IDEs?

Yes they show, but very light decorations

@dsyme dsyme changed the title Match discards aren't type checked against case Pattern discard allowed for union case that takes no data Sep 23, 2022
@abelbraaksma
Copy link
Contributor

@edgarfgp great you’re trying to fix that old bug I reported 6yrs back in #1592. Once this is done, we should probably change the docs accordingly, see suggestion here on this issue: dotnet/docs#20858.

Btw, if we make this informational (not a warning), surely a langversion switch isn’t necessary?

@edgarfgp
Copy link
Contributor

edgarfgp commented Oct 9, 2022

@abelbraaksma Thanks for raising this years ago , This was confusing to me while I was learning . So I decided fix it :)

Once this is done, we should probably change the docs accordingly, see suggestion here on this issue: dotnet/docs#20858.

Yeah that is good idea. will update the docs if this gets merged

Btw, if we make this informational (not a warning), surely a langversion switch isn’t necessary?

I thought the same while I was implementing this and IMO a warning (no informational) will be more explicit and will help us remove those unnecessary wildcards. Also, it seems the informational warning is shown, but with very light decorations.

@T-Gro
Copy link
Member

T-Gro commented Oct 10, 2022

I agree that warning + language switch is better in making this more visible.
Info-warnings are use very sparsely, typically only when deprecating language features...

@vzarytovskii
Copy link
Member

I agree that warning + language switch is better in making this more visible.
Info-warnings are use very sparsely, typically only when deprecating language features...

Agree, I think it should be a warning behind language version. It should also be easy to add a quick fix to VS (and other IDEs) which will be removing unnecessary discards.

Repository owner moved this from Not Planned to Done in F# Compiler and Tooling Oct 10, 2022
@abelbraaksma
Copy link
Contributor

Awesome to see this fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-PatternMatching pattern compilation, active patterns, performance, codegen Bug good first issue help wanted
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants