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

Avoid re-parsing of input source files #2367

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

gergoerdi
Copy link
Contributor

Part of the work towards #2357

@gergoerdi
Copy link
Contributor Author

Yes, I'm using a global IORef.

If we want to avoid that, we need to find a place to store an arbitrary value (in this case, a ParsedModule) and have it make its way all the way to the post-typechecking hook. I was hoping we could use a module annotation (analogously to how you can put already resolved names into a RdrName) , but it looks like those only contain text and an expression.

Copy link
Collaborator

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

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

Thanks @gergoerdi.

I left a couple of comments for you to consider.

Does this PR solve your use case? If not, what else would be necessary?

@gergoerdi
Copy link
Contributor Author

Does this PR solve your use case? If not, what else would be necessary?

Not fully yet, no. I'll add details to #235, but basically there are two other points where source files are read in by LH. One of them is easily disabled by just setting noannotations = True in the config. The other one is some random place for caching I guess? I'll look it up.

@gergoerdi
Copy link
Contributor Author

Note that due to the breadcrumb stuff, this PR now includes #2368 as well. I think #2368 has less potential for disruption, and it fixes a manifest bug, so I think it will go in before this anyway.

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Oct 9, 2024

FWIW, I 100% disagree with the HLint suggestion here:

warning Use tuple-section
Suggestion in swapBreadcrumb in module Language.Haskell.Liquid.GHC.Plugin:

Use tuple-section

  • Found: \ old -> (old, new)
  • Perhaps: (, new)
  • Note: may require {-# LANGUAGE TupleSections #-} adding to the top of the file

Copy link
Collaborator

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

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

FWIW, I 100% disagree with the HLint suggestion here:

Want to try disabling that one either globally or only in Plugin.hs?

@gergoerdi
Copy link
Contributor Author

In fact, in review I don't like my current code either :)

So what we really want is:

  1. First time we parse a module, there is no breadcrumb for the current module so we can store a Parsed breadcrumb. Later on, if we get to the after-parser hook again and do find some existing breadcrumb, that is an assertion failure.

  2. First time we typecheck a module, there is a Parsed breadcrumb for the current module. We replace it with a Typechecking breadcrumb, which also makes sure we don't retain all ASTs. Getting to typechecking without a breadcrumb is an assertion failure.

  3. Recrusive typecheckers will see that there is already a Typechecking breadcrumb and do nothing.

I'm not sure we should ever remove the Typechecking breadcrumb because then we have no way of later detecting some of the invalid states above. Maybe replace it with a Done breadcrumb that is an error to encounter in both hooks.

I'll redo the implementation based on these ideas.

Copy link
Collaborator

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

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

Can't find anything else to complain about 🎉

@facundominguez facundominguez merged commit bf68729 into ucsd-progsys:develop Oct 10, 2024
4 checks passed
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