Describe the impact of ? on arrays of arrays

[jskeet]

Alternative to #1386.

Fixes #1385.

[jskeet]

I'm fully expecting there to be some more work here, but I'm hoping it's at least close.

[jskeet]

@Nigel-Ecma: I believe the grammar test failure is now a matter of changing the expected tokens (due to the introduction of non_array_non_nullable_type) - let me know if this is something I can fix myself, or whether it needs your help :)

[BillWagner]

I like this approach. I have a couple comments to consider and discuss.

[Nigel-Ecma]

I’ve made a few comments but obviously there is a fair bit to go yet – e.g. conversions. Some of it is still confusing, but it seems a good start to dressing this up ;-)

[Nigel-Ecma]

@Nigel-Ecma: I believe the grammar test failure is now a matter of changing the expected tokens (due to the introduction of non_array_non_nullable_type) - let me know if this is something I can fix myself, or whether it needs your help :)

With the proviso that I haven’t downloaded this and run the failing tests it does look like the errors reported are valid changes in the expect parse due to the revised grammar.

If this is the case then it will just need a few updates to the “reference” parse files (the ones that are machine generated and ignored in PR reviews). You can generate them yourself but you might prefer one of us with the tools already downloaded to run and produce them does it! It can wait until the PR is approved and before it is merged.

[jskeet]

Next steps:

• Nigel to email Jon the files to add to this PR to get the grammar checker passing
• Jon will update this PR, then merge.
• Jon to file a new issue around mapping between nullable and non-nullable arrays, see Allow arrays of nullable reference types #1386, to also include conversions - and anything else folks think is missing
• Jon will hopefully create a new PR to address that

[BillWagner]

Let's based on our discussion in the meeting.

Per meeting discussion

[jnm2]

I love this approach!

[Nigel-Ecma]

The existing grammar supported T?[] iff T was a value type, this PR intends to extend it to all T. The existing grammar supported this as value_type covers both nullable & non-nullable value types – the only nullable types that existed at the time. The revised grammar here provides two ways that non_array_type can parse V? as shown below – text in brackets, e.g. [?], following a rule indicates the input token(s) it is matching:

non_array_type ➜ non_array_non_nullable_type nullable_type_annotation? [`?`]
    non_array_non_nullable_type ➜ value_type
                                ➜ non_nullable_value_type  [`V`]

or

non_array_type ➜ non_array_non_nullable_type nullable_type_annotation?
    non_array_non_nullable_type ➜ value_type
                                ➜ nullable_value_type
                                ➜ non_nullable_value_type [`V`] nullable_type_annotation [`?`]

As there are two parses the new grammar is ambiguous, but I suspect ambiguity was not intended and this needs to be fixed in the grammar and not with a prose disambiguation rule.

Changing line 67 to reference non_nullable_value_type would seem to fixed the immediate issue, but I have not worked that through the PR’s total grammar changes myself.

[jskeet]

That didn't take me as long to understand as I feared - and thank you for the suggestion, which I've applied. Fortunately the name of the rule is pretty clear in terms of what it's trying to convey, too.

@BillWagner please could you check that you're happy with the change on this line as well? (value_type => non_nullable_value_type). I suspect we'll still need Nigel to run the grammar checker etc, but hopefully this will have cleared up the ambiguity.

[jskeet]

This is still waiting for another PR from @Nigel-Ecma that will affect the grammar, then we'll come back to it.

[jskeet]

All dependent PRs are now merged, so @Nigel-Ecma should be able to regenerate the necessary files for the GitHub checks to pass.

[jskeet]

Tests have now passed - merging.