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

Feature: An alternative approach to handling "null conditional" ops #251

Merged
merged 23 commits into from
Oct 20, 2021
Merged

Feature: An alternative approach to handling "null conditional" ops #251

merged 23 commits into from
Oct 20, 2021

Conversation

Nigel-Ecma
Copy link
Contributor

@Nigel-Ecma Nigel-Ecma commented Apr 5, 2021

Following on from the discussion on #7 at the last meeting this is a draft PR adding the null conditional operations into the grammar at the same level as their respective unconditional siblings.

Some of this is guesswork/invention...

On the invention side in particular note that three terminals: ?., ?( and ?[; have been added to Operator_Or_Punctuator.

…ops, adding

them into the grammar at the same level as their respective unconditional siblings.
@dnfadmin
Copy link

dnfadmin commented Apr 5, 2021

CLA assistant check
All CLA requirements met.

@Nigel-Ecma Nigel-Ecma added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Apr 5, 2021
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely prefer this approach over the one in #7 - thanks very much for the work here, Nigel.

As you can tell, I'm slightly confused about how null conditional invocation expressions work - see comments for details.

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/lexical-structure.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
@jskeet
Copy link
Contributor

jskeet commented Apr 7, 2021

Nigel to have a look at whether this PR's approach makes sense, after we had more discussion about why the grammar in #7 was expressed that way.

@Nigel-Ecma
Copy link
Contributor Author

Nigel to have a look at whether this PR's approach makes sense, after we had more discussion about why the grammar in #7 was expressed that way.

Before I look into this properly can I just confirm my understanding of our meeting conversation:

Note: In the second bullet I don't say the "same" as #251 might be described as "null propagating" where a null-conditional operator supplies a null to another null-conditional operator, etc., which by-description results in multiple "is this null" tests but any compiler would likely short circuit. So the semantics might not the same but be equivalent – just a different way of describing the same result.

If that summary is either correct, or once it is corrected, I'll look into whether the approach in #7 is required and if not which of #7 & #251 might be more desirable.

Remove null_conditonal_invocation_expression as it is not implemented.
Remove '?(' - null_conditional_invocation_expression - as it is not implemented
Remove null_conditional_invocation_expression as it is not implemented.
@Nigel-Ecma Nigel-Ecma closed this Apr 29, 2021
@Nigel-Ecma Nigel-Ecma reopened this May 3, 2021
…_element_access to include trailing

unconditional_access_part(s) which extend the access to include any immediately following member_access
and/or element_access operations. This change alters the semantics so that a null LHS of either operation
results in a null being the result of the operation and all immediately following unconditional accesses.

Only the grammar required to be changed, the description of the semantics stays the same.

Note: This could maybe be defined to include null_conditional access in unconditional_access_part but
the current choice was chosen for simplicity – which is of course subjective!
@Nigel-Ecma
Copy link
Contributor Author

Nigel-Ecma commented May 3, 2021

The grammar has been extended so that null-conditional access operations "capture" any immediately following unconditional access operations. This should match the desired semantics, keep the description of these operators in the same part of the grammar as their unconditional siblings. Changes to statement expressions are not included as they feel like a wart that may have a better solution, but they could follow the same as #7 if there is no solution found. @MadsTorgersen is this the intended semantics?

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not attempting to assess whether this is equivalent to #7 or not. I still prefer this approach to organizing the feature over the approach in #7 if we can satisfy ourselves that it's equivalent.

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
- "null conditional invocations" (a subset of the existing null conditional
expressions) which can be used as statement expressions or lambda bodies
- "null conditional projection initializers" (another subset) supporting
null conditional member access in anonymous object creation
- some other tweaks
@Nigel-Ecma Nigel-Ecma changed the title Feature: First pass at an alternative approach to handling "null conditional" ops Feature: Getting closer (or there?) to an alternative approach to handling "null conditional" ops May 31, 2021
@Nigel-Ecma
Copy link
Contributor Author

A summary of the key differences & changes compared to the original MS:

  • The null conditional operators were added to unary_expression rather than alongside their non-conditional siblings in primary_expression in the MS approach. Here they are added to primary_expression.
  • The MS approach captured all member/element accesses including conditional ones which follow an initial null conditional operation. Here only non-conditional ones are captured, this simplifies the description of the semantics. The overall semantics remains the same.
    • As part of the simplification a null_conditional_invocation_expression is introduced which syntactically is a struct subset of null_conditional_member/element_access – those that end in an invocation.
    • The null_conditional_member/element_access descriptions have the semantic restriction that the expressions must not be of type void/classified as nothing and reference null_conditional_invocation_expression for those that are void. This separates out the semantics for value producing expressions, defined as having the same meaning as a if/then/else, and non-value producing ones, defined using an if/then, into different clauses rather than a longer set of nested bullets. (The meanings follow the same code as in previous approach.)
    • Note: In doing this PR I noticed that in the Standard there is sometimes reference to use of something in a "statement expression" and in others "statement expression or lambda body". I wonder whether some/all of the former should be the latter... I haven't checked, but I have included null_conditional_invocation_expression in anonymous_method_body.
  • Also added a null_conditional_projection_initializer, a subset of *null-conditional-member-access", which does not include capturing adjacent accesses/invocations. As the name suggests it is a projection initializer for anonymous objects.

To test the grammar is parsing as expected I use a simple test file, nullConditional.cs. (Note: the grammar does take advantage of ANTLR's top-to-bottom rule ordering so the above mentioned subset productions get picked over the full set productions.) This file along with the graphic output of TestRig/grun as nullConditional.png is attached as a ZIP (sorry, Github barfed on the PNG and suggested I ZIP it). (Note: the grammar fed to ANTLR has undergone mutual left recursion removal in primary_expression so you might spot "missing" nodes in the PNG due to this.)

This is hopefully #251 pretty much done, please comment & compare with #7.

nullConditional.zip

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more thoughts, but I definitely like this approach. Will discuss later.

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
: primary_expression '?' '.' Identifier type_argument_list? captured_access*
;

captured_access
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "captured access" a new concept here? If so, might we want to consider naming it something else to avoid it being confused with variable capture within anonymous functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new concept, the language doesn't really have any other operators which behave quite in the way the null -conditional ones do – they "capture" following operations (#7 captures more than #251, it is one of the differences, but that isn't fundamental per se and both have to find a way to describe the behaviour). After some iterations I came up with "captured access", I didn't consider the comparison with "variable capture"...

"[null] conditional access" didn't seem right as the accesses themselves are not conditional [and they will fault on null], they are just the subject of another conditional – as the explanation of the semantics in terms of an equivalent conditional (?/':orif`) shows. Captured, Controlled, Effected, Impacted, Subject? I just noticed I went with the first alphabetically (pure happenstance) ;-)

Another case where wordsmithing input is welcome!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nested_access ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they’re nested – if anything isn't it the enclosed/first which is impacting the enclosing/following so nested sounds sort of backwards to me? So while captured isn’t great I think its a smidgen better. But is there something better? Words are hard, give me numbers any day ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to be clear, a captured/nested access is one that's gated by a null conditional member access - i.e. it looks unconditional in itself, but it's conditional based on the earlier null conditional member access, yes?

"Implicitly conditional access"? "Effectively conditional access"? I'm not liking either of those, but if we can agree on what impression we're trying to give, that'll be a start :)

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
@jskeet jskeet self-requested a review June 2, 2021 21:44
@jskeet
Copy link
Contributor

jskeet commented Jul 1, 2021

Assigned to @MadsTorgersen as his "second item" to focus on.

gafter
gafter previously requested changes Jul 28, 2021
: primary_expression '?' '.' Identifier type_argument_list? captured_access*
;

captured_access
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nested_access ?

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Show resolved Hide resolved
…to language etc.

- *null_conditional_invocation_expression* was omitted as an alternative in *method_body* (thanks @gafter) - fixed
- the null conditional operations had not been added to the precedence table - fixed
- added text specifying how overlapping alternatives must be resolved – take the first; and informative note explaining
why the overlapping exists - descriptive simplicity, the rules could be elaborated to remove the overlap, and ANTLR
uses the same conveneince to keep grammars shorter/easier to read
@Nigel-Ecma
Copy link
Contributor Author

Nigel-Ecma commented Sep 14, 2021 via email

@Nigel-Ecma Nigel-Ecma changed the title Feature: Getting closer (or there?) to an alternative approach to handling "null conditional" ops Feature: An alternative approach to handling "null conditional" ops Sep 14, 2021
@jskeet
Copy link
Contributor

jskeet commented Sep 14, 2021

“Captured” works I think… partly as it doesn’t try to give the detail and leaves the reader to figure it out from the grammar and prose :-)

I'm still uncomfortable with captured given that it has a very specific (and entirely different) meaning elsewhere. I think this we should discuss in the meeting.

@jskeet
Copy link
Contributor

jskeet commented Sep 22, 2021

In current meeting: "dependent" has the most backing. (Somewhat reluctantly from all, but better than alternatives.)

@Nigel-Ecma Nigel-Ecma added this to the C# 6 milestone Sep 27, 2021
@Nigel-Ecma Nigel-Ecma marked this pull request as ready for review September 27, 2021 22:45
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @Nigel-Ecma

I think I found 2 small nits that I commented on. Once we resolve those, I think we should merge this, in favor of #7

standard/classes.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
Typos caught by @BillWagner, two rather significant!

Co-authored-by: Bill Wagner <[email protected]>
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about the difference between null_conditional_invocation_expression and null_conditional_member_access, as noted in comments. I suspect this is a problem of me misunderstanding rather than in the proposal, but I'd like to just check it through during our meeting.

standard/classes.md Show resolved Hide resolved
@@ -1283,6 +1285,50 @@ In a member access of the form `E.I`, if `E` is a single identifier, and if the
> ```
> Within the `A` class, those occurrences of the Color identifier that reference the Color type are delimited by `**`, and those that reference the Color field are not. *end example*

### §null-conditional-member-access Null Conditional Member Access

A *null_conditional_member_access* is a conditional version of *member_access* ([§12.7.5](expressions.md#1275-member-access)) and it is a binding time error if the result type is `void`. For a null conditional expression where the result type may be `void` see (§null-conditional-invocation-expression).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: double space in "and it"

: primary_expression '?' '.' Identifier type_argument_list? dependent_access*
;

dependent_access
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to check, primary_expression will cover nested null-conditional member accesses, yes? So if we have:

x?.M1()?.M2().M3

that ends up with a "top-level" null_conditional_member_access where the primary_expression is x?.M1()?.M2(), which is itself a null_conditional_member_access with a primary_expression of x?.M1(), which is itself a null_conditional_member_access with a primary expression of x... is that right?

@@ -1283,6 +1285,50 @@ In a member access of the form `E.I`, if `E` is a single identifier, and if the
> ```
> Within the `A` class, those occurrences of the Color identifier that reference the Color type are delimited by `**`, and those that reference the Color field are not. *end example*

### §null-conditional-member-access Null Conditional Member Access
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just been trying to figure out how this works in a couple of cases, and while I think it's the right approach, we may need a bit more explanation.

For example consider:

string x = null;
int? y = x?.GetHashCode();

My understanding is that that is a null_conditional_member_access with a dependent_access of () - so we get the null value. It's not a null_conditional_invocation_expression, because that's only used in places where the result (if any) is discarded. Is that the case? If so, it's a bit confusing because x.GetHashCode() is an invocation expresson, isn't it?

I'm not suggesting that we change any of the existing text - just consider adding an example/note.

| block
;
```

When recognising an *anonymous_function_body* if both the *null_conditional_invocation_expression* and *expression* alternatives are applicable then the former shall be chosen.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this one. To go back to my previous example, consider:

Func<string, int?> func = x => x?.GetHashCode();

Does that satisfy the requirements of this paragraph? We'd still want to use an expression with a null_conditional_member_access rather than a null_conditional_invocation_expression in order to avoid "losing" the return value, wouldn't we? I may have missed something here.

@BillWagner
Copy link
Member

@jskeet

I'm a bit confused about the difference between null_conditional_invocation_expression and null_conditional_member_access, as noted in comments. I suspect this is a problem of me misunderstanding rather than in the proposal, but I'd like to just check it through during our meeting.

I thought the distinction was meant to handle the case where an invocation expression had a void result. That makes the discussion of changing the result type to a nullable unnecessary. I too, could be misunderstanding this distinction.

@jskeet
Copy link
Contributor

jskeet commented Oct 18, 2021

@BillWagner:

I thought the distinction was meant to handle the case where an invocation expression had a void result. That makes the discussion of changing the result type to a nullable unnecessary. I too, could be misunderstanding this distinction.

I agree with the motivation - but then the part about choosing null_conditional_invocation_expression over expression confuses me.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, Nigel. I think I now understand and it's all good :)

@jskeet jskeet dismissed gafter’s stale review October 20, 2021 20:16

We believe the change has now been made (or a slightly different version)

@jskeet jskeet merged commit dd90903 into dotnet:draft-v6 Oct 20, 2021
@jskeet jskeet mentioned this pull request Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants