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

Ctype.unroll_abbrev bug #1890

Merged
merged 3 commits into from Jul 10, 2018

Conversation

Projects
None yet
2 participants
@trefis
Copy link
Contributor

commented Jul 9, 2018

The call to unroll_abbrev inside Ctype.nondep_type_decl allows one to break privateness, as shown by the following example (reported by @lpw25):

        OCaml version 4.07.0+rc1

# module M : sig
    type t = private [ `Bar of 'a | `Foo ] as 'a
    val bar : t
  end = struct
    type t = [ `Bar of 'a | `Foo ] as 'a
    let bar = `Bar `Foo
  end;;
module M : sig type t = private [ `Bar of 'a | `Foo ] as 'a val bar : t end

# let y =
    match (M.bar :> [ `Bar of 'a | `Foo ] as 'a) with
    | `Bar x -> x
    | `Foo -> assert false
  ;;
val y : [ `Bar of 'a | `Foo ] as 'a = `Foo

# let y =
    match (M.bar :> [ `Bar of M.t | `Foo ]) with
    | `Bar x -> x
    | `Foo -> assert false
  ;;
Characters 16-49:
    match (M.bar :> [ `Bar of M.t | `Foo ]) with
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Type M.t is not a subtype of [ `Bar of M.t | `Foo ] 
       Type M.t = [ `Bar of M.t | `Foo ] is not a subtype of M.t 

# module F(X : sig end) : sig
    type s = private [ `Bar of 'a | `Foo ] as 'a
    
    val from : M.t -> s
    val to_  : s -> M.t
  end = struct
    type s = M.t
    
    let from x = x
    let to_ x = x
  end;;
module F :
  functor (X : sig  end) ->
    sig
      type s = private [ `Bar of 'a | `Foo ] as 'a
      val from : M.t -> s
      val to_ : s -> M.t
    end

# module N = F(struct end);;
module N :
  sig
    type s = private [ `Bar of s | `Foo ]
    val from : M.t -> s
    val to_ : s -> M.t
  end

# let y =
    match (N.from M.bar :> [ `Bar of N.s | `Foo ]) with
    | `Bar x -> N.to_ x
    | `Foo -> assert false
  ;;
val y : M.t = `Foo

On trunk, thanks to the check added in #1826, this issue is caught and one gets the following error when defining N:

>> Fatal error: nondep_supertype not included in original module type
Fatal error: exception Misc.Fatal_error

This GPR proposes to remove the call to unroll_abbrev: it's a somewhat obscure piece of code, that can cause bugs when misused, and which is used only to get nicer printing of types, as far as I can tell.

For the curious reader: unroll_abbrev was first introduced by Jérôme in 7974a9d , and was used every time one created a manifest that could be an object type, all the way up to 87b1730 when it was removed everywhere except in nondep_type_decl.

@garrigue
Copy link
Contributor

left a comment

I agree that the role of unroll_abbrev is so rare that it is simpler to just remove it.

@trefis trefis force-pushed the trefis:unroll-abbrev branch from 5b16df7 to cc7b8c0 Jul 10, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Thanks for the review!

I will merge as soon as travis confirms it's OK (apparently I had added a tab in my original commit! :o)

@trefis trefis force-pushed the trefis:unroll-abbrev branch from cc7b8c0 to e1befaf Jul 10, 2018

@trefis trefis merged commit 1f05cc8 into ocaml:trunk Jul 10, 2018

1 of 2 checks passed

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

@trefis trefis deleted the trefis:unroll-abbrev branch Jul 10, 2018

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.