Refactor strong mode to use standard Analyzer errors

Refactor strong mode to use standard Analyzer errors

Also changes implicit casts errors to be determined by the type system isAssignableTo code, and updates our tests to show the user-facing error levels (StaticTypeWarningCode is upgraded to errors as far as users can see, but this wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583

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

Committed: https://github.com/dart-lang/sdk/commit/831c875484fe5e6ce25b50b93b99b4076df63285

Reverted & then re-landed in: https://codereview.chromium.org/2062793003/

[Jennifer Messerly, 2016-06-10 22:20:35]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2016-06-10 22:24:09]

Description was changed from

==========
refactor strong mode to use standard Analyzer errors
also changes implicit casts errors to be determined by the type system
isAssignableTo code
found while looking at #26583
==========

to

==========
Refactor strong mode to use standard Analyzer errors

Also changes implicit casts errors to be determined by the type system
isAssignableTo code, and updates our tests to show the user-facing error levels
(StaticTypeWarningCode is upgraded to errors as far as users can see, but this
wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583
==========

[Jennifer Messerly, 2016-06-10 22:24:09]

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

[Jennifer Messerly, 2016-06-10 22:28:41]

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

https://codereview.chromium.org/2060013002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:1905: 'num n; int i =
/*info:ASSIGNMENT_CAST,severe:INVALID_ASSIGNMENT*/n;');
Note the slight change here, compared with the change I landed earlier today.

Because we handle this in type system, we get both a hint and warning. No one
can turn on strongModeHints yet, except via AnalysisOptionsImpl, so it's not a
user-visible issue at this time.

Once we can integrate our implicit cast marking into Analyzer we can get rid of
it.

[Jennifer Messerly, 2016-06-10 22:29:33]

Aside: I would also like to fix "severe" to say "error" and "info" to say "hint"
in the checker_test. As a follow up if that sounds good to y'all?

[Jennifer Messerly, 2016-06-10 22:59:24]

https://codereview.chromium.org/2060013002/diff/60001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/ast_properties.dart (right):

https://codereview.chromium.org/2060013002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/ast_properties.dart:20: bool
hasImplicitCasts(CompilationUnit node) {
testing out the DDC side, I realized this short-circuit feature was
unimplemented. :)

I can either rip it out or fix it. Currently working towards the latter.

[Jennifer Messerly, 2016-06-10 23:13:12]

https://codereview.chromium.org/2060013002/diff/60001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/ast_properties.dart (right):

https://codereview.chromium.org/2060013002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/ast_properties.dart:20: bool
hasImplicitCasts(CompilationUnit node) {
On 2016/06/10 22:59:24, John Messerly wrote:
> testing out the DDC side, I realized this short-circuit feature was
> unimplemented. :)
> 
> I can either rip it out or fix it. Currently working towards the latter.

FYI -- DDC side is passing now. That CL is here:
https://codereview.chromium.org/2057373003

[Brian Wilkerson, 2016-06-11 00:08:33]

Nice! LGTM

> Aside: I would also like to fix "severe" to say "error" and "info" to say
> "hint" in the checker_test. As a follow up if that sounds good to y'all?

sgtm

[Leaf, 2016-06-11 00:24:39]

lgtm! thanks for cleaning all this up!

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:308: }
I think this (and the next check) can be moved under the implicitCasts flag. 
Probably doesn't make much difference but doesn't hurt to skip some is checks
when possible.

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/dart_test.dart (left):

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/dart_test.dart:5094:
expect(DynamicInvoke.get(idx.target), isNotNull);
Yep, still not null.  :)

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

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:272:
foo(/*info:INFERRED_TYPE_CLOSURE*/(x) { return null; });
Why did this change (probably not bad, just curious)?  I think it used to be
duplicated because there were two events here - the parameter and the return
type.  Maybe these get automatically de-duped by the analyzer now?

[Jennifer Messerly, 2016-06-11 00:40:38]

Thanks guys. I will have a look at making the changes suggested + landing on
Monday. Cheers & hope y'all have a good weekend.

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:308: }
On 2016/06/11 00:24:39, Leaf wrote:
> I think this (and the next check) can be moved under the implicitCasts flag. 
> Probably doesn't make much difference but doesn't hurt to skip some is checks
> when possible.

Ah yes, great idea!

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/dart_test.dart (left):

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/dart_test.dart:5094:
expect(DynamicInvoke.get(idx.target), isNotNull);
On 2016/06/11 00:24:39, Leaf wrote:
> Yep, still not null.  :)

Haha, yeah, not sure what that was supposed to be doing :)

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

https://codereview.chromium.org/2060013002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:272:
foo(/*info:INFERRED_TYPE_CLOSURE*/(x) { return null; });
On 2016/06/11 00:24:39, Leaf wrote:
> Why did this change (probably not bad, just curious)?  I think it used to be
> duplicated because there were two events here - the parameter and the return
> type.  Maybe these get automatically de-duped by the analyzer now? 

yup exactly. Unless ErrorCodes were identical, we didn't get any de-duping
(operator == of AnalysisError:       if (!identical(errorCode, other.errorCode))
{ return false; })

Now we get deduping if the message+span is the same. Like if we infer `dynamic`
for both the parameter and return type.

[Jennifer Messerly, 2016-06-13 16:54:23]

Description was changed from

==========
Refactor strong mode to use standard Analyzer errors

Also changes implicit casts errors to be determined by the type system
isAssignableTo code, and updates our tests to show the user-facing error levels
(StaticTypeWarningCode is upgraded to errors as far as users can see, but this
wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583
==========

to

==========
Refactor strong mode to use standard Analyzer errors

Also changes implicit casts errors to be determined by the type system
isAssignableTo code, and updates our tests to show the user-facing error levels
(StaticTypeWarningCode is upgraded to errors as far as users can see, but this
wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583

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

Committed:
https://github.com/dart-lang/sdk/commit/831c875484fe5e6ce25b50b93b99b4076df63285
==========

[Jennifer Messerly, 2016-06-13 16:54:25]

Committed patchset #4 (id:80001) manually as
831c875484fe5e6ce25b50b93b99b4076df63285 (presubmit successful).

[Jennifer Messerly, 2016-06-13 17:56:24]

Description was changed from

==========
Refactor strong mode to use standard Analyzer errors

Also changes implicit casts errors to be determined by the type system
isAssignableTo code, and updates our tests to show the user-facing error levels
(StaticTypeWarningCode is upgraded to errors as far as users can see, but this
wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583

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

Committed:
https://github.com/dart-lang/sdk/commit/831c875484fe5e6ce25b50b93b99b4076df63285
==========

to

==========
Refactor strong mode to use standard Analyzer errors

Also changes implicit casts errors to be determined by the type system
isAssignableTo code, and updates our tests to show the user-facing error levels
(StaticTypeWarningCode is upgraded to errors as far as users can see, but this
wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583

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

Committed:
https://github.com/dart-lang/sdk/commit/831c875484fe5e6ce25b50b93b99b4076df63285

Reverted & then re-landed in: https://codereview.chromium.org/2062793003/
==========