Skip to content

Fix #9640: regression introduced by #9623#9642

Merged
gasche merged 5 commits intoocaml:trunkfrom
garrigue:fix9640
Jun 5, 2020
Merged

Fix #9640: regression introduced by #9623#9642
gasche merged 5 commits intoocaml:trunkfrom
garrigue:fix9640

Conversation

@garrigue
Copy link
Copy Markdown
Contributor

@garrigue garrigue commented Jun 5, 2020

In merge_constraint, #9623 was precomputing a common sig_env for the whole signature, but forgot to do so recursively.
This PR fixes #9640.

@garrigue garrigue requested a review from gasche June 5, 2020 13:06
@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 5, 2020

I see how this fixes the problem, thanks!

I think the code would be easier to read if it was scructured as follows. (I thought of trying that during #9623 but I didn't want to change the code too much. I now regret not doing it, as I think it could have revealed the issue you are now fixing.)

  let rec merge_in_sig outer_env namelist sg =
    let sig_env = Env.add_signature sg outer_env in
    merge_path sig_env namelist sg
  and merge_path sig_env namelist sig_items =
    match (namelist, sig_items) with
    | ([name], sig_items) ->
        merge_name sig_env name None sig_items
    | (s :: namelist, (Sig_module(id, _, md, rs, priv) as item) :: rem)
      when Ident.name id = s ->
        let ((path, _path_loc, tcstr), new_sig_items) =
          merge_in_sig (extract_sig sig_env loc md.md_type) namelist in
        real_ids := path :: !real_ids;
        let item =
          match md, constr with
          | {md_type = Mty_alias _}, (Pwith_module _ | Pwith_type _) ->
            item
          | _ ->
            let newmd = { md with md_type = Mty_signature new_sig_items } in
            Sig_module(id, Mp_present, newmd, rs, priv)
        in
        (path, lid, tcstr),
        item :: rem
    | (_, []) ->
        raise(Error(loc, sig_env, With_no_component lid.txt))
    | (_, item :: rem) ->
        let (cstr, items) = merge_path sig_env namelist rem in
        cstr, item :: items
  and merge_name sig_env name row_id sig_items =
    match sig_items with
    | (* all the other cases *)
    | [] ->
        raise(Error(loc, sig_env, With_no_component lid.txt))
    | item :: rem ->
        let cstr, items = merge_name sig_env name row_id rem in
        cstr, item :: items
  in

@garrigue
Copy link
Copy Markdown
Contributor Author

garrigue commented Jun 5, 2020

I did the simpler part of your change (factorize signature processing and handling of submodules). The other part seems to duplicate more than it factorizes.

@garrigue
Copy link
Copy Markdown
Contributor Author

garrigue commented Jun 5, 2020

I also did one last factorisation for with_type / with_typesubst. Same idea: make clearer what is identical and what is different.
If you think this is one commit too much for a bug fix, I can remove it and make a separate PR.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

The factorization are good, and I believe that the fix is correct.

@gasche gasche merged commit 357d624 into ocaml:trunk Jun 5, 2020
gasche added a commit that referenced this pull request Jun 5, 2020
Fix #9640: regression introduced by #9623

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

gasche commented Jun 5, 2020

Merged, and cherry-picked in 4.11 ( ef2d35e ).

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

Successfully merging this pull request may close these issues.

Possible regression following 9623

2 participants