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

Polymorphic methods and fields on polymorphic variants #10664

Closed
mikeshulman opened this issue Sep 26, 2021 · 6 comments
Closed

Polymorphic methods and fields on polymorphic variants #10664

mikeshulman opened this issue Sep 26, 2021 · 6 comments

Comments

@mikeshulman
Copy link

There seems to be a problem with compiling pattern-matches against the application of a polymorphic method or field to a polymorphic variant. Here is an example with a method:

class idfunc =
  object
    method id : 'ab. ([< `A | `B ] as 'ab) -> 'ab = fun x -> x
  end

let act : [ `A | `B ] -> string =
 fun x -> match (new idfunc)#id x with `B -> "B" | `A -> "A"

let _ = print_endline (act `A)

This prints B, whereas it should print A. Here is a similar example with a field:

type idfunc = { id : 'ab. ([< `A | `B ] as 'ab) -> 'ab }

let f = { id = (fun x -> x) }

let act : [ `A | `B ] -> string =
 fun x -> match f.id x with `B -> "B" | `A -> "A"

let _ = print_endline (act `A)

This also prints B.

The problem goes away if any of the following changes are made:

  • Changing the type of id to 'ab. 'ab -> 'ab.
  • Changing the type of id to [ `A | `B ] -> [ `A | `B ].
  • Coercing the value (new idfunc)#id x or f.id x to [ `A | `B ] before matching.

In addition,

  • Calling act `B instead also prints B.
  • Switching the order of the match clauses causes the code to always print A instead.
  • Adding a catchall case | _ -> "C" at the end of the match (with or without changing the type of act to [> `A | `B ] -> string) changes the return value to C.

Some brief initial discussion was here.

@gasche
Copy link
Member

gasche commented Sep 26, 2021

Testing indicates that the bug was introduced in 4.07 (.0 or .1), and is absent from 4.06.

@gasche
Copy link
Member

gasche commented Sep 26, 2021

Bisecting indicates that d43ccfc introduced the bad behavior. cc @trefis @garrigue
(see #7618)

@gasche
Copy link
Member

gasche commented Sep 26, 2021

The bug seems caused by incorrect row-variable information for the scrutinee type of f.id x -- obtained by instantiating a first-class polymorphic type. (This smells like the kind of irritating issues that, besides the two above, @lpw25 would also know about)

The following patch, which simply disables the use of presence information for better pattern-matching compilation, makes the issue disappear.

diff --git a/lambda/matching.ml b/lambda/matching.ml
index 07639abac8..c28a5ec7c4 100644
--- a/lambda/matching.ml
+++ b/lambda/matching.ml
@@ -1807,7 +1807,7 @@ let divide_variant ~scopes row ctx { cases = cl; args; default = def } =
         in
         let head = Simple.head p in
         let variants = divide rem in
-        if row_field lab row = Rabsent then
+        if false && row_field lab row = Rabsent then
           variants
         else
           let tag = Btype.hash_variant lab in
@@ -2895,7 +2895,7 @@ let combine_variant loc row arg partial ctx def (tag_lambda_list, total1, _pats)
       (fun (_, f) ->
         match row_field_repr f with
         | Rabsent
-        | Reither (true, _ :: _, _, _) ->
+        | Reither (true, _ :: _, _, _) when false ->
             ()
         | _ -> incr num_constr)
       (row_fields row)

(Performing only the first change suffice, but I think requiring both is important to avoid inconsistencies cropping up.)

This is of course not the right fix: the row-variable presence information should be correct.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Sep 28, 2022
@yallop
Copy link
Member

yallop commented Sep 28, 2022

This (rather serious) bug is still present in trunk (as of c534f97).

@garrigue
Copy link
Contributor

Sorry, I had completely overlooked this one. Probably @gasche 's analysis reassured me that it was in good hands...
There is no a PR which I believe does the right thing. It may just create warnings if one uses GADTs under polymorphic variants.

garrigue added a commit to COCTI/ocaml that referenced this issue Oct 5, 2022
garrigue added a commit to COCTI/ocaml that referenced this issue Oct 5, 2022
garrigue added a commit to COCTI/ocaml that referenced this issue Nov 2, 2022
garrigue added a commit to COCTI/ocaml that referenced this issue Nov 2, 2022
garrigue added a commit to COCTI/ocaml that referenced this issue Nov 4, 2022
@gasche gasche closed this as completed in 81acd5d Nov 11, 2022
gasche added a commit that referenced this issue Nov 11, 2022
Fix #10664 by fixing `Ctype.copy` in  `erase_either` mode
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

No branches or pull requests

4 participants