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

PR#7787: fix module type of and recursive modules interaction #1743

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
3 participants
@trefis
Copy link
Contributor

trefis commented Apr 27, 2018

This completes the revert of 65b1193 which was started in GPR #1652 .

For the record, the diff in the lambda code is the following:

  (let
    (O/1002 =
       (function T/1054 is_a_functor
         (let
           (go/1006 =
              (function param/1008 (apply (field 0 (field 0 T/1054)) 42)))
           (makeblock 0 go/1006))))
    (seq (setfield_ptr(root-init) 0 (global PR7787!) O/1002)
      (let (foo/1011 = (function x/1012 (+ x/1012 3)))
        (setfield_ptr(root-init) 5 (global PR7787!) foo/1011))
      0a
      (let (N/1010 = (makeblock 0 (field 5 (global PR7787!))))
        (seq (setfield_ptr(root-init) 4 (global PR7787!) N/1010) 0a))
      (let (T/1009 = (makeblock 0 (field 4 (global PR7787!))))
        (seq (setfield_ptr(root-init) 1 (global PR7787!) T/1009)
          (let
            (M/1013 =
               (apply (field 0 (global CamlinternalMod!))
                 [0: "./PR7787.ml" 22 6] [0: [0: 0a]])
-|           T2/1014 =
-|             (apply (field 0 (global CamlinternalMod!))
-|               [0: "./PR7787.ml" 25 6] [0: [0: [1: 0a]]]))
+|           T2/1014 = (makeblock 0 (field 0 (field 1 (global PR7787!)))))
            (seq
              (apply (field 1 (global CamlinternalMod!)) [0: [0: 0a]] M/1013
                (apply (field 0 (global PR7787!)) T2/1014))
-|            (apply (field 1 (global CamlinternalMod!)) [0: [0: [1: 0a]]]
-|              T2/1014 (makeblock 0 (field 0 (field 1 (global PR7787!)))))
              (setfield_ptr(root-init) 2 (global PR7787!) M/1013)
              (setfield_ptr(root-init) 3 (global PR7787!) T2/1014)
              (apply (field 32 (global Stdlib!))
                (apply (field 0 (field 2 (global PR7787!))) 0a))
              (apply (field 35 (global Stdlib!)) 0a) 0a 0a))))))

Should I create a fresh changelog entry or just add the MPR and GPR number to the entry of #1652 ?
Also, I think this should be backported on 4.07.

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Apr 27, 2018

Note: I added the test in typing-modules because that's where I found the tests of MPR#6307, but it might not actually be the best place for this test.
Any suggestion?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 27, 2018

I think that typing-modules is fine. You can add the MPR and GPR numbers to the existing entry, but also add yourself in the credit line (Leo White and Thomas Refis, ...).

@lpw25

This comment has been minimized.

Copy link
Contributor

lpw25 commented Apr 28, 2018

If you take the failing example, and write out the module signature by hand -- rather than using module type of -- then it doesn't fail. The key difference between these two cases should be that in one the alias is marked Mta_absent and in the other it is marked Mta_present -- so the init_shape function is compiling a Mta_present alias as if it were an Mta_absent alias.

This patch should probably raise not found only for the Mta_present case and then use the old behaviour for the Mta_absent case.

However, this bug has reminded me of another issue that I intended to implement with the [@remove_alias] patch but completely forgot. Really module type of should turn Mta_present aliases into Mta_absent aliases, so that the user cannot observe the difference. I'll make a PR for that on Monday.

@trefis trefis force-pushed the trefis:pr7787 branch from 19ac8ce to 383a70d Apr 30, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Apr 30, 2018

I updated the PR as per @lpw25's comment.

@trefis trefis force-pushed the trefis:pr7787 branch from 383a70d to 47592ef Apr 30, 2018

@lpw25

lpw25 approved these changes Apr 30, 2018

PR#7787: fix module type of and recursive modules interaction
Mta_absent and Mta_present aliases should be compiled differently.

@trefis trefis force-pushed the trefis:pr7787 branch from 47592ef to 1eabf70 Apr 30, 2018

@trefis trefis merged commit 9ba6f3b into ocaml:trunk Apr 30, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Apr 30, 2018

I am going to cherry-pick this to 4.07 tomorrow morning.

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented May 1, 2018

This is now on the 4.07 branch.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.