Skip to content

Remove primitive coercions#1566

Closed
lpw25 wants to merge 2 commits intoocaml:trunkfrom
lpw25:no-primitive-coercions
Closed

Remove primitive coercions#1566
lpw25 wants to merge 2 commits intoocaml:trunkfrom
lpw25:no-primitive-coercions

Conversation

@lpw25
Copy link
Copy Markdown
Contributor

@lpw25 lpw25 commented Jan 12, 2018

Currently external components of a module do not appear in the runtime representation of the module. This means that when coercing an external to a val, as in:

module M : sig
  external foo : int -> int = "foo"
end = struct
  external foo : int -> int = "foo"
end

module N : sig val foo : int -> int end = M

we must insert an eta-expansion of the external into the module. It also means that all references to an external that are not direct applications cause a fresh eta-expansion of the external to be generated.

This PR implements a different scheme. We treat external components much like val components -- they are ordinary fields of the module at runtime containing an eta-expansion of the external. This means that module coercions do not need to consider external components. It also means that most uses of the external that are not direct applications can just reference the field of the module.

Note that direct applications of externals will still be compiled to direct applications of the corresponding primitive. Similarly, uses of builtin externals which can take advantage of type-based specialization will still generate a fresh eta-expansion so that they use the most efficient version of the primitive available.

The advantages of this new approach are:

  • It simplifies the handling of module coercions which are awkward and will make adding some future module features noticeably more difficult.
  • It avoids generating many identical eta-expansions for a single external

The main disadvantage of the new approach:

  • Previously some builtin primitives could be specialised during module coercions based on their type. Now they will always use the existing, possibly unspecialised version of the primitive stored in the module. For example 
    module M : sig
  val eq_int: int -> int -> bool
end = struct
  external eq_int : 'a -> 'a -> bool = "%equal"
end
    would previously create a module that used integer equality, but would now create a module that used generic equality. Of course, if the type of the external were changed to int -> int -> bool then the new scheme would also use integer equality. Similarly, if the types were not changed until a later coercion then the old scheme would have used generic equality.

Note that this PR is rebased on top of #1557 so reviewers should ignore the first commit.

@alainfrisch
Copy link
Copy Markdown
Contributor

alainfrisch commented Jan 12, 2018

Does this make linking to external primitives less "weak"? I believe that today, one can link against a module with some external primitives without requiring to link an implementation for those primitives (if the primitives are never called or used indirectly).

@lpw25
Copy link
Copy Markdown
Contributor Author

lpw25 commented Jan 12, 2018

Yes it does. That required some changes to the testsuite. Do you know if anyone uses that for anything?

I also adjusted the rules for when using an external forces linking. Previously it did not apply for builtin primitives, which seemed pretty arbitrary. It would be easy to put it back though if people insisted.

@alainfrisch
Copy link
Copy Markdown
Contributor

alainfrisch commented Jan 12, 2018

Do you know if anyone uses that for anything?

I'm not aware of any specific use, but I wouldn't be surprised that it's used in the wild, perhaps not deliberately; and probably in contexts which would be easy to fix, but perhaps not.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jan 12, 2018

I seemed to remember that we changed something about uses of external through a signature that has the effect that these uses already incur a dependency on the module -- this was MPR#6956, but I don't remember the details. If that is the case, maybe most of the strange uses were already broken by this previous change?

@damiendoligez
Copy link
Copy Markdown
Member

damiendoligez commented Feb 6, 2018

I think that was the flip-side of what @alainfrisch is describing: you could use the external primitives without linking the module.

@garrigue
Copy link
Copy Markdown
Contributor

garrigue commented Feb 7, 2018

  • I didn't think much about it then, but I believe there are cases where one would prefer that built-in primitives do not force linking. A typical example is %identity which is explicitly a no-op, and as a result cannot have dependencies.

  • I'm a bit concerned by the loss of optimization of primitives, for functors for instance.
    Is there a good reason to assume that this optimization is superfluous?

@damiendoligez
Copy link
Copy Markdown
Member

damiendoligez commented Feb 12, 2018

I tried this PR on opam packages and it fails on jbuilder with a compiler fatal error:

