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

Evaluate <> and provide type resolution in lalrpop. #17

Merged
merged 14 commits into from
Nov 21, 2020
Merged

Evaluate <> and provide type resolution in lalrpop. #17

merged 14 commits into from
Nov 21, 2020

Conversation

dnbln
Copy link
Contributor

@dnbln dnbln commented Nov 15, 2020

Will need to cache the resolved lalrpop types (as of now resolves everything and that can get quite expensive).

Edit: Sorry for the quite big PR. This one has plenty of changes, related to <> expressions and type resolution. I only cached the types of nonterminals where there are no "parameters"; also if you think the variables / functions can have better names I would be more than happy to change them.

With that, this PR is more or less ready for review.

@dnbln dnbln marked this pull request as ready for review November 17, 2020 18:23
@AzureMarker
Copy link
Owner

Thanks for working on this! I'll get to work on the review soon.

@dnbln
Copy link
Contributor Author

dnbln commented Nov 18, 2020

Found a weird corner-case in the grammar while testing:

Given this:

X: () = {
}

Y: () = {
}

F = {
    <a: X> Y <Y> => f(<>)
}

The alternative in F has 2 symbols: <a: X> and Y<Y>. Just putting it here because I thought it was weird.

Copy link
Owner

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good! I'm excited to see no errors in my grammar files and good type inference. Just a few small things here and there, but congrats on getting this to work!

@AzureMarker
Copy link
Owner

AzureMarker commented Nov 20, 2020

RE: the odd grammar issue, yeah that's odd. I wonder how LALRPOP handles that itself. After diving into the code, it looks like the lexer emits either a MacroId or a normal Id depending on if there is an < immediately following an identifier (with no space in-between):
https://github.com/lalrpop/lalrpop/blob/cba3a7463c014b8444623848219532628054e9a1/lalrpop/src/tok/mod.rs#L647

Edit: created #19

@dnbln dnbln requested a review from AzureMarker November 21, 2020 17:24
Copy link
Owner

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks again for working on this.

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.

2 participants