Avoid issuing incorrect errors when super mixins are enabled

Avoid issuing incorrect errors when super mixins are enabled

BUG=
R=jmesserly@google.com

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

[Leaf, 2017-07-27 20:46:22]

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

[Leaf, 2017-07-27 20:46:23]

Here's my attempt to fix https://github.com/flutter/flutter/issues/11415 .  This
does *not* implement proper strong mode checking for super mixins - it just
tries to avoid erroneously rejecting some valid super mixin patterns while still
correctly rejecting bad code when super mixins are off.

[Jennifer Messerly, 2017-07-27 21:07:59]

the fix LGTM, there may be some additional problems though, because the "visit
concrete members" pattern is used in a few places. (it would be so great to
consolidate this somehow... the Element model data structures aren't ideal for
override checking =\ )

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

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1409: /// For each member of this
class and non-overridden inherited member, we
fyi: this function is also doing a "concrete member" walk ... so it might have a
bug related to super mixins

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1450:
element.mixins.reversed.forEach(visitSuper);
this could be problematic

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1743: for (var s in
_getSuperclasses(type, (t) => t.element.isAbstract)) {
based on comments below, this call site may need to change

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1928: var superclasses =
_getSuperclasses(element.type);
this set of concrete superclasses also contains mixins recursively.

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1931: if
(_checkConcreteMemberOverride(member, s, loc)) break;
in particular, we'll break out of the loop once we hit a concrete member.

I think with super mixins, we may hit an incorrect member (a member that is not
actually concrete)

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1947:
t.mixins.reversed.forEach(visit);
this may be the problematic location

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

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:3173:
test_mixinApplicationIsConcrete() async {
style thing: existing tests in this file often use async/await needlessly.

I've been fixing tests where possible to be:

    test_mixinApplicationIsConcrete() {
      return checkFile(...);
    }

because checkFile return a Future, there's not really a need for async/await in
most tests. (also "checkFile" does "addFile" and "check" in one step)

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:3934:
test_superMixin_invalidApplication() async {
(same comment from above would apply to these tests too)

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:3935: //Failing:
https://github.com/dart-lang/sdk/issues/30283
trivia nit: space after "//"

[Leaf, 2017-07-27 21:38:09]

Fixed the style issues.

I'll land this fix and then look into the potentially still missing things that
you pointed out.

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

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:3173:
test_mixinApplicationIsConcrete() async {
On 2017/07/27 21:07:58, Jennifer Messerly wrote:
> style thing: existing tests in this file often use async/await needlessly.
> 
> I've been fixing tests where possible to be:
> 
>     test_mixinApplicationIsConcrete() {
>       return checkFile(...);
>     }
> 
> because checkFile return a Future, there's not really a need for async/await
in
> most tests. (also "checkFile" does "addFile" and "check" in one step)
> 

Done.

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:3934:
test_superMixin_invalidApplication() async {
On 2017/07/27 21:07:58, Jennifer Messerly wrote:
> (same comment from above would apply to these tests too)

Done.

https://codereview.chromium.org/2986063002/diff/60001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:3935: //Failing:
https://github.com/dart-lang/sdk/issues/30283
On 2017/07/27 21:07:58, Jennifer Messerly wrote:
> trivia nit: space after "//"

Done.

[Leaf, 2017-07-27 21:39:04]

Description was changed from

==========
Avoid issuing incorrect errors when super mixins are enabled

BUG=
==========

to

==========
Avoid issuing incorrect errors when super mixins are enabled

BUG=
R=jmesserly@google.com

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

[Leaf, 2017-07-27 21:39:05]

Committed patchset #5 (id:80001) manually as
bbb0ed27ad34d6f35d90415feb68e55cdded4d73 (presubmit successful).