fix #25573, add option to disable implicit dynamic

fix #25573, add option to disable implicit dynamic

R=leafp@google.com

Committed: https://github.com/dart-lang/sdk/commit/844c82b5f3e478a52460b51550f06462607c8c74

[Jennifer Messerly, 2016-06-22 23:58:05]

jmesserly@google.com changed reviewers:
+ leafp@google.com

[Jennifer Messerly, 2016-06-22 23:58:06]

Hi Leaf,

I wanted to get your opinion on the design of this. Definitely take a look at
the test cases first and see if they match your expectations.

Contrary to one of my doc comments (which I'll fix) this CL does not get us to
message safety even if you avoid ever writing "dynamic", due to allowing
implicit dynamic in types (e.g. `class C<T> { T some(); } C c = new C<int>();
c.some().thing();`). However, getting totally away from that seems difficult as
we'd discussed (`is` checks, to pick one example), so that may be addressed in a
follow up, probably under a different flag.

Note also, I need to test out how well these messages work as UI, I anticipate
there may be some tweaks to them once this is added to the front-ends like
CLI/server/DDC.

Thanks!
- John

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:936: static Iterable<DartType>
recoverTypeArguments(
Moved from DDC:

https://github.com/dart-lang/dev_compiler/blob/2c19b4520cb748f52e01e44c70a588...

[Bob Nystrom, 2016-06-23 23:25:52]

rnystrom@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2016-06-23 23:25:53]

I just went through the tests but there's my thoughts.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1924: var
/*error:IMPLICIT_DYNAMIC_FIELD*/x1 = (<dynamic>[])[0];
This one is a little surprising to me. My intuition is that a value resulting
from an invocation on an explicit dynamic is also an explicit dynamic and would
be OK, but I don't have a strong opinion. I can see the value in flagging this.

I wonder what other people would think about this.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1961: }
These test cases are fantastic. So thorough!

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1970: List<dynamic> l2 =
/*error:IMPLICIT_DYNAMIC_LIST_LITERAL*/[];
This case is interesting too. This might be one to noodle on. I'm not sure if I
consider downward inference "explicit" or not.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1972: var l3 =
/*error:IMPLICIT_DYNAMIC_LIST_LITERAL*/[d, d];
I *think* I would consider this to be explicit. One way to look at it is like
this:

makePair<T>(T obj) => [obj, obj];

We wouldn't report an error on this method definition, which means there's no
error if you do:

dynamic d = ...;
makePair(d);

So that makes me feel like this is pretty much explicit too:

makePair(dynamic obj) => [obj, obj];

What do you think?

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2019: new
D<int>()./*error:IMPLICIT_DYNAMIC_METHOD*/m(d);
This feels like an explicit use of dynamic to me since the type argument is
inferred from something explicitly dynamic. What do you think?

[Jennifer Messerly, 2016-06-24 01:15:56]

Thanks Bob for taking a look. I'll do some more investigation on feasibility.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1924: var
/*error:IMPLICIT_DYNAMIC_FIELD*/x1 = (<dynamic>[])[0];
On 2016/06/23 23:25:52, Bob Nystrom wrote:
> This one is a little surprising to me. My intuition is that a value resulting
> from an invocation on an explicit dynamic is also an explicit dynamic and
would
> be OK, but I don't have a strong opinion. I can see the value in flagging
this.
> 
> I wonder what other people would think about this.

Yeah, I added it for illustration. The current technique lacks power to
implement different behavior. Think of it as a simple prototype -- Dart 2 may
want something more sophisticated.

The current model can be thought of as:
* find a place where type was omitted,
* see if dynamic was the conclusion,
* if so -> error

There are certainly other approaches. For example, we could internally track
"implicit dynamic" vs "explicit dynamic" as different types in Analyzer. That
seems tricky.

Another approach is to integrate this with inference. If we infer a type at all
(even if "dynamic"), then don't report an error. But we don't track inference
locations precisely enough to do that by the time we arrive in ErrorVerifier.

Perhaps there's a way to move the implementation into ResolverVisitor itself.
ResolverVisitor is a lot more hairy, which is why I implemented this in
ErrorVerifier, but I think that would get what you want without needing to
introduce any new types or inference tracking.

I'll reflect on it though.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1961: }
On 2016/06/23 23:25:52, Bob Nystrom wrote:
> These test cases are fantastic. So thorough!

Thanks. Wanted to illustrate what it does, especially because I'm not sure we
have it right yet. :)

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1970: List<dynamic> l2 =
/*error:IMPLICIT_DYNAMIC_LIST_LITERAL*/[];
On 2016/06/23 23:25:52, Bob Nystrom wrote:
> This case is interesting too. This might be one to noodle on. I'm not sure if
I
> consider downward inference "explicit" or not.

Yeah this dovetails into my other comment. We would really need to integrate
this into inference, and make sure inference doesn't push down a type in this
case:

    List l2 = ...;

