-
Notifications
You must be signed in to change notification settings - Fork 205
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
Adjust several locations where required types are discussed #2884
Conversation
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
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.
lgtm assuming some clarifying informative text is added, see my comment.
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.
This looks great. Thank you for clarifying it!
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
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.
Review response.
The paragraph about type checks applied to relational expressions had to be rewritten extensively, because of the ordering dependencies (e.g., we need to find the operator and the type of the operator parameter before we can type check the constant expression).
I used the text from @stereotype441 as a starting point for a small section about coercions.
I've weaseled around the distinction between the two models (coercion == transformation of an expression in-situ, vs. coercion == semantic step where the same operation is performed, but presumably on a temporary variable which is a built-in property of the semantics of the given pattern matching, assignment, etc.).
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
Thanks for feedback! @lrhn, do you have further comments? |
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
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.
Review response
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/0546-patterns/feature-specification.md
Outdated
Show resolved
Hide resolved
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.
Review response.
I don't think we're in a position to introduce a notion of "promoted matched value type after failed match" with ||
patterns. Otherwise I think the issues have been resolved.
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.
Still LGTM.
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.
Latest review response: Logical-or patterns now do require promotions based on a failing match in the first operand to be taken into account.
This PR changes several locations in the patterns feature specification where the 'required type' of a pattern is discussed. It adds rules about the required type in some cases where it was left unspecified. In particular, it says that the constant expression in a relationalPattern is type checked with the context type
_
(I can't see that we have anything else). The required type of a relationalPattern is kept open (Object?
). The actual checks associated with this kind of pattern are performed during the traversal of the pattern, and there is no single typeT
would would make the same checks occur just by being the matched value type.The changes include a somewhat controversial part: The definition of "assignable". The new text changes the order of the two bullet points where assignability is discussed: First we check whether there is a need for a coercion, and insert it if needed, and obtain a new
M
(soM
might have beenX Function<X>(X)
and is nowint Function(int)
). Next, we check thatM
is assignable to the required type of the pattern.This approach is compatible with both definitions of 'assignable' (both the one in the nnbd feature specification and the language specification, and the one that includes coercions): We just don't rely on 'assignability' to take coercions into account, and then it doesn't matter whether or not they are part of 'assignability'. ;-)
Other changes:
I changed all references to primitive equality to use the phrase 'primitive equality'. There are several locations where 'operator
==
' is mentioned specifically, and some of them have been preserved, because it was actually specifically about this operator.I added a few words to indicate that 'pattern type checking' can include provision of missing parts (so
[var j]
could be transformed into<double>[var j]
at this time, based onM
).