-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add support for new pattern kinds #873
base: draft-v8
Are you sure you want to change the base?
Conversation
0d6dda3
to
237570c
Compare
Successfully rebased on the updated |
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 haven't managed to work my way through all of this yet but am submitting what I have so far. Overall this seems to have carried over too much non-Standard from the original proposals and some issues repeating/rewording stuff already in the Standard.. There are also grammar issues that may break expression parsing…
|
||
```ANTLR | ||
switch_expression | ||
: range_expression 'switch' '{' (switch_expression_arms ','?)? '}' |
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.
Something has gone wrong here:
- range_expression does not appear to be defined anywhere
- it appears the intention is to slot switch_expression between multiplicative_expression & unary_expression in the expression hierarchy, if this is the case then switch_expression needs to include unary_expression so as not to break the grammar layering
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.
range_expression is defined by the V8 PR #605, "Add Support for Indexers and Ranges," which has not yet been merged.
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.
Thanks @RexJaeschke. From #605 we have:
range_expression : unary_expression | range_expression? '..' range_expression? ;
This does connect to unary_expression but the grammar is still broken. The switch_expression grammar allows for empty braces,switch { }
, which doesn’t seem right, was the intended grammar:
switch_expression
: range_expression ( 'switch' '{' (switch_expression_arms ','? '}' )?
;
I haven't checked that all fits (see next para) but it makes a bit more logical sense and should at least fix the layering issue.
Also how is e switch { ... } switch { ... }
to be handled, if at all? I.e. using one switch to provide the value to switch on for a second (third, …) one? Something like:
switch_expression
: range_expression
| switch_expression ( 'switch' '{' (switch_expression_arms ','? '}' )?
;
might be wanted, but that is left-recursive… Requiring parentheses ((e switch { ... }) switch { ... }
is another option.
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.
Fix to my own comment of Oct 30, 2023: It doesn’t matter if the rule is left recursive per se. While we avoid it when not required, left/right recursion is the way left/right associativity is represented.
So the question here is how should switch { … } switch { … } switch { … }
associate, if it is supported?
Probably right associative surely?
Once supported/associtivity is decided the grammar will follow≥ (famous last words ;-))
(I haven’t checked what any implementation does)
: switch_expression | ||
| multiplicative_expression '*' switch_expression | ||
| multiplicative_expression '/' switch_expression | ||
| multiplicative_expression '%' switch_expression |
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.
Something has gone wrong here, see comment on line 3565 above
<!-- markdownlint-disable MD028 --> | ||
|
||
<!-- markdownlint-enable MD028 --> | ||
> *Note*: There is a grammar ambiguity between *type* and *constant_pattern* in a `relational_expression` involved `is`; either might be a valid parse of a qualified identifier. In such a case, only if it fails to bind as a type (for compatibility with previous versions of the language), is it resolved to be the first thing found (which must be either a constant or a type). This ambiguity is only present on the right-hand side of such an expression. |
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.
@@ -4438,6 +4498,8 @@ For an expression of the form `E is P`, where `E` is a relational expression of | |||
- `E` does not designate a value or does not have a type. | |||
- The pattern `P` is not applicable ([§11.2](patterns.md#112-pattern-forms)) to the type `T`. | |||
|
|||
Every *identifier* of the pattern introduces a new local variable that is definitely assigned once the corresponding *relational_expression* tests `true`. |
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.
“…assigned if the…” maybe? "Once" sounds like it is a temporal thing waiting on the relational expression to become true.
|
||
### §positional-pattern-new-clause Positional pattern | ||
|
||
A *positional_pattern* checks that the input value is not `null`, invokes an appropriate `Deconstruct` method ([§12.7](expressions.md#127-deconstruction)), and performs further pattern matching on the resulting values. It also supports a tuple-like pattern syntax (without the type being provided) when the type of the input value is the same as the type containing `Deconstruct`, or if the type of the input value is a tuple type, or if the type of the input value is `object` or `System.ITuple` and the runtime type of the expression implements `System.ITuple`. |
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 feels like it is repeating stuff which is (or should be, e.g. System.ITuple
is not in §12.7 at present) in §12.7 Deconstruction and this here should be something like:
A positional_pattern checks that the input value is not
null
, performs a deconstruction (§12.7), and performs further pattern matching on the resulting values.
; | ||
single_variable_designation | ||
: identifier | ||
; | ||
``` | ||
|
||
> *Note*: The ordering of the grammar rules in *simple_designation* is important. By putting *discard_designation” first, the source token `_` is recognized as a discard rather than as a named identifier. *end note* |
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.
We have not previously specified that the alternatives in a C# grammar rule are ordered, even though we use ANTLR as a notation. Rather we have specified a disambiguation rule, and sometimes a note saying ANTLR semantics give an implementer a free implementation of the rule; e.g. in §12.19.1 we have:
When recognising an anonymous_function_body if both the null_conditional_invocation_expression and expression alternatives are applicable then the former shall be chosen.
Note: The overlapping of, and priority between, alternatives here is solely for descriptive convenience; the grammar rules could be elaborated to remove the overlap. ANTLR, and other grammar systems, adopt the same convenience and so anonymous_function_body has the specified semantics automatically. end note
This note here should either be re-written in our current style or we should re-consider how we describe overlapping/ambiguous alternatives and re-write them all to consistent to whatever we decide.
|
||
Each pattern form defines the set of types for input values that the pattern may be applied to. A pattern `P` is *applicable to* a type `T` if `T` is among the types whose values the pattern may match. It is a compile-time error if a pattern `P` appears in a program to match a pattern input value ([§11.1](patterns.md#111-general)) of type `T` if `P` is not applicable to `T`. | ||
|
||
Each pattern form defines the set of values for which the pattern *matches* the value at runtime. | ||
|
||
With regard to the order of evaluation of operations and side effects during pattern-matching, an implementation is permitted to reorder calls to `Deconstruct`, property accesses, and invocations of methods in `System.ITuple`, and it may assume that returned values are the same from multiple calls. The implementation should not invoke functions that cannot affect the result. |
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.
Line 192 below says the order is not specified, so an implementation cannot “reorder” – it just picks an order. What the Standard needs to say is that the order of operations, and any resulting side effects, is implementation defined/specified or unspecified (which term from the collection is appropriate here).
The last sentence (“The implementation should not invoke functions that cannot affect the result”) is either redundant or should be a Note.
Prior to V8, we have a term “switch expression” defined as the expression inside parens in a
switch
statement.Now, we’re introducing a switch as an expression, and we have a new grammar rule called switch_expression. The difference between “switch expression” (without a *…* fence or separating underscore) and *switch_expression* likely is too subtle. So, how to differentiate them?
Although C calls such a switch expression a “controlling expression” (and uses that term as well for
if
,while
,for
, anddo
statements), we already state, “The governing type of aswitch
statement is established by the switch expression.”As such, I have replaced all occurrences of “switch expression” (which exist only in the statements chapter) with “switch’s governing expression.”