Detect and recover from missing comma in tupel

[flashspys]

This PR will make the Parser capable of detect and recover from a missing comma in tuples. The motivating example was the following:

var a: (foo: Foo bar: Bar)

 --- Before Diagnostics
 
 1 │ var a: (foo: Foo bar: Bar)
   ∣                  ╰─ unexpected text 'bar: Bar' in tuple type

 
 --- After Diagnostics
 
 1 │ var a: (foo: Foo bar: Bar)
   ∣                  ╰─ expected ',' in tuple type

The recover logic includes some interesting logic for the case var x: (foo bar). In this case we could either recover to a missing comma or a missing colon. I checked if the first part of the tuple is uppercase to decide which one is missing.

[flashspys]

In this case we have to "reparse" an already as identifier parsed token as a type. I'm not quite sure if there is a better way.

[ahoppen]

I don’t see any issues here in wrapping first in a RawSimpleTypeIdentifierSyntax but because we won’t be parsing generic types like Array<Int> correctly here. It would also be worth adding test cases for this, I think.

What I don’t understand here, is why we are wrapping first here. For (foo Bar) doesn’t this result in two tuple elements, one that only has a label and one that only has a type but no label? I think we should be forming a single tuple element, that has a name foo, missing colon and type Bar.

[flashspys]

I don’t see any issues here in wrapping first in a RawSimpleTypeIdentifierSyntax but because we won’t be parsing generic types like Array correctly here.

As we visit this code only if first was set above, and this is only the case if self.lookahead().startsParameterName(false) is true, first must be an argument label (and thus, always a RawSimpleTypeIdentifierSyntax), this code does not apply for things like Array<Int>

It would also be worth adding test cases for this, I think.

I added some test cases for generic types

What I don’t understand here, is why we are wrapping first here. For (foo Bar) doesn’t this result in two tuple elements, one that only has a label and one that only has a type but no label?

For (foo Bar) we start to look for an argument label (first), an optional second argument label (second), and a colon. If we found first (foo), but no second and no colon, we are in the case where we must decide if foo is an identifier and a colon is missing or a type and a comma is missing. In the first case we don't visited the commented code and proceed to parse Bar as a type. But if we think a comma is missing we must convert first (already parsed as an argument label) to a type. Bar now has to be parsed as a type, too, but as a separate RawTupleTypeElementSyntax, thus we cannot proceed with the current loop here, but we have to continue the loop and start the "tuple element parsing" on Bar again, resulting in two separate RawTupleTypeElementSyntax, with the first one missing a comma. To summarize: Is first uppercase, finish this RawTupleTypeElementSyntax directly, if not, proceed this RawTupleTypeElementSyntax.

I think we should be forming a single tuple element, that has a name foo, missing colon and type Bar.

This is exactly what's happening here if this code is not visited, respectively, foo is not uppercase.

[ahoppen]

As we visit this code only if first was set above, and this is only the case if self.lookahead().startsParameterName(false) is true, first must be an argument label (and thus, always a RawSimpleTypeIdentifierSyntax), this code does not apply for things like Array<Int>

Ah, I see. That makes sense. 👍

It would also be worth adding test cases for this, I think.

I added some test cases for generic types

Thank you 🙏

For (foo Bar) we start to look for an argument label (first), an optional second argument label (second), and a colon. If we found first (foo), but no second and no colon, we are in the case where we must decide if foo is an identifier and a colon is missing or a type and a comma is missing. In the first case we don't visited the commented code and proceed to parse Bar as a type. But if we think a comma is missing we must convert first (already parsed as an argument label) to a type. Bar now has to be parsed as a type, too, but as a separate RawTupleTypeElementSyntax, thus we cannot proceed with the current loop here, but we have to continue the loop and start the "tuple element parsing" on Bar again, resulting in two separate RawTupleTypeElementSyntax, with the first one missing a comma. To summarize: Is first uppercase, finish this RawTupleTypeElementSyntax directly, if not, proceed this RawTupleTypeElementSyntax.

Oh, I know what my misunderstanding was. I thought we were parsing tuples in patterns (as in let (a, b) = foo()) here, but this is for tuple types. In that case everything makes sense 👍

[ahoppen]

Thank you. I like the idea of using capitalization to disambiguate type vs identifier 👍

[ahoppen]

I don’t see any issues here in wrapping first in a RawSimpleTypeIdentifierSyntax but because we won’t be parsing generic types like Array<Int> correctly here. It would also be worth adding test cases for this, I think.

What I don’t understand here, is why we are wrapping first here. For (foo Bar) doesn’t this result in two tuple elements, one that only has a label and one that only has a type but no label? I think we should be forming a single tuple element, that has a name foo, missing colon and type Bar.

[ahoppen]

I’ve got two more minor comments inline. Also: Could you squash your commits?

[ahoppen]

Indentation seems off here.

[flashspys]

Indeed, also the indention below. I changed it.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test