Stop precomputing ClassElement.isValidMixin.

Stop precomputing ClassElement.isValidMixin.

Previously, we would precompute the value of ClassElement.isValidMixin
and store it in the Modifier.MIXIN.  But this didn't really save any
time, because (a) isValidMixin isn't used inside analyzer, and (b) it
is easily derivable from other information already present in the
element model.

Also, we were computing it incorrectly when the `--supermixin` flag
was present.

This CL replaces the precomputation logic with a direct implementation
in ClassElement.isValidMixin which is correct regardless of the
presence of the `--supermixin` flag.  The tests are beefed up so that
(a) they validate correct behavior for both states of the flag, (b)
they validate correct behavior in the presence of factory
constructors, and (c) they validate that the behavior of
`isValidMixin` is consistent with the error messages produced by the
analyzer.

In addition to fixing bugs, the removal of Modifier.MIXIN means we
have one less piece of information to store in summaries.

R=brianwilkerson@google.com

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

[Paul Berry, 2016-01-26 13:48:49]

paulberry@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2016-01-26 15:03:21]

LGTM

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/element.dart (left):

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/element.dart:1553: static const Modifier MIXIN =
const Modifier('MIXIN', 12);
Technically, this is a breaking change. I'm guessing, though, that it isn't a
problem in practice.

[Paul Berry, 2016-01-26 15:27:10]

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/element.dart (left):

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/element.dart:1553: static const Modifier MIXIN =
const Modifier('MIXIN', 12);
On 2016/01/26 15:03:21, Brian Wilkerson wrote:
> Technically, this is a breaking change. I'm guessing, though, that it isn't a
> problem in practice.

Agreed.  How would you feel if I submitted a follow-up CL that moved the class
Modifier into lib/src, so that it's no longer exposed as part of the public
interface?  AFAICT, all references to it are in lib/src, so I think it makes
sense for it to live in lib/src as well.

[Paul Berry, 2016-01-26 15:29:52]

Description was changed from

==========
Stop precomputing ClassElement.isValidMixin.

Previously, we would precompute the value of ClassElement.isValidMixin
and store it in the Modifier.MIXIN.  But this didn't really save any
time, because (a) isValidMixin isn't used inside analyzer, and (b) it
is easily derivable from other information already present in the
element model.

Also, we were computing it incorrectly when the `--supermixin` flag
was present.

This CL replaces the precomputation logic with a direct implementation
in ClassElement.isValidMixin which is correct regardless of the
presence of the `--supermixin` flag.  The tests are beefed up so that
(a) they validate correct behavior for both states of the flag, (b)
they validate correct behavior in the presence of factory
constructors, and (c) they validate that the behavior of
`isValidMixin` is consistent with the error messages produced by the
analyzer.

In addition to fixing bugs, the removal of Modifier.MIXIN means we
have one less piece of information to store in summaries.
==========

to

==========
Stop precomputing ClassElement.isValidMixin.

Previously, we would precompute the value of ClassElement.isValidMixin
and store it in the Modifier.MIXIN.  But this didn't really save any
time, because (a) isValidMixin isn't used inside analyzer, and (b) it
is easily derivable from other information already present in the
element model.

Also, we were computing it incorrectly when the `--supermixin` flag
was present.

This CL replaces the precomputation logic with a direct implementation
in ClassElement.isValidMixin which is correct regardless of the
presence of the `--supermixin` flag.  The tests are beefed up so that
(a) they validate correct behavior for both states of the flag, (b)
they validate correct behavior in the presence of factory
constructors, and (c) they validate that the behavior of
`isValidMixin` is consistent with the error messages produced by the
analyzer.

In addition to fixing bugs, the removal of Modifier.MIXIN means we
have one less piece of information to store in summaries.

R=brianwilkerson@google.com

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

[Paul Berry, 2016-01-26 15:29:54]

Committed patchset #1 (id:1) manually as
4e86a006db963f3e335734202abcc02a2ea7e705 (presubmit successful).

[Brian Wilkerson, 2016-01-26 15:33:50]

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/element.dart (left):

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/element.dart:1553: static const Modifier MIXIN =
const Modifier('MIXIN', 12);
> How would you feel if I submitted a follow-up CL that moved the class
> Modifier into lib/src ...

I wondered, when I reviewed this CL, why I had included it in the public API in
the first place. I need to take a closer look, but I suspect that would be a
good plan. I'd also be happy to move it so you don't get distracted.

[Paul Berry, 2016-01-26 16:35:09]

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/element.dart (left):

https://codereview.chromium.org/1635063003/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/element.dart:1553: static const Modifier MIXIN =
const Modifier('MIXIN', 12);
On 2016/01/26 15:33:50, Brian Wilkerson wrote:
> > How would you feel if I submitted a follow-up CL that moved the class
> > Modifier into lib/src ...
> 
> I wondered, when I reviewed this CL, why I had included it in the public API
in
> the first place. I need to take a closer look, but I suspect that would be a
> good plan. I'd also be happy to move it so you don't get distracted.

Ok, thanks!  I'll leave the ball in your court, then.