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

Apply substitution to all modules when packing #2175

Merged
merged 2 commits into from Dec 1, 2018

Conversation

Projects
None yet
2 participants
@lpw25
Copy link
Contributor

commented Nov 30, 2018

When creating the resulting module type from -pack the substitution to rewrite internal names to external names was only being applied in link order. Most of the time that works, but interface dependencies do not always follow the same ordering as implementation dependencies so really the substitution needs to be applied to all the packed modules. In particular, before this PR it was easy to accidentally create a pack containing an mli-only module which still contained references to the internal name of the module -- resulting in vary confusing type errors.

@gasche

gasche approved these changes Nov 30, 2018

Copy link
Member

left a comment

I see what the change is doing, but I find it somewhat perverse to fix a fairly tricky control-flow issue by some fairly tricky new control-flow implementation, without leaving a comment on the fact that the new behavior is intentional.

The behavior could be made clearer by first doing a map over the units, adding oldid and newid to the (name, sg) tuple, then a fold to collect a substitution, and finally a new map to produce the packed signature. (I guess that no comment would be necessary then, as the intent would be clear from the code structure.)

@lpw25 lpw25 force-pushed the lpw25:pack-subst-all branch from cb242b7 to f0cd886 Dec 1, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

I've changed the function to make it clearer, and added a Changes entry.

@gasche gasche merged commit 3d288ae into ocaml:trunk Dec 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

See MPR#7932 as a regression caused by this change -- possibly for good reasons.

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.