Don't report redundant type errors in strong mode.

Don't report redundant type errors in strong mode.

Currently, "sideways casts" -- type errors where one type is assigned
to an unrelated type -- are reported by both ErrorVerifier and strong
mode's Checker. This leads to duplicate errors that the user can see.

ErrorVerifier's errors are generally better: they give the user more
contextual information and are easier to read. So this CL eliminates
Checker's reporting of these errors and only uses ErrorVerifier's.

However, in strong mode, type errors like this are fatal: DDC can't
generate correct code. So this also automatically upgrades all static
type warnings to errors when strong mode is enabled.

R=brianwilkerson@google.com, leafp@google.com

Committed: https://github.com/dart-lang/sdk/commit/2ef00b0c3d0182b5e4ea5ca55fd00b9d038ae40d

[Bob Nystrom, 2016-03-09 23:10:28]

rnystrom@google.com changed reviewers:
+ brianwilkerson@google.com, leafp@google.com

[Bob Nystrom, 2016-03-09 23:10:28]

I think this gets rid of the main chunk of duplicate error reporting. I'm
guessing there's still some overlap around method overrides, but this mostly
eliminates the not-very-user-friendly "Type check failed: {0} is not of type
{1}" errors from strong mode.

Leaf, the most interesting stuff to look at here is actually the test changes.
Those show how the errors are different now. I think the ErrorVerifier warnings
should correctly cover the cases that used to be severe:STATIC_TYPE_ERROR, but
let me know if I missed something.

Thanks!

[Brian Wilkerson, 2016-03-10 00:09:34]

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/conte...
File pkg/analyzer/test/src/context/mock_sdk.dart (right):

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/conte...
pkg/analyzer/test/src/context/mock_sdk.dart:155: List();
What's the motivation for these changes? Both List and Map are abstract in the
SDK, so it seems like they ought to be that way in the mock SDK.

[Bob Nystrom, 2016-03-10 00:40:55]

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/conte...
File pkg/analyzer/test/src/context/mock_sdk.dart (right):

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/conte...
pkg/analyzer/test/src/context/mock_sdk.dart:155: List();
On 2016/03/10 00:09:33, Brian Wilkerson wrote:
> What's the motivation for these changes? Both List and Map are abstract in the
> SDK, so it seems like they ought to be that way in the mock SDK.

strong_mode_test uses `new List()` in a few tests. Before this CL, that was fine
since the strong mode tests didn't validate any non-strong mode errors at all.
Now that they do, it was producing spurious "can't construct an abstract class"
warnings. I figured the simple solution was to add the constructor here, but I
can do something different if you'd like.

[Brian Wilkerson, 2016-03-10 00:44:22]

LGTM

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/conte...
File pkg/analyzer/test/src/context/mock_sdk.dart (right):

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/conte...
pkg/analyzer/test/src/context/mock_sdk.dart:155: List();
I believe that we've been reproducing the SDK exactly except that it's a subset.
That would require adding a factory constructor, and a fake private
implementation. Not sure which is better. I'll leave it to your discretion.

[Leaf, 2016-03-10 22:58:25]

I have a couple of questions about the changes in the error codes.

Also, I presume that all of the errors that are changing from severe to warning
would be promoted to errors in the final user visible output?  Or are we
downgrading those to warnings?

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

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:120:
/*info:DYNAMIC_INVOKE*/f.col(/*info:ARGUMENT_TYPE_NOT_ASSIGNABLE*/3);
Interesting.  I guess this is the propagated type info kicking in.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1344: mOfOs =
/*info:DOWN_CAST_IMPLICIT, info:INVALID_ASSIGNMENT*/lOfDs;
Why did these show up?  I think this is just an implicit downcast for us.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1408: static const int
/*severe:VARIABLE_TYPE_MISMATCH*/i = /*info:ASSIGNMENT_CAST*/n;
What is this about?  I don't think this is supposed to be an error - just a
cast?

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2898: yield
/*warning:YIELD_OF_INVALID_TYPE*/new
/*warning:NEW_WITH_ABSTRACT_CLASS*/Iterable<int>();
I think the extra error here is just noise, the test should have a valid thing
of type Iterable<int> here.  Maybe just yield bar3()?

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2905: yield* new
/*warning:NEW_WITH_ABSTRACT_CLASS*/Iterable<int>();
ditto.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2908: yield*
/*info:INFERRED_TYPE_ALLOCATION*/new
/*warning:NEW_WITH_ABSTRACT_CLASS*/Iterable();
and again.

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

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:472: String y =
/*info:DYNAMIC_CAST,info:INVALID_ASSIGNMENT*/x;
Again, what is this?  Or is this a propagated type thing?

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1430:
/*severe:MAP_KEY_TYPE_NOT_ASSIGNABLE,warning:MAP_KEY_TYPE_NOT_ASSIGNABLE*/"hello":
Why two errors here?

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1826:
takeOON(/*warning:DOWN_CAST_COMPOSITE,warning:ARGUMENT_TYPE_NOT_ASSIGNABLE*/new
C().m);
It seems like something changed here.   I think this is changing this from just
a down cast warning to an actual error.  Similarly below.

