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

Do not put a dummy method in object types #1620

Merged
merged 6 commits into from Mar 6, 2018

Conversation

Projects
None yet
5 participants
@trefis
Copy link
Contributor

trefis commented Feb 20, 2018

Dummy methods are inserted in object types to prevent scope escapes (and, a fortiori, mistakenly closing an object type).

This check is known to be brittle:

  • it will sometimes reject programs that it shouldn't, as illustrated by MPR#7391 .
  • it will sometimes fail to do its job and will allow closing some object types, which can then lead to assertion failures in later stage of typechecking, as illustrated by the test added in commit c3027c1 .

However, while that check is important for classes, there seem to be no reason to do it for objects: we always close the type of an object.

So in theory, we could fix the first issue by simply not adding a dummy method when "initializing" the type of an object. This is what is done in commit 1096f57. On its own however, it is not enough to fix the test cases produced in MPR#7391. This is because a dummy method makes its way inside the object type through inheritance. This also seems wrong: the dummy method should be made absent when finalizing the class declaration. This is done by commit 77cefbd .

These two changes taken together are enough to fix the first class of issues. But it also paves the way to fixing the second set of issues: having done the changes described above, if we ever find a dummy method inside the type of an object it means that the type of that object has been unified with the type of a class which hasn't been fully defined yet. That means we shouldn't close our object type, because it would imply closing the self type of the class being defined which isn't allowed. Commit 5333a22 implements that new check, which does indeed fix the example added by commit c3027c1 .

N.B.

  • the changes introduced by 77cefbd require a bootstrap, I included it in that commit.
  • lablgtk still compiles fine with the changes introduced here.
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 26, 2018

@garrigue: gentle ping, do you plan to do a review of this PR in the coming weeks?

@@ -329,6 +329,7 @@ let close_object ty =
match ty.desc with
Tvar _ ->
link_type ty (newty2 ty.level Tnil)
| Tfield(lab, _, _, _) when lab = dummy_method -> raise (Unify [])

This comment has been minimized.

@garrigue

garrigue Feb 27, 2018

Contributor

Shouldn't you test here for whether the kind is Fabsent ?

This comment has been minimized.

@trefis

trefis Feb 27, 2018

Author Contributor

No, repr already gets rid of Fabsent fields for us.

@@ -3519,7 +3520,7 @@ let match_class_types ?(trace=true) env pat_sch subj_sch =
| _ -> CM_Hide_public lab::err
end
in
if Concr.mem lab sign1.csig_concr then err
if lab = dummy_method || Concr.mem lab sign1.csig_concr then err

This comment has been minimized.

@garrigue

garrigue Feb 27, 2018

Contributor

Why this change? We are comparing class types, so they are bound to both contain the dummy method, no?

This comment has been minimized.

@trefis

trefis Feb 27, 2018

Author Contributor

Sadly, it is not that simple. Consider the following example (extracted/reduced from ocamldoc's codebase):

class type doc_generator =
  object method generate : int list -> unit end

class generator : doc_generator = object
  method generate _ = ()
end

When we arrive in match_class_types the dummy method is already Fabsent in doc_generator as the class type is already fully defined, but it is still Fvar in generator itself.
And since flatten_fields omits Fabsent fields from its result, the dummy method will be present in one of the lists and not the other.

So if that change is is not present, one gets this confusing error message:

Error: The class type object method generate : 'a -> unit end
       is not matched by the class type doc_generator
       The virtual method *dummy method* cannot be hidden

(you'll notice that *dummy method* doesn't appear it the object type, that will be because we've just changed it from Fvar to Fabsent).

@@ -3644,7 +3645,7 @@ let match_class_declarations env patt_params patt_type subj_params subj_type =
| _ -> CM_Hide_public lab::err
end
in
if Concr.mem lab sign1.csig_concr then err
if lab = dummy_method || Concr.mem lab sign1.csig_concr then err

This comment has been minimized.

@garrigue

garrigue Feb 27, 2018

Contributor

Same as previous comment.

This comment has been minimized.

@trefis

trefis Feb 27, 2018

Author Contributor

This one is indeed not necessary, contrarily to the previous one when this function is called both class (type) declarations are complete.

I'll revert it.

| _ -> ()
) methods
end;

This comment has been minimized.

@garrigue

garrigue Feb 27, 2018

Contributor

You should explain somewhere the new invariant:

  • the self_type of a final object type contain no dummy method from the beginning
  • for class definitions, a dummy method is added during the definition, but removed by final_decl, so that immediate objects one cannot inherit from not yet finished classes.

By the way, I wonder how it could happen: it seems that one cannot refer to a not yet finished declaration anyway (Unbound_class_2).

This comment has been minimized.

@trefis

trefis Feb 27, 2018

Author Contributor

Fair point, I'll add a comment above class_structure.

It can happen because we can unify the self type of a "final" object with the self type of a yet unfinished class; this was previously allowed but resulted in an assert false later on (I believe this bug has been here for ever, I reproduced the assert false from 4.03 all the way to trunk).
Such an example is added in commit c3027c1 .

| Closing_self_type self ->
fprintf ppf "@[Cannot close type of object literal:@ %a@.\
Self types cannot be closed.@]"
Printtyp.type_scheme self

This comment has been minimized.

@garrigue

garrigue Feb 27, 2018

Contributor

I don't think people will understand this message...

This comment has been minimized.

@trefis

trefis Feb 27, 2018

Author Contributor

Perhaps the usual gang ( @gasche , @Octachron ) will have a better proposition.

Personally I do not find it harder to understand than the other related messages, is there something in particular that you find obscure?

This comment has been minimized.

@Octachron

Octachron Feb 28, 2018

Contributor

I think you could share the explanation that you added in the code itself, something like

@[Cannot close type of object literal:@ %a@,\
      Some self types have been unified with the self type of a not yet finished class,@ \
      they cannot be closed@]

(also using "@." should be avoided since it sends the formatting engine of Format in panic mode)

This comment has been minimized.

@lpw25

lpw25 Feb 28, 2018

Contributor

How about:

"Cannot close type of object literal: it has been unified with the self type of a class that is not yet completely defined"

This reuses the terminology ("not yet completely defined") from similar error messages in the class system.

@trefis trefis force-pushed the trefis:objects-are-no-dummies branch from 26acace to 692ad2e Feb 27, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Mar 5, 2018

Thanks @Octachron and @lpw25 , I updated the error message as per your last suggestion.

@garrigue : are you satisfied with the state of this PR now? I assume, since you made technical comments about the implementation, that you are OK with the general idea?

@trefis trefis force-pushed the trefis:objects-are-no-dummies branch from 06a086a to a943877 Mar 6, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Mar 6, 2018

Thanks!

@trefis trefis merged commit e62cc3e into ocaml:trunk Mar 6, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@trefis trefis deleted the trefis:objects-are-no-dummies branch Mar 6, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented on 86e1c8a Jan 9, 2019

It seems that MPR#7894 is a regression that appears right after this commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.