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

Better diagnostics when using "as alias" in binding without wrapping expression in parens #5259

Open
Tracked by #1103
smoothdeveloper opened this issue Jun 29, 2018 · 6 comments
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Milestone

Comments

@smoothdeveloper
Copy link
Contributor

When giving alias via as, the first element of a tuple can be matched without wrapping it between parens, while the same doesn't work for any other items in the tuple.

let foo a b = 
  match a, b with
  | Some _ as aa, None -> aa, None
  | None, Some _ as bb -> None, bb // doesn't compile

This expression was expected to have type ''a option' but here has type ''b option * 'c option'

if I wrap the second tuple element in the match in parens, it compiles:

let foo a b = 
  match a, b with
  | Some _ as aa, None -> aa, None
  | None, (Some _ as bb) -> None, bb

This seems to be inconsistent behaviour, failing principle of least surprise, and leading to confusing error message.

Is it according to spec or some issue that needs to be fixed? Should the parens be mandatory or optional in that case?

Reproduces in VS2015 and VS2017.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jun 29, 2018

Yes, I've noticed that too a few times. But I heard somewhere that the syntax with parens in patterns is limited and somewhat special. It probably has to do with disambiguation, but I'm not sure. A related issue is that parens cannot be used to capture arguments for active patterns (weird, but true): #4828

@cartermp
Copy link
Contributor

This is likely due to disambiguation, so it could be by design.

@smoothdeveloper
Copy link
Contributor Author

Ok, I'm starting to understand how the error message comes to be, it is taking Some _, None and giving it an alias.

This is still rather confusing, wondering if we can make it any better but it seems making it consistent either way would be a breaking change, so we need to figure out if something can be done with the error message.

@cartermp
Copy link
Contributor

@isaacabraham this seems like a good candidate for tracking better error messages. The issue here is that we cannot disambiguate, but because the code is a valid alias for the whole pattern, we emit an error that may be misleading.

@cartermp cartermp added this to the Unknown milestone Aug 25, 2018
@dsyme dsyme added Area-Compiler-PatternMatching pattern compilation, active patterns, performance, codegen and removed Area-Compiler labels Mar 31, 2022
@dsyme dsyme added Area-Diagnostics mistakes and possible improvements to diagnostics and removed Area-Compiler-PatternMatching pattern compilation, active patterns, performance, codegen labels Apr 20, 2022
@dsyme dsyme changed the title Inconsistent error when using "as alias" in binding when matching tuples, without wrapping expression in parens Better diagnostics when using "as alias" in binding without wrapping expression in parens Apr 20, 2022
@vzarytovskii vzarytovskii moved this to Not Planned in F# Compiler and Tooling Jun 17, 2022
@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented Dec 15, 2023

This follows from the fact that as has lower precedence than , (and basically everything else), and that , is non-associative:

A as B, C as D

is equivalent to

((A as B), C) as D

One way to avoid parentheses in the original example is to use & instead of as, since & has higher precedence than ,:

let foo a b = 
  match a, b with
  | Some _ & aa, None -> aa, None
  | None, Some _ & bb -> None, bb

As for a more helpful error message for the original example with as, though, I suppose that during pattern match compilation you'd need to recognize the fact that the match branches' return types could not be unified, together with the fact that the result expressions involved values bound using as.

I notice that the keyword description for as, visible when hovering over as in an editor, is currently this:

keywordDescriptionAs,"Used to give the current class object an object name. Also used to give a name to a whole pattern within a pattern match."

Would it be worth updating "also used to give a name to a whole pattern within a pattern match" to point out that it has very low precedence and will thus bind everything to its left?

@smoothdeveloper
Copy link
Contributor Author

@brianrourkeboll I still fumble on this error when using as along with some other friction with coallescing several patterns (| A ((data1,_) as data) when someThing data && somethingElse data1 | B -> () and not using bindings in the branch).

Maybe if there is a revisit of pattern matching to enable as, when but dismissing the bound values in the branch and several patterns, it would be good time to improve error message for non unifiable pattern due to as and the precedence you highlight (assuming it is somehow related / coupled in the impl.), but on the get go, it seems tricky to identify (and to make a "nice to have" codefix), and the user can easily hover the patterns and see the type mismatching.

I think when I logged it, it was more of "principle of least surprise" approach (which is subjective / debatable) and confirming it is per the spec, more than being stuck with it, only experienced users commented here / no "newly adopting F#" users came here.

"Wrap in parens" related issues and suggestions maybe should be tagged, considering there are compound adjustments to the parser that could be taken in consideration as a top level feature, and tackling few language suggestions at once.

I don't feel it is important for this specific issue.

Would it be worth updating "also used to give a name to a whole pattern within a pattern match" to point out that it has very low precedence and will thus bind everything to its left?

Adding note that it has low precedence, which can mandate disambiguation through wrapping the binding with parens (or something shorter, more english obviously) would make sense, I agree and this is an easy adjustment 🙂

Let see if there are good suggestion to complete the message on as keyword but technically, this is a closed issue (by design / per spec), as far as I'm concerned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Projects
Status: New
Development

No branches or pull requests

5 participants