- /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/bin/ocamlopt.opt -w -40 -o boot.exe unix.cmxa boot.ml
- Fatal error: exception Invalid_argument("index out of bounds")
[ERROR] The compilation of jbuilder failed at "ocaml bootstrap.ml".

I get this on all jbuilder versions between 1.0+beta9 and 1.0+beta16.

It seems that no other package (that currently compiles with trunk) is impacted.

I'll try to dig a little deeper.

@gasche
Copy link
Copy Markdown
Member

gasche commented Feb 12, 2018

Having a stack trace of the compiler failure would be nice.

(This looks quite like the jbuilder error due to pattern matching changes, which could still be present in this PR if it targets a version of trunk that is buggy in this way and has not been rebased since.)

@trefis
Copy link
Copy Markdown
Contributor

trefis commented Feb 12, 2018

which could still be present in this PR if it targets a version of trunk that is buggy in this way and has not been rebased since

That indeed seems to be the case.

@damiendoligez
Copy link
Copy Markdown
Member

damiendoligez commented Feb 12, 2018

The exception is raised by the anonymous function passed to List.iter in Matching.record_matching_line, so it looks like you're right.

@damiendoligez
Copy link
Copy Markdown
Member

damiendoligez commented Feb 13, 2018

Update: clarity gives me a fatal error in Bytegen.comp_primitive.

full opam log:

The following actions will be performed:
  - install clarity 0.3.0

=-=- Gathering sources =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
[clarity: http] Command started
[clarity: http] Command started
[clarity.0.3.0] https://github.com/IndiscriminateCoding/clarity/archive/0.3.0.zip downloaded