but does in this case:

    List<dynamic> l2 = ...;

I don't think we currently have a distinction between those at all during
resolution/inference.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1972: var l3 =
/*error:IMPLICIT_DYNAMIC_LIST_LITERAL*/[d, d];
On 2016/06/23 23:25:52, Bob Nystrom wrote:
> I *think* I would consider this to be explicit. One way to look at it is like
> this:
> 
> makePair<T>(T obj) => [obj, obj];
> 
> We wouldn't report an error on this method definition, which means there's no
> error if you do:
> 
> dynamic d = ...;
> makePair(d);

That is an error in the current impl. makePair is implicitly instantiated as
`makePair<dynamic>(d)`.

> 
> So that makes me feel like this is pretty much explicit too:
> 
> makePair(dynamic obj) => [obj, obj];
> 
> What do you think?

Well, this fits with your other comments :). What I am hearing is you want it to
treat inferred dynamic as explicit.

That is definitely sensible. After all, if we infer dynamic from something,
either:
A. it was explicit somewhere else, or
B. we issued an error somewhere else.

So either way, we aren't adding anything by issuing another error.

As I said somewhere further up let me think on this. Perhaps I can move the
implementation to ResolverVisitor without too much trouble. It makes me nervous
but given how simple the ErrorVerifier impl is, maybe it won't be too bad.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2019: new
D<int>()./*error:IMPLICIT_DYNAMIC_METHOD*/m(d);
On 2016/06/23 23:25:52, Bob Nystrom wrote:
> This feels like an explicit use of dynamic to me since the type argument is
> inferred from something explicitly dynamic. What do you think?

yes, same as your other examples. They will either all work or all be errors,
depending on our treatment of inference-from-dynamic :)

[Leaf, 2016-06-24 21:40:26]

Per conversation offline, this lgtm to land as a --no-implicit-dynamic (or
something similar), with a --no-inferred-dynamic flag to come which only forbids
dynamic in positions where we might have inferred it.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:756:
_checkForImplicitDynamicReturn(node, node.element);
Will this fire on things that are setters only?  If so, is that what we want?

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:773:
_checkForImplicitDynamicReturn(node, node.element);
Good question.  :)  I think probably?  But this might be something we want to
get feedback on.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:3597:
variable.type.isDynamic) {
Can we trust that type is non-null here?

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:3662:
element.returnType.isDynamic) {
Can we trust returnType not to be null?

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1961: }
On 2016/06/24 01:15:56, John Messerly wrote:
> On 2016/06/23 23:25:52, Bob Nystrom wrote:
> > These test cases are fantastic. So thorough!
 
+1!

[Jennifer Messerly, 2016-06-27 18:38:17]

Thanks!

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:756:
_checkForImplicitDynamicReturn(node, node.element);
On 2016/06/24 21:40:25, Leaf wrote:
> Will this fire on things that are setters only?  If so, is that what we want?

There's a check inside `_checkForImplicitDynamicReturn` that it's not a setter.
It looks like there's a test case for it as well.

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:773:
_checkForImplicitDynamicReturn(node, node.element);
On 2016/06/24 21:40:25, Leaf wrote:
> Good question.  :)  I think probably?  But this might be something we want to
> get feedback on.

Sounds good. Based on our chat I'm going to leave this out for now. Keeps the
flag a bit more friendly, and those lambdas should usually be okay regardless,
given that we have errors in other places (generic methods, variable
declarations).

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:3597:
variable.type.isDynamic) {
On 2016/06/24 21:40:26, Leaf wrote:
> Can we trust that type is non-null here?

As near as I can tell, yes. An Element should always have a non-null type after
resolution. Inside this file, it doesn't seem to check for an element's type
being `null`.

(Definitely AST child nodes can be null, due to errors during parse. Also within
the Resolver visitors things are still being filled in, so more care is needed.)

https://codereview.chromium.org/2093523002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:3662:
element.returnType.isDynamic) {
On 2016/06/24 21:40:25, Leaf wrote:
> Can we trust returnType not to be null?

I believe so, yes. Most of the methods in this file do not check for null
returnType.

(One place I do know it can happen: via tests. They sometimes build incomplete
Element models, that resolution would never make, then pass them to the APIs. I
have been fixing that in the tests when I encounter it. Invariants == good :) )

[Jennifer Messerly, 2016-06-27 19:30:22]

Description was changed from

==========
fix #25573, add option to disable implicit dynamic
==========

to

==========
fix #25573, add option to disable implicit dynamic

R=leafp@google.com

Committed:
https://github.com/dart-lang/sdk/commit/844c82b5f3e478a52460b51550f06462607c8c74
==========

[Jennifer Messerly, 2016-06-27 19:30:23]

Committed patchset #3 (id:40001) manually as
844c82b5f3e478a52460b51550f06462607c8c74 (presubmit successful).