[Bob Nystrom, 2016-03-15 00:08:01]

On 2016/03/10 22:58:25, Leaf wrote:
> I have a couple of questions about the changes in the error codes.
> 
> Also, I presume that all of the errors that are changing from severe to
warning
> would be promoted to errors in the final user visible output?  Or are we
> downgrading those to warnings?

That's right. It's handled by ErrorProcessor, which analyzer_cli and
analysis_server go through. Our tests go straight to ErrorVerifier, so you see
the unpromoted severity. If you try running this patch on some actual code in
the command-line analyzer, they will come through as errors.

[Bob Nystrom, 2016-03-15 00:08:13]

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

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:120:
/*info:DYNAMIC_INVOKE*/f.col(/*info:ARGUMENT_TYPE_NOT_ASSIGNABLE*/3);
On 2016/03/10 22:58:25, Leaf wrote:
> Interesting.  I guess this is the propagated type info kicking in.

Yeah, I think so. That's why it's an info and not a warning. Before this patch,
we weren't running ErrorVerifier at all in this test, so I'm guessing this hint
was there before too.

Added a comment.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1344: mOfOs =
/*info:DOWN_CAST_IMPLICIT, info:INVALID_ASSIGNMENT*/lOfDs;
On 2016/03/10 22:58:25, Leaf wrote:
> Why did these show up?  I think this is just an implicit downcast for us.

I believe it's a hint coming from type propagation at:

lOfDs = lOfAs;

It knows lOfDs is actually lOfAs, so you get the same warning as on line 1346,
though downgraded to a hint. Want me to rearrange the code to nullify the type
propagation?

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1408: static const int
/*severe:VARIABLE_TYPE_MISMATCH*/i = /*info:ASSIGNMENT_CAST*/n;
On 2016/03/10 22:58:25, Leaf wrote:
> What is this about?  I don't think this is supposed to be an error - just a
> cast?   

n and i are const, so analyzer knows the concrete value is the wrong type.
(That's also why this is severe, not warning.) Added a comment.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2898: yield
/*warning:YIELD_OF_INVALID_TYPE*/new
/*warning:NEW_WITH_ABSTRACT_CLASS*/Iterable<int>();
On 2016/03/10 22:58:25, Leaf wrote:
> I think the extra error here is just noise, the test should have a valid thing
> of type Iterable<int> here.  Maybe just yield bar3()?

Ah, great idea. I didn't want to change it to List because I thought it might be
useful for the test to validate it works with any iterable.

Done.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2905: yield* new
/*warning:NEW_WITH_ABSTRACT_CLASS*/Iterable<int>();
On 2016/03/10 22:58:25, Leaf wrote:
> ditto.

Done.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:2908: yield*
/*info:INFERRED_TYPE_ALLOCATION*/new
/*warning:NEW_WITH_ABSTRACT_CLASS*/Iterable();
On 2016/03/10 22:58:25, Leaf wrote:
> and again.

Changed this to new List() since we want to test inferring the type argument.

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

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:472: String y =
/*info:DYNAMIC_CAST,info:INVALID_ASSIGNMENT*/x;
On 2016/03/10 22:58:25, Leaf wrote:
> Again, what is this?  Or is this a propagated type thing?

Yeah, looks like it. Added a comment.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1430:
/*severe:MAP_KEY_TYPE_NOT_ASSIGNABLE,warning:MAP_KEY_TYPE_NOT_ASSIGNABLE*/"hello":
On 2016/03/10 22:58:25, Leaf wrote:
> Why two errors here?

The names are not very helpful here because they have the same name. The warning
is the type error you expect to see. The severe one is the more precise compile
time error coming from the constant evaluator.

Added a comment.

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1826:
takeOON(/*warning:DOWN_CAST_COMPOSITE,warning:ARGUMENT_TYPE_NOT_ASSIGNABLE*/new
C().m);
On 2016/03/10 22:58:25, Leaf wrote:
> It seems like something changed here.   I think this is changing this from
just
> a down cast warning to an actual error.  Similarly below.

I just checked and I get the same warning without my change too. It's just that
before my patch, no ErrorVerifier warnings were generated or validated by these
tests.

It does seem wrong to me too, but maybe not any wronger because of this patch?

[Leaf, 2016-03-15 00:28:04]

LGTM, thanks!

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

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1344: mOfOs =
/*info:DOWN_CAST_IMPLICIT, info:INVALID_ASSIGNMENT*/lOfDs;
On 2016/03/15 00:08:13, Bob Nystrom wrote:
> On 2016/03/10 22:58:25, Leaf wrote:
> > Why did these show up?  I think this is just an implicit downcast for us.
> 
> I believe it's a hint coming from type propagation at:
> 
> lOfDs = lOfAs;
> 
> It knows lOfDs is actually lOfAs, so you get the same warning as on line 1346,
> though downgraded to a hint. Want me to rearrange the code to nullify the type
> propagation?

I see.  Not a big deal, but it might be better to null the assigned variable at
the end of each block since this is essentially just noise for the purposes of
the test.

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

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1826:
takeOON(/*warning:DOWN_CAST_COMPOSITE,warning:ARGUMENT_TYPE_NOT_ASSIGNABLE*/new
C().m);
On 2016/03/15 00:08:13, Bob Nystrom wrote:
> On 2016/03/10 22:58:25, Leaf wrote:
> > It seems like something changed here.   I think this is changing this from
> just
> > a down cast warning to an actual error.  Similarly below.
> 
> I just checked and I get the same warning without my change too. It's just
that
> before my patch, no ErrorVerifier warnings were generated or validated by
these
> tests.
> 
> It does seem wrong to me too, but maybe not any wronger because of this patch?

Ah, I see.  We're failing to do the implicit instantiation here.  A bug I think,
but not new.  ok.

[Bob Nystrom, 2016-03-15 19:14:14]

Description was changed from

==========
Don't report redundant type errors in strong mode.

Currently, "sideways casts" -- type errors where one type is assigned
to an unrelated type -- are reported by both ErrorVerifier and strong
mode's Checker. This leads to duplicate errors that the user can see.

ErrorVerifier's errors are generally better: they give the user more
contextual information and are easier to read. So this CL eliminates
Checker's reporting of these errors and only uses ErrorVerifier's.

However, in strong mode, type errors like this are fatal: DDC can't
generate correct code. So this also automatically upgrades all static
type warnings to errors when strong mode is enabled.
==========

to

==========
Don't report redundant type errors in strong mode.

Currently, "sideways casts" -- type errors where one type is assigned
to an unrelated type -- are reported by both ErrorVerifier and strong
mode's Checker. This leads to duplicate errors that the user can see.

ErrorVerifier's errors are generally better: they give the user more
contextual information and are easier to read. So this CL eliminates
Checker's reporting of these errors and only uses ErrorVerifier's.

However, in strong mode, type errors like this are fatal: DDC can't
generate correct code. So this also automatically upgrades all static
type warnings to errors when strong mode is enabled.

R=brianwilkerson@google.com, leafp@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2ef00b0c3d0182b5e4ea5ca55fd00b9d038ae40d
==========

[Bob Nystrom, 2016-03-15 19:14:16]

Committed patchset #3 (id:40001) manually as
2ef00b0c3d0182b5e4ea5ca55fd00b9d038ae40d (presubmit successful).

[Bob Nystrom, 2016-03-15 19:14:34]

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

https://codereview.chromium.org/1780783002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:1344: mOfOs =
/*info:DOWN_CAST_IMPLICIT, info:INVALID_ASSIGNMENT*/lOfDs;
On 2016/03/15 00:28:04, Leaf wrote:
> On 2016/03/15 00:08:13, Bob Nystrom wrote:
> > On 2016/03/10 22:58:25, Leaf wrote:
> > > Why did these show up?  I think this is just an implicit downcast for us.
> > 
> > I believe it's a hint coming from type propagation at:
> > 
> > lOfDs = lOfAs;
> > 
> > It knows lOfDs is actually lOfAs, so you get the same warning as on line
1346,
> > though downgraded to a hint. Want me to rearrange the code to nullify the
type
> > propagation?
> 
> I see.  Not a big deal, but it might be better to null the assigned variable
at
> the end of each block since this is essentially just noise for the purposes of
> the test.

Done.