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

[internal] Fix glob unsoundness #440

Merged
merged 2 commits into from
Sep 22, 2023
Merged

[internal] Fix glob unsoundness #440

merged 2 commits into from
Sep 22, 2023

Conversation

Cameron-Low
Copy link
Contributor

This fixes #102 through eager normalization of module globs at typing and instantiation time.

@Cameron-Low Cameron-Low self-assigned this Sep 14, 2023
Copy link
Contributor

@bgregoir bgregoir left a comment

Choose a reason for hiding this comment

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

Hi Cameron,
I have not read the full code but I think the reference f_norm_mod is not reasonable.
I think the good way to do it, is that in EcCoreFol, adding a module in the substitution should
have this type
f_bind_mod : f_subst -> EcIdent.t -> EcPath.mpath * (EcIdent -> form) (* memory to global formula *) -> f_subst
instead of
f_bind_mod : f_subst -> EcIdent.t -> EcPath.mpath -> f_subst
Then in EcFol, you can add a f_bind_mod that is parametrized by an env allowing to
compute the form of the global.
I have used (EcIdent -> form) but it can be any data structure allowing to build the formula once you have the memory, for example the type "use" defined in EcEnv.ml (which can be move).

But this global reference is for sur a big source of bug.

@Cameron-Low
Copy link
Contributor Author

No worries, I've fixed that following your suggestion.

@strub strub added this to the Release 2022.07 milestone Sep 21, 2023
@strub
Copy link
Member

strub commented Sep 21, 2023

@bgregoir With Cameron's fix to your comment, are you happy to go? For me, the PR is ok.

Copy link
Contributor

@bgregoir bgregoir left a comment

Choose a reason for hiding this comment

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

Thank a lot for all of this.
Have we try to check is the bug is still present?
Our hope is not but just to be sure....

@strub
Copy link
Member

strub commented Sep 21, 2023

Yes, this should be checked. But we should also check how badly it impacts projects.

@Cameron-Low
Copy link
Contributor Author

I’ve checked the examples given in the original issue and they no longer work. I haven’t done anything much broader than that though.

@fdupress
Copy link
Member

I'll run a check on SHA3.

Beyond that, I think someone will need to put some work on #399 to split off the external checks so it can be run only on PRs without losing dependencies on build jobs (so cachix has a chance to do its work instead of getting slammed with parallel builds). It unfortunately cannot be me right now.

@alleystoughton
Copy link
Member

I tried this on all my developments, and everything works. :-)

@strub strub self-assigned this Sep 22, 2023
@strub strub merged commit 50bcef7 into main Sep 22, 2023
12 checks passed
@strub strub deleted the fix-glob branch September 22, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsoundness of glob
5 participants