=-=- Processing actions -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
[clarity: jbuilder build] Command started
+ jbuilder "build" (CWD=/home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/build/clarity.0.3.0)
-       ocamlc lib/clarity__.{cmi,cmo,cmt}
-     ocamldep lib/clarity.dependsi.ocamldep-output
-       ocamlc lib/clarity__Fn.{cmi,cmti}
-     ocamldep lib/clarity.depends.ocamldep-output
-       ocamlc lib/clarity__Functor.{cmi,cmti}
-       ocamlc lib/clarity__Semigroup.{cmi,cmti}
-       ocamlc lib/clarity__Void.{cmi,cmti}
-     ocamlopt lib/clarity__.{cmx,o}
-       ocamlc lib/clarity__Traversable.{cmi,cmti}
-       ocamlc lib/clarity__Fn.{cmo,cmt}
-       ocamlc lib/clarity__Semigroup.{cmo,cmt}
-       ocamlc lib/clarity__Functor.{cmo,cmt}
-       ocamlc lib/clarity__Applicative.{cmi,cmti}
-       ocamlc lib/clarity__Monoid.{cmi,cmti}
-       ocamlc lib/clarity__Void.{cmo,cmt}
-     ocamlopt lib/clarity__Semigroup.{cmx,o}
-     ocamlopt lib/clarity__Fn.{cmx,o}
-     ocamlopt lib/clarity__Void.{cmx,o}
-       ocamlc lib/clarity__Traversable.{cmo,cmt}
-       ocamlc lib/clarity__Foldable.{cmi,cmti}
-       ocamlc lib/clarity__Monoid.{cmo,cmt}
-       ocamlc lib/clarity__Applicative.{cmo,cmt}
-       ocamlc lib/clarity__Monad.{cmi,cmti}
-       ocamlc lib/clarity__Foldable.{cmo,cmt}
-     ocamlopt lib/clarity__Monoid.{cmx,o}
-     ocamlopt lib/clarity__Traversable.{cmx,o}
-     ocamlopt lib/clarity__Functor.{cmx,o}
-       ocamlc lib/clarity__Id.{cmi,cmti}
-       ocamlc lib/clarity__Either.{cmi,cmti}
-       ocamlc lib/clarity__Option.{cmi,cmti}
-       ocamlc lib/clarity__Reader.{cmi,cmti}
-       ocamlc lib/clarity__State.{cmi,cmti}
-       ocamlc lib/clarity__Monad.{cmo,cmt}
-       ocamlc lib/clarity__These.{cmi,cmti}
-       ocamlc lib/clarity__Writer.{cmi,cmti}
-       ocamlc lib/clarity__Id.{cmo,cmt}
-     ocamlopt lib/clarity__Id.{cmx,o}
-     ocamlopt lib/clarity__Foldable.{cmx,o}
-       ocamlc lib/clarity__Validation.{cmi,cmti}
-       ocamlc lib/clarity__Either.{cmo,cmt}
-       ocamlc lib/clarity__Option.{cmo,cmt}
-     ocamlopt lib/clarity__Applicative.{cmx,o}
-       ocamlc lib/clarity__Reader.{cmo,cmt} (exit 2)
- (cd _build/default && /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/bin/ocamlc.opt -w +44+45 -strict-sequence -principal -g -bin-annot -no-alias-deps -I lib -open Clarity__ -o lib/clarity__Reader.cmo -c -impl lib/reader.ml)
- >> Fatal error: Bytegen.comp_primitive
- Fatal error: exception Misc.Fatal_error
-       ocamlc lib/clarity__.{cmi,cmo,cmt}
-     ocamldep lib/clarity.dependsi.ocamldep-output
-       ocamlc lib/clarity__Fn.{cmi,cmti}
-     ocamldep lib/clarity.depends.ocamldep-output
-       ocamlc lib/clarity__Functor.{cmi,cmti}
-       ocamlc lib/clarity__Semigroup.{cmi,cmti}
-       ocamlc lib/clarity__Void.{cmi,cmti}
-     ocamlopt lib/clarity__.{cmx,o}
-       ocamlc lib/clarity__Traversable.{cmi,cmti}
-       ocamlc lib/clarity__Fn.{cmo,cmt}
-       ocamlc lib/clarity__Semigroup.{cmo,cmt}
-       ocamlc lib/clarity__Functor.{cmo,cmt}
-       ocamlc lib/clarity__Applicative.{cmi,cmti}
-       ocamlc lib/clarity__Monoid.{cmi,cmti}
-       ocamlc lib/clarity__Void.{cmo,cmt}
-     ocamlopt lib/clarity__Semigroup.{cmx,o}
-     ocamlopt lib/clarity__Fn.{cmx,o}
-     ocamlopt lib/clarity__Void.{cmx,o}
-       ocamlc lib/clarity__Traversable.{cmo,cmt}
-       ocamlc lib/clarity__Foldable.{cmi,cmti}
-       ocamlc lib/clarity__Monoid.{cmo,cmt}
-       ocamlc lib/clarity__Applicative.{cmo,cmt}
-       ocamlc lib/clarity__Monad.{cmi,cmti}
-       ocamlc lib/clarity__Foldable.{cmo,cmt}
-     ocamlopt lib/clarity__Monoid.{cmx,o}
-     ocamlopt lib/clarity__Traversable.{cmx,o}
-     ocamlopt lib/clarity__Functor.{cmx,o}
-       ocamlc lib/clarity__Id.{cmi,cmti}
-       ocamlc lib/clarity__Either.{cmi,cmti}
-       ocamlc lib/clarity__Option.{cmi,cmti}
-       ocamlc lib/clarity__Reader.{cmi,cmti}
-       ocamlc lib/clarity__State.{cmi,cmti}
-       ocamlc lib/clarity__Monad.{cmo,cmt}
-       ocamlc lib/clarity__These.{cmi,cmti}
-       ocamlc lib/clarity__Writer.{cmi,cmti}
-       ocamlc lib/clarity__Id.{cmo,cmt}
-     ocamlopt lib/clarity__Id.{cmx,o}
-     ocamlopt lib/clarity__Foldable.{cmx,o}
-       ocamlc lib/clarity__Validation.{cmi,cmti}
-       ocamlc lib/clarity__Either.{cmo,cmt}
-       ocamlc lib/clarity__Option.{cmo,cmt}
-     ocamlopt lib/clarity__Applicative.{cmx,o}
-       ocamlc lib/clarity__Reader.{cmo,cmt} (exit 2)
- (cd _build/default && /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/bin/ocamlc.opt -w +44+45 -strict-sequence -principal -g -bin-annot -no-alias-deps -I lib -open Clarity__ -o lib/clarity__Reader.cmo -c -impl lib/reader.ml)
- >> Fatal error: Bytegen.comp_primitive
- Fatal error: exception Misc.Fatal_error
[ERROR] The compilation of clarity failed at "jbuilder build".

