Skip to content

Avoid re-parsing of input source files #2367

Merged
facundominguez merged 3 commits into
ucsd-progsys:developfrom
gergoerdi:T2357
Oct 10, 2024
Merged

Avoid re-parsing of input source files #2367
facundominguez merged 3 commits into
ucsd-progsys:developfrom
gergoerdi:T2357

Conversation

@gergoerdi

Copy link
Copy Markdown
Contributor

Part of the work towards #2357

@gergoerdi

Copy link
Copy Markdown
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.

Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs Outdated

@facundominguez facundominguez left a comment

Copy link
Copy Markdown
Collaborator

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?

Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs
@gergoerdi

Copy link
Copy Markdown
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
Copy Markdown
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

gergoerdi commented Oct 9, 2024

Copy link
Copy Markdown
Contributor Author

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

@facundominguez facundominguez left a comment

Copy link
Copy Markdown
Collaborator

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?

Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs Outdated
Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs
Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs Outdated
Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs Outdated
Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs Outdated
@gergoerdi

Copy link
Copy Markdown
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.

@facundominguez facundominguez left a comment

Copy link
Copy Markdown
Collaborator

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 🎉

Comment thread liquidhaskell-boot/src/Language/Haskell/Liquid/GHC/Plugin.hs
@facundominguez facundominguez merged commit bf68729 into ucsd-progsys:develop Oct 10, 2024
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