[Parse] Do not parse generic arguments of a cast destination if the token is more than a `<` #60088

simanerush · 2022-07-18T01:57:22Z

Resolves #43053.

AnthonyLatsis · 2022-07-18T02:10:46Z

@swift-ci please smoke test macOS

AnthonyLatsis · 2022-07-18T03:04:01Z

@swift-ci please smoke test Linux

AnthonyLatsis

Looks great overall!

lib/Parse/ParseType.cpp

test/Parse/generic_disambiguation.swift

AnthonyLatsis

Thanks! One more thing.

test/Parse/generic_disambiguation.swift

AnthonyLatsis · 2022-07-19T19:45:45Z

@swift-ci please smoke test macOS

xedin

Makes sense to me! @rintaro please take a look as well!

xedin · 2022-07-26T21:21:08Z

@swift-ci please smoke test

AnthonyLatsis · 2022-07-26T22:04:34Z

Note
Reminder to squash

lib/Parse/ParseType.cpp

Co-authored-by: Luciano Almeida <[email protected]>

xedin · 2022-07-27T04:13:17Z

@swift-ci please test

rintaro

Sorry for the late reviewing 🙇

rintaro · 2022-07-27T16:49:59Z

lib/Parse/ParseType.cpp

      if (startsWithLess(Tok)) {
-        auto genericArgsStatus = parseGenericArguments(GenericArgs, LAngle, RAngle);
-        if (genericArgsStatus.isErrorOrHasCompletion())
-          return genericArgsStatus;
+        // Only attempt to parse a generic argument list in a cast destination
+        // type if the token text is just "<", because it can be an operator,
+        // for example: "1 as Int16 << 7".
+        if (Tok.getText().equals("<") ||
+            reason != ParseTypeReason::CastDestination) {


Can't we just use canParseAsGenericArgumentList() instead of startsWithLess(Tok) like expression parsing?
It only accepts single <, but I'm not sure there's a place we need to accept << for a generic arguments in the first place. If we cannot use canParseAsGenericArgumentList(), could you add some code-comment to explain why not?

I see we don't use canParseAsGenericArgumentList() probably because of the SIL type parsing which has generic function types like <T> (T) -> Void. And for non-expression position, we probably want to diagnose errors inside <...> as a generic argument error, but not as a bogus random tokens after a type. E.g.

let a: Dictionary<String: Int>

The error in <String: Int> should be diagnosed as like expected ',' instead of ':' inside parseGenericArguments(). But if we used canParseAsGenericArgumentList() here, it would be parsed as a random tokens after let a: Dictionary.

So I think having ParseTypeReason::CastDestination actually makes sense. And we can use canParseAsGenericArgumentList() only for CastDestination case.

Thank you for the feedback!

lib/Parse/ParseType.cpp

CodaFi

This embeds a form of unbounded lookahead in the Swift type grammar. We should be careful committing to additional lookahead like this to disambiguate parses. This is strictly a change to the language and should probably be discussed on evolution.

CodaFi · 2022-07-27T17:08:59Z

As discussed in #60146, I don't mind if we use unbounded lookahead to diagnose this case, but to accept this production as a result of a lookahead is a much bigger change than I think you intend to make here.

rintaro · 2022-07-27T21:49:29Z

@CodaFi

to accept this production as a result of a lookahead is a much bigger change than I think you intend to make here.

I don't think a lookahead changes the behavior for currently accepted cases. Currently at this position, all < after a type identifier is unconditionally parsed as a generic parameter clause, and that won't change even if we lookahead and decide how to parse it as a result of a lookahead as long as it was successfully parsed as a generic argument clause.

I agree this might be strictly a change to the language, but I don't think this would break any existing code.

simanerush · 2022-07-27T23:57:23Z

@CodaFi I agree with you in some sense, I was also afraid that that might be a bigger change than just a small bug fix. As we can see, this fix had already caused concern with <. But ultimately i was only trying to fix a bug. I was also thinking maybe we could rethink this and emit some helpful diagnostics like Rust does instead of the current approach.

Co-authored-by: Rintaro Ishizaki <[email protected]>

AnthonyLatsis · 2022-07-28T17:35:22Z

I tend to agree with Robert we should be more principled and farsighted in approaching disambiguation problems that require unbounded lookahead in the general case. The ambiguity can easily be worked around, and I would not want this to ever hinder a hypothetical proposal on dependent types. For now though, we can learn from Rust and improve the diagnostic.

@simanerush WDYT?

simanerush · 2022-07-28T18:41:53Z

@AnthonyLatsis I agree with you. Should I close this PR then and open an issue proposing new diagnostics rather than silencing the current diagnostics?

AnthonyLatsis · 2022-07-28T19:23:03Z

Sounds good.

AnthonyLatsis reviewed Jul 18, 2022

View reviewed changes

lib/Parse/ParseType.cpp Outdated Show resolved Hide resolved

test/Parse/generic_disambiguation.swift Outdated Show resolved Hide resolved

test/Parse/generic_disambiguation.swift Outdated Show resolved Hide resolved

AnthonyLatsis reviewed Jul 19, 2022

View reviewed changes

test/Parse/generic_disambiguation.swift Outdated Show resolved Hide resolved

simanerush force-pushed the sima-branch branch from de0e780 to ddbe845 Compare July 19, 2022 01:37

simanerush added 5 commits July 18, 2022 19:47

Fix [SR-436] Parse failure that shouldn’t be

6021739

add expected error

253ae12

Add FIXME

56681e9

Change comment, add more tests.

34c53c7

Improve readability of expected diagnostics comments

381f075

simanerush force-pushed the sima-branch branch from ddbe845 to 381f075 Compare July 19, 2022 01:54

simanerush added 2 commits July 18, 2022 21:13

Fix comment formatting & remove unnecessary check

288f957

Now actually fix comment formatting

6d3503b

AnthonyLatsis requested a review from rintaro July 19, 2022 19:25

simanerush changed the title ~~[Parse] Do not parse generic arguments of a cast destination if the token is more than a "<"~~ [Parse] Do not parse generic arguments of a cast destination if the token is more than a < Jul 20, 2022

AnthonyLatsis requested a review from xedin July 23, 2022 22:36

simanerush mentioned this pull request Jul 25, 2022

Parse failure with < after a cast destination type. #60146

Closed

xedin approved these changes Jul 26, 2022

View reviewed changes

LucianoPAlmeida reviewed Jul 27, 2022

View reviewed changes

lib/Parse/ParseType.cpp Outdated Show resolved Hide resolved

Update lib/Parse/ParseType.cpp

7274fdc

Co-authored-by: Luciano Almeida <[email protected]>

rintaro reviewed Jul 27, 2022

View reviewed changes

CodaFi requested changes Jul 27, 2022

View reviewed changes

Update lib/Parse/ParseType.cpp

80cd290

Co-authored-by: Rintaro Ishizaki <[email protected]>

simanerush closed this Jul 28, 2022

simanerush mentioned this pull request Jul 29, 2022

Emit diagnostics for parsing disambiguation #60295

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Parse] Do not parse generic arguments of a cast destination if the token is more than a `<` #60088

[Parse] Do not parse generic arguments of a cast destination if the token is more than a `<` #60088

simanerush commented Jul 18, 2022

AnthonyLatsis commented Jul 18, 2022

AnthonyLatsis commented Jul 18, 2022

AnthonyLatsis left a comment

AnthonyLatsis left a comment

AnthonyLatsis commented Jul 19, 2022

xedin left a comment

xedin commented Jul 26, 2022

AnthonyLatsis commented Jul 26, 2022

xedin commented Jul 27, 2022

rintaro left a comment

rintaro Jul 27, 2022

rintaro Jul 27, 2022 •

edited

Loading

simanerush Jul 27, 2022

CodaFi left a comment •

edited

Loading

CodaFi commented Jul 27, 2022 •

edited

Loading

rintaro commented Jul 27, 2022 •

edited

Loading

simanerush commented Jul 27, 2022

AnthonyLatsis commented Jul 28, 2022

simanerush commented Jul 28, 2022 •

edited

Loading

AnthonyLatsis commented Jul 28, 2022

[Parse] Do not parse generic arguments of a cast destination if the token is more than a < #60088

[Parse] Do not parse generic arguments of a cast destination if the token is more than a < #60088

Conversation

simanerush commented Jul 18, 2022

AnthonyLatsis commented Jul 18, 2022

AnthonyLatsis commented Jul 18, 2022

AnthonyLatsis left a comment

Choose a reason for hiding this comment

AnthonyLatsis left a comment

Choose a reason for hiding this comment

AnthonyLatsis commented Jul 19, 2022

xedin left a comment

Choose a reason for hiding this comment

xedin commented Jul 26, 2022

AnthonyLatsis commented Jul 26, 2022

xedin commented Jul 27, 2022

rintaro left a comment

Choose a reason for hiding this comment

rintaro Jul 27, 2022

Choose a reason for hiding this comment

rintaro Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

simanerush Jul 27, 2022

Choose a reason for hiding this comment

CodaFi left a comment • edited Loading

Choose a reason for hiding this comment

CodaFi commented Jul 27, 2022 • edited Loading

rintaro commented Jul 27, 2022 • edited Loading

simanerush commented Jul 27, 2022

AnthonyLatsis commented Jul 28, 2022

simanerush commented Jul 28, 2022 • edited Loading

AnthonyLatsis commented Jul 28, 2022

[Parse] Do not parse generic arguments of a cast destination if the token is more than a `<` #60088

[Parse] Do not parse generic arguments of a cast destination if the token is more than a `<` #60088

rintaro Jul 27, 2022 •

edited

Loading

CodaFi left a comment •

edited

Loading

CodaFi commented Jul 27, 2022 •

edited

Loading

rintaro commented Jul 27, 2022 •

edited

Loading

simanerush commented Jul 28, 2022 •

edited

Loading