-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Don't treat definitions from the current module specially #8900
base: trunk
Are you sure you want to change the base?
Conversation
In your example, the special treatment looks legit: two abstract types defined in another module ( |
This is debatable. When I declare an abstract type I might mean a fresh empty type. I might also mean a non-empty type where all the values are constructed using C primitives. For example, Similarly, when I declare two abstract types I might mean two separate types, but I also might mean two equivalent types whose equivalence I do not intend to expose. Without a concrete definition OCaml cannot be certain that two types are genuinely incompatible. However, this is all besides the point. The issue is more practical than that. Treating types from the same structure differently simply creates confusion for no increase in expressivity. Having explained what is happening is such cases many, many times I think it would be easier to just remove this special casing. |
On paper I agree, but I'm fairly certain this is going to break some code, so we really need to evaluate this on opam. |
Yes… and no? Which doesn't mean that we shouldn't run this on opam. But I think once that's done we just want to merge, and notify maintainers that they should update their package. |
As much as I wish I hadn't created this difference in the first place, this needs to be debated and advertised. Personally I would advocate to wait until we provide a way to define abstract types with an identity, which would also avoid the workaround we have for abstract types from the initial environment. |
I think that this point should make us take a long, hard pause. |
I really hope this doesn't break many code snippets, because such snippets are very confusing for beginners. They are wrong and teach people to use GADTs incorrectly. I suspect that making such snippets be an explicit error will probably improve the situation for beginners. |
Maybe the way to make progress on this PR is to gather information from opam on the nature of the breakages that it creates? That should be some clue as to the nature of the incompatibilities introduced by the change. |
An example that works today and would be broken by the change is: type true_type
type false_type
type _ singleton_bool =
| True : true_type singleton_bool
| False : false_type singleton_bool Where However, the example I tried to write first is below, and I realized that it is in fact rejected today. type zero
type 'n succ
# type ('a, _) vec =
| Nil : ('a, zero) vec
| Cons : 'a * ('a, 'n) vec -> ('a, 'n succ) vec
;;
Error: In this definition, a type variable cannot be deduced
from the type parameters. If I understand correclty, this fails because axiomatized types (abstract types declared in the current module) are not considered injective in their parameters: This suggests that only zero-ary uses of axiomatized types could be used for GADTs in the wild, and that the axiomatized-types approach would (already today) soon show its limitations to the GADT user. |
I have run some tests on the opam repository comparing a version of the compiler with this PR integrated and without: The handful of new failures seems completely unrelated. |
Thanks @Octachron. I think those results on opam are very encouraging. Are people still nervous about making the change, or can we go ahead with it? If people are nervous, are there further experiments that would help make them less nervous? |
As I said above, I still advocate to wait until we have a way to give abstract types an identity, so that we could also remove the special case for predefined abstract types. |
I find the results generally reassuring. We had this intuition (Jacques mentioned it and I shared it) that many beginner-oriented tutorials would be affected by the change. Could we maybe look at a couple such places (starting with the nice introduction to GADTs by @Octachron in the manual) and check that there is no issue there as well? @Octachron here are some comments from my experience looking at your opamcheck-PR results:
I wish we could investigate point (2), which may be related to the current PR. |
Concerning tutorials, I would argue that any tutorial relying on the special behavior for local abstract types is dangerous. Such tutorial teaches how to write toy examples that become mysteriously broken once you try to transform them to real code by putting the code in a separated module. This is utterly confusing for most people. |
After a bit of investigation, the macaroon failure is not directly related, the package builds with just 4.08.0+pr8900 . Concerning tutorials, the manual tutorial and ocamlverse tutorials would not need to be changed after this PR. Scouring the first 10 pages of google results for blog post on GADTs in OCaml, I only found two blog posts using abstract types as type level GADT. (I sent a fix for one of them). However, there was a handful of lecture slides subject to this issue. (For instance, https://www.irif.fr/~treinen/teaching/pfav/cours/cours10.handout.pdf or http://yann.regis-gianas.org/mpri/01-gadts-checking.pdf ) |
cc @yurug @treinen: this change to OCaml will break your teaching documents for GADTs, because the pattern of declaring fully-abstract types locally will stop working. Instead of type empty
type non_empty
type _ gadt = Foo : empty gadt | Bar : non_empty gadt you should use known-distinct types for the phantom types, for example type empty = Empty
type non_empty = Non_empty
type _ gadt = Foo : empty gadt | Bar : non_empty gadt |
Hi @gasche! My mpri slides are already using this idiom, which I find more sensible. |
Do we want to consider this for 4.10? or would people like more time to make a decision? |
@garrigue Do you have a concrete plan for that ? If you don't, I propose we just merge the current version, and improve the situation for predefined abstract types later. |
I can write a proposal this week. Or earlier if you prefer. |
Just added a proof of concept proposal of unique types: #9042 |
I am still very much in favor of this PR, and still semi-regularly explaining that the special behavior for abstract types defined in the current module is a beginner trap. |
It's an old discussion, people are generally in agreement that the current behavior is not good, and the opam results are good. Sounds good, right? Besides the question of teaching materials (which I think is important, but basically impossible to quantify), the thing that is giving me pause is that personally I consider that the idiom @garrigue is patiently pushing for a different approach using "generative abstract types" (see #9042 and ocaml/RFCs#4). I haven't invested enough effort in the proposal to know what to think of it, but in general I admire the idea of not satisfying ourselves with a hack, and trying to find a feature that captures our intent here. (And I think that the external/generative/nominal proposals are also justified by other needs, for example @alainfrisch's interest in runtime type representations, it's not like the admittedly-small problem we are discussing here is the only motivation.) You want more support (at least from @garrigue and myself) for changing the behavior of |
I'm not especially keen on it, either, but I consider
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that I've been long in advancing the nominal types, I agree that it is better to get rid of this kludge.
I expected more changes in the testsuite.
This seems to show that I'm already always writing my code that doesn't depend on this.
Before merging, there should be a log message explaining the change and how to fix code in case it breaks it.
I am worried by the interaction between this choice and the fact that people are asking for structural subtyping specifically for empty types (see eg #9459). We can add structural subtyping without type equalities, but it feels to me like we are treading on very fragile ground with this choice. On the other hand I actually like @lpw25's proposal of using |
Could you say exactly what you're worried about? I don't see what problem there could be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to add @stedolan's segfaulting example from #11492 to the test suite in this PR.
We could also include a variation on the example that uses ascription rather than functors
module M = struct type t = int let int = Int end
include (M : sig type t val int : t is_int end)
since that's also currently unsound.
I am worried about the fact that someday |
I'm not worried about that, because it doesn't seem any more likely than |
I think maybe I'm missing something: whilst |
At least this currently works (OCaml 5.0) type t = |
type u = |
type _ gadt = T : t gadt | U : u gadt
let f : t gadt -> unit = function T -> ()
(* no exhaustivity warning *) |
Oh, @lpw25 is right, of course. While they're currently known to be unequal, they won't be after this PR. |
Hm, this also means that my old proposal to allow renaming constructors when re-exporting a datatype is definitely dead for good after this PR. (Not that I was very insistent on it or anything .) It will be even harder if this PR is merged to reason on the fact that types are known to be incompatible. I guess that it will be a motivation to look again at #9042 or other proposals in that space. |
524a04d
to
9f6c3dc
Compare
9f6c3dc
to
d657b1f
Compare
Rebased, comments addressed and Changes entry added |
Note: this PR should also update the manual: ocaml/manual/src/tutorials/gadtexamples.etex Lines 300 to 304 in 4d932a8
|
The type-checker treats abstract type definitions in the current specially:
In my experience this special treatment confuses people a lot and is not really useful for anything.
This PR removes the special treatment. This is not a backwards compatible change so it could probably use some testing on opam.