-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Informal proposal for strong mode toplevel inference #28218
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple small things here and there, but otherwise this looks really nice.
It might be worth talking a little about how this compares to the current implementation. Which parts are different? Which currently-inferred won't be with this and vice versa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally found a bit of time to finish the review. ;-)
Clearly, the rule that inferred and annotated types must be treated identically is the most controversial point.
* The inference dependency of the identifier is itself if the identifier is | ||
a candidate for inference. Otherwise there are no inference dependencies. | ||
* A simple identifier denoting a formal parameter has the type with which it was | ||
annotated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the declaration of the identifier is a parameter that doesn't have an annotation then the identifier isn't an IE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* No inference dependencies | ||
* A function literal with an expression body for which all parameter types are | ||
type annotated and for which the return expression is an IE and has an | ||
inferred type, has the corresponding function type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should clarify what happens if the function literal is declared with "async". I'm assuming that the return type should be Future<flatten(T)>
(where T
is the inferred type of the return expression, and flatten
is as described in the spec).
Also, I notice that await ...
expressions are not considered IEs. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added await
and some other missing cases.
@leafpetersen can we land this? |
obvious type. | ||
* No inference dependencies | ||
* An instance creation expression with no omitted type arguments has the | ||
obvious type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this item different than the previous one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate. Done.
* A method invocation `o.m(...)` with no omitted type arguments where the | ||
receiver is an IE with inferred type `T` such that `T` provides a signature | ||
for `m` with return type `S` has type `S` (with any generic arguments | ||
substituted in). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are binary expressions handled as method invocations?
Is a + b
handled as a.+(b)
?
Will you specify additional logic for int + int
, int + double
, double + double
, etc?
Are unary expressions handled as method invocations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In general, the intention is that the inferred toplevel type should be the same as the inferred type in a local context. I added bullets for binary and unary expressions.
Top level inference has two phases: | ||
|
||
1. **Method override inference** | ||
* If you omit a return type or parameter type from an overridden method, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does exactly overridden method
mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added "or implemented". Intent is overriding a superclass method (including one from a mixin) or implementing an interface method. See the next section for more detail - are there specific things that I'm not capturing here or that are unclear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are fairly complex rules about methods overriding in the specification. I try to understand whether you indent that we follow all these rules, or whether we use simple 'find by name and infer formal parameter types one-by-one'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more concrete about cases where the interpretation differs? Note I am not defining how inference behaves on erroneous cases such as defining a method with the same name as a field from a superclass, or defining a method with the same name as a superclass method with an incompatible signature. Those are errors: the tool can choose how best to handle surfacing those errors in a way that doesn't cause noise from cascading inference errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example this code is not an error.
But what is the type of a
in C
?
class A {
foo(num a) {}
}
class B {
foo(double a) {}
}
class C extends A implements B {
foo(a) {}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's intended to be addressed in the section below:
If there are multiple overridden/implemented methods and
the type to be filled in does not agree, it is an error.
So your example is an inference failure error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
The exact meaning of does not agree
was not clear.
Now I understand what it means.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified.
* No inference dependencies | ||
* A simple or qualified identifier referring to a top level function, static | ||
variable, field, getter; or a static class variable, static getter or method; | ||
or an instance method; has the inferred type of the identifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing this to ...has the inferred type of the referent.
an error. | ||
* Note: specifically, references to instance fields and instance getters are | ||
disallowed here. | ||
* The inference dependency of the identifier is itself if the identifier is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing this to The inference dependency of the identifier is the referent if the referent is a candidate for inference...
* Inference dependencies are those of the left hand operand. | ||
* A multiplicative binary expression is treated as a method call as defined in | ||
the spec, with the same exceptions for when the left hand operand is of type `int`. | ||
* Inference dependencies are those of the left hand operand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work for *
and %
, since the exceptions for when the left hand operand is of type int
require us to know the type of the right hand operand; we can't know this type reliably unless the right hand operand is also an interface dependency.
A similar situation exists for +
and -
, which it looks like you forgot to cover (I don't see a bullet addressing additive expressions).
Suggestion: change this to:
- A multiplicative or additive binary expression is treated as a method call as defined in the spec, with the same exceptions for when the left hand operand is of type
int
.- Inference dependencies are those of the left hand and right hand operands if the operator is
*
,%
,+
, or-
. - Otherwise, inference dependencies are those of the left hand operand.
- Inference dependencies are those of the left hand and right hand operands if the operator is
The informal spec for strong mode top level inference (#28218) says "If there are multiple overridden/implemented methods, and any two of them have non-equal types (declared or inferred) for a parameter position which is being inferred for the overriding method, it is an error." This CL fixes several SDK errors that arise from this rule. For example, the classes _Closure, Function, and Object contained members declared as follows: class _Closure implements Function { bool operator ==(other) ... } class Function { bool operator ==(Object other) ... } class Object { bool operator ==(other) ... } The type of Object's operator == was (dynamic) -> bool; the type of Function's operator == was (Object) -> bool; therefore the type of _Closure's operator == (which overrides both, since _Closure extends Object and implements Function) cannot be inferred and must be specified. A similar situation exists for _Double and _IntegerImplementation (both implement num, which declares operator == to have type (Object) -> bool), and _Uri (which implements Uri, which declares operator == to have type (Object) -> bool). This CL fixes the error by specifying the type explicitly in the classes _Closure, _Double, _IntegerImplementation, and _Uri. Change-Id: I91f7ceef8549399d438ba4be8c408493b3f338db Reviewed-on: https://dart-review.googlesource.com/28100 Reviewed-by: Vyacheslav Egorov <[email protected]>
@@ -60,7 +60,7 @@ void main(List<String> args) { | |||
'--modules=legacy', // TODO(vsm): Change this to use common format. | |||
'--single-out-file', | |||
'--inline-source-map', | |||
'-p', | |||
'--packages', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this an accidental change? It doesn't seem like it belongs in this pull request
@leafpetersen can we merge this? |
We shouldn't merge it. I'd rather move this to the language repo. I'll sort it out next week and then close this. |
This is an informal specification of proposed changes to strong mode top level inference (#27499).