Start separating ClassElementImpl for Class and Enum.

Start separating ClassElementImpl for Class and Enum.

Maybe they should be named differently though, like ClassElementImpl
and EnumElementImpl.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2016-06-01 14:08:30]

It isn't clear to me why we're making a distinction between classes and enums,
so I'd like to get some background off-line.

> Maybe they should be named differently though, like ClassElementImpl and
EnumElementImpl.

Yeah, I don't like the current names, but I'm concerned about the implications
of renaming ClassElementImpl.

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

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:65: abstract class
ClassElementImpl extends ElementImpl
I'll just note that this is a breaking change. I don't know whether we have any
clients that are creating class elements (I hope not), but if so this will break
them.

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1197: super.abstract =
isAbstract;
Why isn't this an error?

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1239: assert(false);
Do we really want to only throw in checked mode?

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:7644: var zzz = typeParameters;
nit: Perhaps a better name and an explicit type?

[scheglov, 2016-06-01 15:29:26]

A different approach: https://codereview.chromium.org/2029053002

On 2016/06/01 14:08:30, Brian Wilkerson wrote:
> It isn't clear to me why we're making a distinction between classes and enums,
> so I'd like to get some background off-line.
> 
> > Maybe they should be named differently though, like ClassElementImpl and
> EnumElementImpl.
> 
> Yeah, I don't like the current names, but I'm concerned about the implications
> of renaming ClassElementImpl.
> 
>
https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
> File pkg/analyzer/lib/src/dart/element/element.dart (right):
> 
>
https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
> pkg/analyzer/lib/src/dart/element/element.dart:65: abstract class
> ClassElementImpl extends ElementImpl
> I'll just note that this is a breaking change. I don't know whether we have
any
> clients that are creating class elements (I hope not), but if so this will
break
> them.
> 
>
https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
> pkg/analyzer/lib/src/dart/element/element.dart:1197: super.abstract =
> isAbstract;
> Why isn't this an error?
> 
>
https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
> pkg/analyzer/lib/src/dart/element/element.dart:1239: assert(false);
> Do we really want to only throw in checked mode?
> 
>
https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
> pkg/analyzer/lib/src/dart/element/element.dart:7644: var zzz = typeParameters;
> nit: Perhaps a better name and an explicit type?

[scheglov, 2016-06-01 17:25:28]

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

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1197: super.abstract =
isAbstract;
On 2016/06/01 14:08:30, Brian Wilkerson wrote:
> Why isn't this an error?

Fixed.

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1239: assert(false);
On 2016/06/01 14:08:30, Brian Wilkerson wrote:
> Do we really want to only throw in checked mode?

Well, this is what we do everywhere.

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:7644: var zzz = typeParameters;
On 2016/06/01 14:08:30, Brian Wilkerson wrote:
> nit: Perhaps a better name and an explicit type?

Sorry, forgot to inline after debugging.
Fixed.

[Brian Wilkerson, 2016-06-01 17:29:44]

After off-line discussions, lgtm.

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

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:339: return
hasModifier(Modifier.MIXIN_APPLICATION);
Seems like this (and several other members) ought to just get copied down to
ClassElementImpl_Class. There's no value in having those members call 'super'
when the superclass implementation won't be used by ClassElementImpl_Enum.

[Paul Berry, 2016-06-01 17:33:14]

lgtm

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

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:65: abstract class
ClassElementImpl extends ElementImpl
On 2016/06/01 14:08:30, Brian Wilkerson wrote:
> I'll just note that this is a breaking change. I don't know whether we have
any
> clients that are creating class elements (I hope not), but if so this will
break
> them.

As discussed in person, I'm ok with making this change.  IMHO it's a stretch to
call it a breaking change at all--the class is called "...Impl" and is inside
"src/", so any client that is broken by this change was already breaking pub
conventions and accessing analyzer internals.

https://codereview.chromium.org/2027893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:139:
ClassElementImpl.forNode(Identifier name) : super.forNode(name);
As a side note, we could reduce the impact of this change by changing this
constructor into a factory constructor that forwards to ClassElementImpl_Class
or ClassElementImpl_Enum as appropriate.

Your call, though.

[scheglov, 2016-06-01 17:38:42]

Description was changed from

==========
Start separating ClassElementImpl for Class and Enum.

Maybe they should be named differently though, like ClassElementImpl
and EnumElementImpl.

R=brianwilkerson@google.com, paulberry@google.com
BUG=
==========

to

==========
Start separating ClassElementImpl for Class and Enum.

Maybe they should be named differently though, like ClassElementImpl
and EnumElementImpl.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[scheglov, 2016-06-01 17:38:43]

Committed patchset #2 (id:20001) manually as
be990cd13490bea5d8c272b518b744cfc56886d1 (presubmit successful).