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

Remove late modifier for Dart generated context classes #3286

Merged
merged 3 commits into from
Dec 30, 2021

Conversation

idraper
Copy link
Contributor

@idraper idraper commented Sep 26, 2021

When antlr generates code using the Dart runtime, it creates context classes that use the late modifier on labeled grammar elements. For example:

term : EOF_=EOF?;

would generate a term context class where EOF_ would be declared as:

late Token? EOF_;

However, in the above example, there is the possibility that EOF_ would not be set. This is an error in Dart at runtime when trying to access the EOF_ member since the late keyword means Dart expects the value to be assigned before use. The late modifier should be removed since the type is already nullable and thus is not required to be assigned (see here for details about null safety with late).

Thus, the runtime should just generate:

Token? EOF_;

@lingyv-li
Copy link
Member

Thanks for your contribution. Can you please provide a reproducible example of when this would fail?

@renancaraujo @canastro can you please take a look at this PR? Thanks

@canastro
Copy link
Contributor

The logic seems sound and the code seems good to go. But, it would be nice to have someone from the core to confirm that EOF_ can actually be not set and its not just some other bug creeping in. I don't have enough knowledge on antlr to confirm or not.

@parrt
Copy link
Member

parrt commented Dec 28, 2021

@lingyv-li if you think it looks correct and useful, we can merge.

@lingyv-li
Copy link
Member

@parrt Yes this LGTM

@parrt parrt added this to the 4.9.4 milestone Dec 30, 2021
@parrt parrt merged commit 5d72d80 into antlr:master Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants