Skip to content

Record types and try-catch-on #2406

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

Closed
jensjoha opened this issue Aug 15, 2022 · 3 comments · Fixed by #2422
Closed

Record types and try-catch-on #2406

jensjoha opened this issue Aug 15, 2022 · 3 comments · Fixed by #2422
Labels
records Issues related to records.

Comments

@jensjoha
Copy link

jensjoha commented Aug 15, 2022

I've asked in the chat as well. Posting here to make it easier to find if needed later.

Currently

void foo() {
  try {
    ;
  } on Bar {
    ;
  }

  on(a, b, c) {
    ;
  }
  on(1, 2, 3);
}

is a try statement, then a local function declaration named on taking 3 parameters, then a call to that local function.

With record types this will instead parse as a try statement with a on specificRecordType clause, then a function call.

Any input?

/cc @lrhn @munificent @eernstg

@jensjoha
Copy link
Author

On and probably /cc @bwilkerson

@lrhn
Copy link
Member

lrhn commented Aug 15, 2022

The general problem is that built-in-identifier type used to be unambiguous, and that our contextual keywords were only allowed in positions where they were unambiguous (just before an identifier).
With two adjacent identifiers separated by whitespace, the first one had to be either a type or a modifier, and since modifiers were built-in identifiers or contextual keywords (or reserved), they couldn't be the name of a type.

With records, the type can now start with a non-identifier, so "built-in-identifier type" doesn't have to be two adjacent identifiers, and same for a contextual keyword followed by a type, which means it becomes ambiguous with many more grammar productions.

At least, a lot of our keywords can only occur as keywords outside of function bodies, and are either required by the context or clearly distinguishable from alternatives (not occurring where any expression can also occur). So, this may be the only actual ambiguity. (But we should check.)

I'll (presently) suggest that we make on after try { ... } always mean the on keyword.
If you really, really need to call a function named on right after a try block, you'll have to add parentheses.
In practice that'll likely never happen. I found one instance of a method actually called on, and it's an instance method, so you'd only do on(args) inside a subclass. That doesn't happen after a }.

So, on being a contextual keyword would actually be enforced by the context, not just allowed. Should make parsing easier: Is parsing try, looking for on/catch/finally. If you find on, you don't even need to look ahead (to check that the next token is an identifier), you can just start parsing a type immediately.

@lrhn lrhn added the patterns Issues related to pattern matching. label Aug 15, 2022
@munificent
Copy link
Member

munificent commented Aug 18, 2022

I'll (presently) suggest that we make on after try { ... } always mean the on keyword.

+1. I'll update the proposal to make that explicit.

cc @leafpetersen @eernstg @jakemac53 @natebosch @stereotype441 @kallentu

@munificent munificent added records Issues related to records. and removed patterns Issues related to pattern matching. labels Aug 18, 2022
munificent added a commit that referenced this issue Aug 19, 2022
- Support constant records. Fix #2337.
- Support empty and one-positional-field records. Fix #2386.
- Re-add support for positional field getters Fix #2388.
- Specify the behavior of `toString()`. Fix #2389.
- Disambiguate record types in `on` clauses. Fix #2406.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
records Issues related to records.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants