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

Parser refactor #3450

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

andrevidela
Copy link
Collaborator

@andrevidela andrevidela commented Dec 20, 2024

Description

Apologies, this PR blew up. Those changes aim to simplify some aspects of the parser, by moving some of its logic into the desugarer. It contains:

  • Preliminary work for Parsing Performance & Maintainability. #3369
    • By updating the surface level syntax to avoid folding pi-types at parsing-time, there is still more to do but those are the ones I could do.
    • By removing FC from PDecl and relying on fcBounds to keep track of locations, rather than thread the locations manually
    • By documenting the grammar for each sub parser
  • Preliminary work for let binders in record  #3402
    • This is actually why I started cleaning up top-level declarations
  • Fixes Invalid FC of Pi-types #3417
  • Extracts all the tests from ideMode005 into their own tests so that they can run individually and in parallel
    • I could not find any motivation for the previous architecture which required a bespoke script to find the diff between expected and actual output. This solution makes the test suite uniform
  • Updating semantic highlighting to keep track of the names that are highlighted, not just their positions.

If I had more time I would have written a shorter PR.

Merry Christmas

@andrevidela
Copy link
Collaborator Author

Alrighty I guess I'm cleaning up those ideMode tests too because currently they're a nightmare to deal with.

@andrevidela
Copy link
Collaborator Author

It turns out there was a very easy change (involving typecase) that adds a lot of information to semantic decorations, so the tests have been adapted.

@andrevidela andrevidela marked this pull request as ready for review December 21, 2024 02:30
@mattpolzin
Copy link
Collaborator

I've just been reading some of this PR to see if I can make any constructive comments. Would it be feasible to pull the WithFC changes into a different PR to the extent they don't involve new code? That seems to make up a substantial chunk of the code in this PR st first blush.

@andrevidela
Copy link
Collaborator Author

What changes in particular are you thinking of? It feels like it would be easiest to split off the changes of the test suite with the changes to the surface syntax tree.

@mattpolzin
Copy link
Collaborator

At a glance it looked like the total number of changed lines could be reduced a fair bit by moving the (mostly trivial) WithFC/MkFCVal changes to another PR that could get merged without much fuss leaving the more interesting stuff for this PR.

@andrevidela
Copy link
Collaborator Author

Alright. I'll see what I can do

@andrevidela andrevidela marked this pull request as draft January 6, 2025 20:56
@andrevidela
Copy link
Collaborator Author

On hold until #3460

* Attempt to remove stateful semantic decoration
* Remove redundant semantic coloring functions
@andrevidela andrevidela marked this pull request as ready for review January 7, 2025 18:13
@andrevidela
Copy link
Collaborator Author

Rebased and ready to review again

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.

Invalid FC of Pi-types
2 participants