-
Notifications
You must be signed in to change notification settings - Fork 28
SIP-60 - Alternative bind patterns #74
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well written SIP, thank you! One thing that would be worth investigating is: how does this interact with quoted patterns? /cc @nicolasstucki I think it's fine to just not support them at least initially, but that should be explicit in the SIP.
case Baz(i: Int) | ||
|
||
def fun = this match | ||
case Bar(x @ given String) | Baz(x @ given Int) => ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would stay simple here and require the types to match, the user can always write given (Int | String)
or use a type alias to avoid repetition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was originally my intention, but I didn't like the fact that it "clashes" with the rules set up for named bindings and thought it might be better to be consistent with the named binding rule and hence wouldn't have to carve out an exception in the specification.
I don't feel strongly either way; in my opinion if I was binding a given
as a union type, I would use an alias or write it out explicitly anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing it in the SIP meeting, we feel that it might be best to not include support for given bind variables in alternatives at least initially, since it opens a can of worms and it's not clear if there is a lot of usecases for it. (But for the record, it seems that if we wanted to support them we would have to require the types to be identical to handle case Foo(given A, given B) | Bla(given B, given A)
which works out because the givens get desugared into given_A @ _
, given_B @ _
).
If $`X_{n,i}`$, is the type of the binding $`x_i`$ within an alternative $`p_n`$, then the consequent type, $`X_i`$, of the | ||
variable $`x_i`$ within the pattern scope, $`\Gamma`$ is the least upper-bound of all the types $`X_{n, i}`$ associated with | ||
the variable, $`x_i`$ within each branch. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the spec doesn't mention the effect of the presence of Ti on the pattern scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the specification isn't complete for Scala 3 (in order for the amendments to be complete, I'd first need to change the specification to reflect the current behaviour in dotty), should I include the changes to the spec to reflect the current behaviour within this SIP as well? Or is a separate PR to the spec enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate PR to the spec would be great and highly appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant PR is here.
For the record, should the synthesized names part of the spec or are they an implementation detail i.e. should the following be valid code according to the spec? It currently compiles.
type MyString = String
("foo", "foo") match:
case (given MyString, given String) => ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I'd say they should be part of the spec since they have to be for non-local definitions anyway (for ABI stability) and since it doesn't seem like we need to treat them differently in local patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be in favour of that interpretation.
It leads to a more straightforward interpretation of both:
x @ given String | y @ given String
and
given MyString | given String
Both are invalid now due to fact that the names introduced in each branch are different (if we don't take the alternative for missing variables), regardless of whatever instances are available in the implicit scope.
I can change the given bind variables section to better reflect this interpretation if that would be useful.
About quoted patterns
For the sake of this SIP, I would say that we do not support quoted patterns. That said, the current specification should also work for quoted patterns (maybe partially). I would assume that this would be one of the use-cases
The content of expression splices (the Type variables in quoted patterns are another story, I am not sure if we can apply the same specification to these constructs, we would have to check if it is sound or can lead to confusion on the user level. |
Co-authored-by: Guillaume Martres <smarter@ubuntu.com>
|
||
On the other hand, if it is decided that each bound variable ought to be the same type, then arguably "sharing" explicit type ascriptions across branches would reduce boilerplate. | ||
|
||
#### Missing variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding missing variables, we need a decision. The spec should either specify this is an error or assign the type A | Null
. Because currently explicit nulls are under a flag, I'm inclined to think that this should be an error at least until explicit nulls are always enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with explicit nulls I would not mix null
with other features like it would be done here. I think it is entirely reasonable to make this an error.
|
||
Arguably, missing a variable entirely is more likely to be an error — the absence of a requirement for `var` declarations before assigning variables in Python means that beginners can easily assign variables to the wrong variable. | ||
|
||
It may be, that the enforcement of having to have the same bind variables within each branch ought to be left to a linter rather thana a hard restriction within the language itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo "thana"
~~~ | ||
|
||
For the expression to make sense with the current semantics around pattern matches, `z` must be defined in both branches; otherwise the | ||
case body would be nonsensical if `z` was referenced within it (see [missing variables](#missing-variables) for a proposed alternative). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link to the "Missing variable" section does not seem to work.
The SIP committee voted to accept this proposal as experimental, congratulations! There was consensus among the committee for the following changes:
@yilinwei Would you be willing to make these changes? |
Certainly; I am happy to make the changes. I am a little busy at the moment, so will get to within the next month or so. |
Sounds good, thanks! |
Hi @yilinwei, do you think you'll have time to make those changes? Otherwise no worries, I should be able to find some time soon. |
This is the associated contributors thread.
The linked implementation is incomplete - I am happy to provide a more complete implementation at a later stage of the process.