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

Type annotations on methods cannot control the choice of abbreviation #5545

Closed
vicuna opened this issue Mar 20, 2012 · 10 comments
Closed

Type annotations on methods cannot control the choice of abbreviation #5545

vicuna opened this issue Mar 20, 2012 · 10 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Mar 20, 2012

Original bug ID: 5545
Reporter: @alainfrisch
Assigned to: @garrigue
Status: closed (set by @xavierleroy on 2016-12-07T10:47:27Z)
Resolution: fixed
Priority: low
Severity: tweak
Target version: 4.02.3+dev
Fixed in version: 4.03.0+dev / +beta1
Category: typing
Related to: #5619
Monitored by: @jmeber

Bug description

When using type abbreviations, annotations usually allow the programmer to pick an explicit choice for naming the type of a sub-expression:

type foo = int

let foo =
let x : foo = 10 in
let y : int = x in
(x, y)

--> val foo : foo * int

For objects, this does not work so well:

class o =
object(this)
method x : foo = 10
method y : int = this # x
end

--> class o : object method x : foo method y : foo end

Note that despite the type annotation on method y, the inferred type is "foo", not "int". We can get "int" by putting the annotation on the body:

class o =
object(this)
method x : foo = 10
method y = (this # x : int)
end

--> class o : object method x : foo method y : int end

However, this does not work in general. In the following example, the definition of method y "pollutes" the type inferred for method x, and I cannot see a way to force the type-checker to report type "int" (without touching the definition for y):

class o =
object(this)
method x : int = (10 : int)
method y = (this # x : foo)
end

--> class o : object method x : foo method y : foo end

File attachments

@vicuna
Copy link
Author

vicuna commented Mar 21, 2012

Comment author: @garrigue

A solution to this problem is to use the -principal option, as it duplicates enough nodes to avoid this kind of interferences.
Whether this should be done in all cases is another question. Originally, the unification algorithm was designed so as to propagate abbreviations. You are suggesting that this behavior is wrong in some cases.
Note that here int and foo are supposed to be strictly equivalent. The semantics of ocaml do not distinguish between the two.
The only place where abbreviations are guaranteed to be kept is in interfaces. Indeed, if foo is made abstract there, they would be different.

@vicuna
Copy link
Author

vicuna commented Mar 21, 2012

Comment author: @alainfrisch

A solution to this problem is to use the -principal option

I tried that, but even for the last example, the call to "this # x" pollutes the type for method x with -principal.

Originally, the unification algorithm was designed so as to propagate abbreviations.

I'm surprised by the difference between standard let definitions, where type annotations seem to be enough to have the type-checker picks the desired name, and typing of objects.

@vicuna
Copy link
Author

vicuna commented Mar 20, 2015

Comment author: @alainfrisch

Note: since commit 12289, method type are unshared in -principal mode.

@vicuna
Copy link
Author

vicuna commented Mar 25, 2015

Comment author: @garrigue

generalize_spine.diff attempts to do the type separation cleanly, without waking up #5619.
I.e. only use generalized types for methods called through self.
This way it can be enabled even in non-principal mode.

Does it do what you want?

@vicuna
Copy link
Author

vicuna commented Mar 25, 2015

Comment author: @garrigue

For the record, enabling it for normal mode has no noticeable performance cost, but increases the size of the cmi's for ocamldoc by 2% (476KB -> 486KB).

@vicuna
Copy link
Author

vicuna commented Mar 25, 2015

Comment author: @alainfrisch

Thanks Jacques!

I've tested the patch and it works as expected. I've uploaded a diff that extends your patch with a note in Changes, a new test testsuite/tests/typing-objects/pr5545.ml, and updated expected results for two tests:

  • testsuite/tests/typing-objects/Tests.ml

    The result is now the same with and without -principal, and it seems correct.

  • testsuite/tests/typing-poly/poly.ml

    A new warning ("this use of a polymorphic method is not principal") is triggered after the change. I don't know if this is correct or not. Can you have a look?

@vicuna
Copy link
Author

vicuna commented Mar 25, 2015

Comment author: @alainfrisch

Something else: should we done something about the call to generalize_spine in Typeclass.initial_env? (It is currently done only in principal mode.)

@vicuna
Copy link
Author

vicuna commented Mar 25, 2015

Comment author: @garrigue

Committed patch_5545.diff in trunk at revision 15964.

Concerning the use of generalize_spine in initial_env, its role is different.
First, it acts on a fresh type, so it is just an abbreviation for begin_def / end_def / generalize_structure.
Next, its role (in the non-principal mode) would be to ensure that the type of the constructor is left unchanged. However this doesn't look like the right place to do that:

# class c (x : int) = object method x = x method y = new c (1 : foo) end;;
class c : foo -> object method x : int method y : c end

in both normal and principal mode.
This is similar to recursive definitions:

# let rec f (x : int) = if x = 0 then x else f (1 : foo);;
val f : foo -> int = <fun>

@vicuna
Copy link
Author

vicuna commented Mar 26, 2015

Comment author: @alainfrisch

Thanks!

What about the new warning in testsuite/tests/typing-poly/poly.ml?

@vicuna
Copy link
Author

vicuna commented Mar 27, 2015

Comment author: @garrigue

It is a result of the "cleaner" approach: only methods of self itself are generalized, not those of objects with the same type as self.
I didn't come up with a repro case, but I'm afraid the old approach could lead to bugs such as 5619.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants