Skip to content

must use set_state rather than backtrack in Typecore.type_cases#10006

Merged
gasche merged 1 commit intoocaml:trunkfrom
garrigue:type_cases_set_state
Nov 6, 2020
Merged

must use set_state rather than backtrack in Typecore.type_cases#10006
gasche merged 1 commit intoocaml:trunkfrom
garrigue:type_cases_set_state

Conversation

@garrigue
Copy link
Copy Markdown
Contributor

@garrigue garrigue commented Nov 5, 2020

#9811 wrongly used only Btype.backtrack after failure in a strict setting.
This could result in keeping wrong levels when retyping, and led to a failure in parsexp:
janestreet/parsexp#5

This PR fixes this by using save_state / set_state instead (with a dummy ref env).

@garrigue garrigue requested review from gasche and trefis November 5, 2020 02:29
@garrigue garrigue mentioned this pull request Nov 5, 2020
Merged
@Octachron Octachron added this to the 4.12 milestone Nov 5, 2020
Octachron
Octachron approved these changes Nov 6, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me, since the strict branch can fail at an unknown level, thus level needs to be restored before retyping in the relaxed mode.

(The dummy rev env feels a bit unfortunate, but replacing it with ref Env.empty does not seems really better.)

As a fix to a new 4.12 feature, the PR should be cherry-picked to 4.12 .

@gasche
Copy link
Copy Markdown
Member

gasche commented Nov 6, 2020

Ah, I forgot to comment on this PR. I agree it fixes the bug. I'm not fond of the Env.t ref API, but I am not sure that we should explicit state-passing instead (save_state : Env.t -> state; set_state : state -> Env.t).

@kit-ty-kate
Copy link
Copy Markdown
Member

kit-ty-kate commented Nov 6, 2020

Note that the bug this PR fixes also affects functoria.

Using the 4.12 branch:

$ dune build --profile=release
File "runtime/functoria_runtime.ml", lines 36-39, characters 35-24:
36 | ...................................match t.kind with
37 |     | Opt (d,_) -> Some d
38 |     | Flag -> Some false
39 |     | Required _ -> None
Warning 18 [not-principal]: 
  The return type of this pattern-matching is ambiguous.
  Please add a type annotation, as the choice of `a option' is not principal.
File "runtime/functoria_runtime.ml", line 1:
Error: The implementation runtime/functoria_runtime.ml
       does not match the interface runtime/.functoria_runtime.objs/byte/functoria_runtime.cmi:
       ...
       In module Key:
       Values do not match:
         val default : '_a t -> '_a option
       is not included in
         val default : 'a t -> 'a option
       File "runtime/functoria_runtime.mli", line 65, characters 2-33:
         Expected declaration
       File "runtime/functoria_runtime.ml", line 59, characters 6-13:
         Actual declaration

Using the 4.12 branch with this PR cherry-picked on top of it:

$ dune build --profile=release
File "runtime/functoria_runtime.ml", lines 36-39, characters 35-24:
36 | ...................................match t.kind with
37 |     | Opt (d,_) -> Some d
38 |     | Flag -> Some false
39 |     | Required _ -> None
Warning 18 [not-principal]: 
  The return type of this pattern-matching is ambiguous.
  Please add a type annotation, as the choice of `a option' is not principal.

@gasche
Copy link
Copy Markdown
Member

gasche commented Nov 6, 2020

Let's merge, then!

@gasche gasche merged commit d42e466 into ocaml:trunk Nov 6, 2020
gasche added a commit that referenced this pull request Nov 6, 2020
@gasche
Merge pull request #10006 from garrigue/type_cases_set_state
fead9e4 
must use set_state rather than backtrack in Typecore.type_cases

(cherry picked from commit d42e466)
@gasche
Copy link
Copy Markdown
Member

gasche commented Nov 6, 2020

Cherry-picked in 4.12 as fead9e4.

@kit-ty-kate kit-ty-kate mentioned this pull request Nov 6, 2020
Closed
21 tasks
This was referenced Nov 13, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Octachron Octachron Octachron approved these changes

@gasche gasche Awaiting requested review from gasche

@trefis trefis Awaiting requested review from trefis

Assignees

No one assigned

Labels

bug no-change-entry-needed

Projects

None yet

Milestone

4.12

Development

Successfully merging this pull request may close these issues.

4 participants

@garrigue @gasche @kit-ty-kate @Octachron