#=== ERROR while installing clarity.0.3.0 =====================================#
# opam-version         1.2.2+dd (3852ba0a7708d4dc37f64dd4cd00f873bf317ac0)
# os                   linux
# command              jbuilder build
# path                 /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/build/clarity.0.3.0
# compiler             4.07.0+trunk+pr1566
# exit-code            1
# env-file             /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/build/clarity.0.3.0/clarity-1899.env
# stdout-file          /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/build/clarity.0.3.0/clarity-1899.out
# stderr-file          /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/build/clarity.0.3.0/clarity-1899.out
### stdout ###
# [...]
#     ocamlopt lib/clarity__Id.{cmx,o}
#     ocamlopt lib/clarity__Foldable.{cmx,o}
#       ocamlc lib/clarity__Validation.{cmi,cmti}
#       ocamlc lib/clarity__Either.{cmo,cmt}
#       ocamlc lib/clarity__Option.{cmo,cmt}
#     ocamlopt lib/clarity__Applicative.{cmx,o}
#       ocamlc lib/clarity__Reader.{cmo,cmt} (exit 2)
# (cd _build/default && /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/bin/ocamlc.opt -w +44+45 -strict-sequence -principal -g -bin-annot -no-alias-deps -I lib -open Clarity__ -o lib/clarity__Reader.cmo -c -impl lib/reader.ml)
# >> Fatal error: Bytegen.comp_primitive
# Fatal error: exception Misc.Fatal_error
### stderr ###
# [...]
#     ocamlopt lib/clarity__Id.{cmx,o}
#     ocamlopt lib/clarity__Foldable.{cmx,o}
#       ocamlc lib/clarity__Validation.{cmi,cmti}
#       ocamlc lib/clarity__Either.{cmo,cmt}
#       ocamlc lib/clarity__Option.{cmo,cmt}
#     ocamlopt lib/clarity__Applicative.{cmx,o}
#       ocamlc lib/clarity__Reader.{cmo,cmt} (exit 2)
# (cd _build/default && /home/doligez/opamcheck/sandbox2/opamstate/4.07.0+trunk+pr1566/dotopam/4.07.0+trunk+pr1566/bin/ocamlc.opt -w +44+45 -strict-sequence -principal -g -bin-annot -no-alias-deps -I lib -open Clarity__ -o lib/clarity__Reader.cmo -c -impl lib/reader.ml)
# >> Fatal error: Bytegen.comp_primitive
# Fatal error: exception Misc.Fatal_error



=-=- Error report -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
The following actions failed
  - install clarity 0.3.0
No changes have been performed
'opam install clarity.0.3.0' failed.

@lpw25 lpw25 mentioned this pull request Apr 9, 2018
Merged
@lpw25 lpw25 mentioned this pull request Jul 12, 2018
Closed
@lpw25 lpw25 force-pushed the no-primitive-coercions branch from b23cf73 to 2675597 Compare August 8, 2018 13:47
@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Oct 16, 2019

1.5 years later, any news about this PR?

For the record, I'm neutral about it: it does simplify the compiler internals, but we lose some opportunities for type-based specialization of primitives (and there are not enough such opportunities already...), and the "no breakage in OPAM" check is unconvincing and needs to be run again.

@lpw25
Copy link
Copy Markdown
Contributor Author

lpw25 commented Oct 16, 2019 •
edited
Loading

I'm fairly neutral on this change now myself. I now believe that this sentence from my description:

and will make adding some future module features noticeably more difficult.

is not really true, which makes me much less interested in it.

@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Oct 17, 2019

OK, thanks for reply. Since there is no longer much interest in the proposed change, and since it has some potential for breaking existing code (by requiring more libraries to be linked), let me close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lpw25 @alainfrisch @gasche @damiendoligez @garrigue @trefis @xavierleroy