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

Fix #7851 by checking type declaration coherence for recursive modules #8570

Merged
merged 2 commits into from Apr 19, 2019

Conversation

Projects
None yet
3 participants
@garrigue
Copy link
Contributor

commented Apr 2, 2019

I suppose that a really robust solution to #7851 would be to fully check the wellformedness of signatures, but in the mid-time a simpler solution is to check the coherence of type declarations for recursive modules.
This can be done by adding one line to Typedecl.check_recmod_typedecl (and also having a few related functions take a path rather than and ident).

I mark this as consider for release, but it is probably not as urgent as #8552; as explained in #7851, this is a very old bug, and triggering it is not so easy. Yet, if we could have it in 4.08 this would be one soundness bug less!

Since the change is a bit light-headed, it would be a good idea to test this on a large code base with many recursive modules, if this happens to exist.

@garrigue garrigue requested a review from lpw25 Apr 2, 2019

@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

I wondered if there was a way to circumvent this fix by using a functor, or extension constructors, but in both cases the signature is not pure anymore, so one cannot create a module from it.

@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

This also means that if one day we allow functors that do not generate code, we should be careful to update the check...

@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Just a reminder that I need a review for this to go in 4.08. @lpw25 @trefis
This is a very small change that fixes (at least partially ?) a soundness bug.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@lpw25 knows more about that particular issue than me, so I leave that one to him.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

I'm not sure this prevents every possible instance of #7851, but it handles an obvious culprit and the change is sound.

@lpw25

lpw25 approved these changes Apr 18, 2019

@garrigue garrigue force-pushed the garrigue:fix7851 branch from f1b9bea to 6f298a2 Apr 19, 2019

@garrigue garrigue merged commit 4fe08b2 into ocaml:trunk Apr 19, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

garrigue added a commit that referenced this pull request Apr 19, 2019

@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

Thanks for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.