Skip to content
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

fix: add error for dot syntax in record selection #7930

Closed
wants to merge 29 commits into from

Conversation

jaschdoc
Copy link
Member

Updates the weeder with a helpful error message when using dot syntax with record selection.

Related to #7916
Related to #7897

@jaschdoc jaschdoc marked this pull request as draft June 22, 2024 10:30
@jaschdoc
Copy link
Member Author

Hmm it seems the # symbol clashes with ## for java types when used in an expression. However, we do not resolve all uses of java types against imports. For instance, a catch-rule works fine with ##java.lang... since a record selection cannot occur in that position, however, 1ii instanceof ##java.math.BigInt is a syntax error since it is parsed as record selection.
The way I see it, we have two options:

  1. We do not merge this weeder change just yet and allow parsing record.label, i.e., the dot syntax for records and simultaneously update the resolver to resolve imports everywhere a java type is used. Hopefully, feat: resolve java types against namespace imports in try-catch #7902 completely fixes this. We then merge this change.
  2. We merge this change and feat: resolve java types against namespace imports in try-catch #7902 in one big PR... I think that becomes too big, though.

In summary, the first option silently allows the old record syntax and then we fix things in the background and finally disallow it, or we do everything in one pr.

@magnus-madsen
Copy link
Member

In summary, the first option silently allows the old record syntax and then we fix things in the background and finally disallow it, or we do everything in one pr.

Lets do it incrementally.

@jaschdoc jaschdoc marked this pull request as ready for review June 24, 2024 10:16
@jaschdoc
Copy link
Member Author

@magnus-madsen compiler works now. We "just" have breaking changes in the community build.

@jaschdoc
Copy link
Member Author

jaschdoc commented Jun 24, 2024

Somehow, the IllegalRecordSelectSyntax error makes it all the way to Lowering which crashes the compiler. Where should this be handled?

EDIT: When I try this in VSCode everything works, i.e., compilation of a program with record.label fails but it does not crash the compiler. So apparently it only crashes in some tests... weird.

@magnus-madsen
Copy link
Member

Somehow, the IllegalRecordSelectSyntax error makes it all the way to Lowering which crashes the compiler. Where should this be handled?

EDIT: When I try this in VSCode everything works, i.e., compilation of a program with record.label fails but it does not crash the compiler. So apparently it only crashes in some tests... weird.

Uh. Hmm. It should not if you return a soft failure.

@magnus-madsen
Copy link
Member

Where do you see it crashing? I don't see it.

@magnus-madsen
Copy link
Member

Could you split this PR into two: one with all the fixes to the syntax and one with the actual error. (because the latter PR is kind of blocked until community builds are updated).

@magnus-madsen
Copy link
Member

What's the "status" on this?

@jaschdoc
Copy link
Member Author

This is done. Only fails because of the community build where libraries still use the dot notation for records

@jaschdoc jaschdoc deleted the syntax-dot-error branch July 29, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants