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

Ensure `with module` and `with type` do not weaken module aliases (MPR#7723) #1698

Merged
merged 4 commits into from Aug 6, 2018

Conversation

Projects
None yet
4 participants
@lpw25
Copy link
Contributor

commented Apr 5, 2018

This fixes MPR#7723 by ensuring that with module .. = and with type .. = do not weaken module aliases. For example:

module M = struct type t end

module type S = sig module N = M end

module type T = S with type N.t = M.t

will now give:

module type T = sig module N = M end

instead of:

module type T = sig module N : sig type t = M.t end end
@Drup

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

The code looks alright to me, but this definitely deserves some more tests.

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

It already has a test. What additional testing would you suggest?

@Drup

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

The buggy example in your mantis bug report, for example ? The current test only covers with type, not with module.

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

I've added the test from the bug report. I'm not sure it is currently possible to trigger the bug with with module because it can't currently introduce module aliases. I expect that to change in the future, and when it does maybe we should add a test.

@Drup

Drup approved these changes Apr 6, 2018

Copy link
Contributor

left a comment

Ok, that's good enough for me.

I suppose @garrigue will want to take a look before merging. :)

@dra27 dra27 changed the title Fix MPR#7723 Ensure `with module` and `with type` do not weaken module aliases (MPR#7723) Apr 26, 2018

@damiendoligez damiendoligez added the bug label May 18, 2018

@lpw25 lpw25 force-pushed the lpw25:fix-mpr7723 branch from 6411769 to ac111fc Jul 25, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

I've rebased and added a Changes entry. @garrigue could you review?

@garrigue
Copy link
Contributor

left a comment

Seems to do what it's intended to.
This said, I would not have duplicated code for that.
Question of personal taste.

@lpw25 lpw25 merged commit 437aff0 into ocaml:trunk Aug 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.