rec_check: remove the duplicate array_type_kind classifier, use Typeopt#1922
rec_check: remove the duplicate array_type_kind classifier, use Typeopt#1922gasche merged 1 commit intoocaml:trunkfrom
Conversation
|
@yallop I tried to go back in the history to find out why there was this duplication in the first place. You started by moving typeopt.ml from bytecomp/ to typing/, but then at some point the array-classification code got duplicated into typecore (and later rec_check). I haven't been able to remember or understand why. My best guess is that you initially wanted the rec-check to be agnostic of Config.flat_float_array setting (it is only later that we refined it to take it into account), and thus you found the Typeopt classifier too precise/complex for your needs. In any case, I tried to reason carefully about the various cases ({float,generic} arrays {with,without} flat float arrays), and I believe that the Typeopt classifier is sound for rec_check, but of course maybe I missed something. |
yallop
left a comment
There was a problem hiding this comment.
I think this is a good change. The inlined code from Typeopt seemed to come from quite an old version (pre-4.04), so it didn't include some recent updates that handled bytes (for example). Consequently there are programs that are accepted with this PR that were previously rejected, including this one:
let f (z: bytes) = let rec x = [| y; z |] and y = z in x;;which was allowed in 4.04, disallowed in 4.07, and is now allowed again with this PR, since bytes is known to be incompatible with float.
I've suggested a couple of changes to the comments in the testsuite, but I'm otherwise happy.
| OCaml versions up to at least 4.07), creating an array inspects its | ||
| first element to decide whether it is a float or not; it would thus | ||
| be unsound to allow to recursively define a float value and an | ||
| be unsound to allow to recursively define avalue and an |
There was a problem hiding this comment.
There's a typo here: avalue.
The comment was perhaps too general before, but now it's not general enough. Recursively defining a value and an array starting with that element is fine if the value is known to be a non-float. More concretely:
(* Fine: z is known to be a non-float *)
let f (z: int) = let rec x = [| y; z |] and y = z in x
(* Disallowed: z is known to be a float, and will be unboxed during array construction *)
let f (z: float) = let rec x = [| y; z |] and y = z in x
(* Disallowed: z is not known to be a non-float, and will be inspected during array construction *)
let f z = let rec x = [| y; z |] and y = z in x| be unsound to allow to recursively define a float value and an | ||
| be unsound to allow to recursively define avalue and an | ||
| array starting with that element (in general we disallow using a | ||
| recursively-defined value in an array literal). |
There was a problem hiding this comment.
(in general we disallow using a recursively-defined value in an array literal).
This sentence (from a previous PR) is not correct: see above. It could be fixed by adding something along the lines of "unless the value is known to have a type not compatible with float".
ceb6ac9 to
329be3a
Compare
A side-result of this change is that, because Typeopt classifies parametric arrays in a more flexible way that the Rec_check code did (when -no-flat-float-array is set, they are marked Paddrarray rather than Pgenarray, to reflect the fact that not dynamic test for unboxing is performed), the -no-flat-float-array compiler starts accepting *more* recursive definition than before: basically, arrays at all types act just like constructors. Our testsuite had a case (letrec-disallowed/generic_arrays.ml) that specifically check that this case of generic array is disallowed, even under -no-flat-float-array. I changed the tests to reflect the finer-grained criterion -- and converted them to expect-style tests along the way.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) | Unused (* the value is not used at all *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) | Unused (* the value is not used at all *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This work was conducted by Alban Reynaud during an internship at INRIA Saclay, with the help and under the supervision of Gabriel Scherer in the Parsifal team. A summary is included below. For more details, see https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value or the comments in the code itself, which has evolved a bit from Alban's internship report above. Summary: -------- Rec_check is based on the idea of assigning "access modes" to variables m ::= Dereference (* full access and inspection of the value *) | Unguarded (* the value result is directly returned *) | Guard (* the value is stored under a data constructor *) | Delayed (* the value is not needed at definition time *) | Unused (* the value is not used at all *) Before this patchset, the implementation (contributed by Jeremy Yallop) was formulated as a type-checker manipulating "uses", which are mappings from free variables to modes u ::= (x : m)* and contexts Γ mapping variables to uses Γ ::= (x : u)* The check relied on a judgment of the form Γ ⊢ e : u where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is the output — this could be written `-Γ ⊢ e : +u`. After this patchset, the implementation uses a simpler notion of context mapping variables to mods Γ ::= (x : m)* and the judgment has the simpler structure Γ ⊢ e : m but now `m` is an input to the judgment, indicating in which usage context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`. Other fixes: ------------ This patchset also fixes a soundness bug around `{ foo with ... }` which is already present in 4.06 and 4.07. Another soundness bug found during this work was submitted and fixed independently as GPR ocaml#1915. This work also led to the refactoring of GPR ocaml#1922, and the refactoring of the testsuite in GPR ocaml#1937.
This PR supersedes the more timid change that I proposed in #1911: that other change is less invasive, but when I tried to justify why it is correct (thanks to the hard looks from @trefis), I found that my only strong argument was "if it isn't, then
Typeopt.array_type_kindalso isn't". This is not reasoning-by-contradiction but a simple contraposition, and the present PR is a constructive proof.A side-result of this change is that, because Typeopt classifies
parametric arrays in a more flexible way that the Rec_check code did
(when -no-flat-float-array is set, they are marked Paddrarray rather
than Pgenarray, to reflect the fact that not dynamic test for unboxing
is performed), the -no-flat-float-array compiler starts accepting
more recursive definition than before: basically, arrays at all
types act just like constructors.
Our testsuite had a case (letrec-disallowed/generic_arrays.ml) that
specifically check that this case of generic array is disallowed,
even under -no-flat-float-array. I changed the tests to reflect
the finer-grained criterion -- and converted them to expect-style
tests along the way.