Skip to content

Fix more cases of zero_alloc / modality variables getting into signatures#2827

Merged
riaqn merged 3 commits intooxcaml:mainfrom
ccasin:destructive-subst-variables
Jul 22, 2024
Merged

Fix more cases of zero_alloc / modality variables getting into signatures#2827
riaqn merged 3 commits intooxcaml:mainfrom
ccasin:destructive-subst-variables

Conversation

@ccasin
Copy link
Copy Markdown
Collaborator

@ccasin ccasin commented Jul 19, 2024

These cases have to do with destructive substitution and module alias types. When we expand an Mty_alias into its definition, we must make the variables rigid if the new module type can be used to constrain something. To quote the tests:

module M_outer = struct
  module M_inner = struct
    type t
    let f () = () (* this has a zero_alloc variable (and a modality variable) *)
  end
end

(* In [M_outer_strong], the type of [M_inner] will be
   [Mty_alias M_outer.M_inner] *)
module type M_outer_strong =  sig
  module M_inner = M_outer.M_inner
end

(* As a result of the destructive substition, we'll eliminate the alias.  When
   this happens, we shouldn't keep the zero_alloc variables on [f] *)
module type M_outer_subst = M_outer_strong
  with type M_inner.t := M_outer.M_inner.t

(* If we had kept the zero_alloc or modality variables, this would give an
   error. *)
module M : M_outer_subst = struct
  module M_inner = struct
    let f () = ()
  end
end

We should think a little harder about other ways this might happen - in particular other uses of Typemod.extract_sig or Mtype.scrape_alias. I've taken a quick scan, and will think about it some more (as the reviewer of this PR might do) but don't want to delay getting this PR in.

This PR has three commits:

  • The first one just adds some debug printers I was tired of not having.
  • The second adds some tests that fail
  • The third fixes the bug and, correspondingly, the tests

@ccasin ccasin requested a review from riaqn July 19, 2024 20:42
@ccasin
Copy link
Copy Markdown
Collaborator Author

ccasin commented Jul 19, 2024

Request review from @riaqn

@ccasin ccasin force-pushed the destructive-subst-variables branch from c6028e8 to 0d3bfa5 Compare July 19, 2024 20:50
Copy link
Copy Markdown
Contributor

@riaqn riaqn left a comment

Choose a reason for hiding this comment

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

LGTM.

I have some further questions but I will resolve them via other channels.

@riaqn riaqn merged commit f1783a7 into oxcaml:main Jul 22, 2024
lukemaurer pushed a commit that referenced this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants