Skip to content

Conversation

@gafter
Copy link
Member

@gafter gafter commented Mar 19, 2025

Proposed replacement for #1287

@Nigel-Ecma Can you please help me set up the testing for this PR?

@gafter gafter marked this pull request as draft March 19, 2025 22:28
@gafter gafter requested a review from Nigel-Ecma March 19, 2025 22:58
@gafter gafter added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Mar 19, 2025
array_type
: non_array_type rank_specifier+
| non_array_type rank_specifier+
| non_array_type ( nullable_type_annotation rank_specifier+ )+
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this allow int??[]?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because a nullable type isn't a non_array_type.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading it right, non_array_type includes value_type which includes nullable_value_type which I believe int? would be.

Copy link
Member

Choose a reason for hiding this comment

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

I'd agree with @gafter. nullable_value_type is defined as non_nullable_value_type nullable_type_annotation. There's no additional condition for a nullable_value_type. So, int?? isn't a valid type.

Copy link
Contributor

@jnm2 jnm2 Nov 18, 2025

Choose a reason for hiding this comment

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

non_array_type may expand into value_type, which may expand into nullable_value_type, which expands into non_nullable_value_type nullable_type_annotation. Therefore, non_array_type may expand into non_nullable_value_type nullable_type_annotation. Substituting that:

non_array_type ( nullable_type_annotation rank_specifier+ )+

may be

non_nullable_value_type nullable_type_annotation ( nullable_type_annotation rank_specifier+ )+

Notice how nullable_type_annotation appears twice. The proposed grammar allows int??[] as a valid construction.

@gafter gafter marked this pull request as ready for review March 20, 2025 20:32
Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

See TG2 email discussion for details.

@Nigel-Ecma Nigel-Ecma marked this pull request as draft April 14, 2025 05:58
@jskeet
Copy link
Contributor

jskeet commented Jun 11, 2025

We want to wait for a meeting where @gafter is present to discuss this as well, but @jnm2 is keen to get his hands dirty with it when he finds time.

Copy link
Contributor

@jnm2 jnm2 left a comment

Choose a reason for hiding this comment

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

We want to wait for a meeting where @gafter is present to discuss this as well, but @jnm2 is keen to get his hands dirty with it when he finds time.

I made it happen! See #1386 for my proposed grammar changes. If we take my PR as the direction, @gafter's other changes in this PR will be valuable to move over.

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.

Once grammar validation is added, I'm ready to accept this.

array_type
: non_array_type rank_specifier+
| non_array_type rank_specifier+
| non_array_type ( nullable_type_annotation rank_specifier+ )+
Copy link
Member

Choose a reason for hiding this comment

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

I'd agree with @gafter. nullable_value_type is defined as non_nullable_value_type nullable_type_annotation. There's no additional condition for a nullable_value_type. So, int?? isn't a valid type.

@jskeet
Copy link
Contributor

jskeet commented Nov 19, 2025

Closing in favor of #1386 - @gafter if you feel strongly that we should come back to this instead, please reopen and we can discuss.

@jskeet jskeet closed this Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

meeting: discuss This issue should be discussed at the next TC49-TG2 meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants