Introduce DartType super hierarchy based on Entity

Introduce DartType super hierarchy based on Entity

R=sigmund@google.com

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

[Johnni Winther, 2016-12-22 12:35:36]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-12-22 15:42:03]

lgtm, but let's at least do the first 2 suggestions below.

https://codereview.chromium.org/2598983002/diff/20001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/types.dart (right):

https://codereview.chromium.org/2598983002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/types.dart:7: abstract class DartType {
mmmm... I wonder if we should pick a different name here. We can always use
prefixes to disambiguate, but since we already have 2 type hierarchies
(dart_type and kernel) with similar classes (e.g. InterfaceType is defined in
both) I wonder if 3 would get a bit too confusing?

I don't yet have good suggestions for a different name, but some things that
might help:
  - add a dartdoc to this library and dart_type explaining the difference
between the two.
  - add some TODOs to clarify the direction these interfaces are headed into,
for example:
      - e.g. the compiler will depend on entities + type entities
      - dart_type will become just an implementation type

Other ideas:
  - maybe rename dart_type.dart to something else? elements/resolved_types.dart?
  - create lib/src/entitites/ with entities.dart and types.dart?
  - since the distinction between entity and element is very subtle, maybe
rename entity => element, and element => resolvedelement ? More precisely, 
      ClassEntity                   => ClassElement
      element_types.InterfaceType   => InterfaceType
      ClassElement                  => ResolvedClassElement (bc it results from
resolution)
      dart_types.InterfaceType      => ResolvedInterfaceType

  so in terms of libraries they would respectively be stored as:
      elements/elements.dart
      elements/types.dart
      elements/resolved_elements.dart or resolution/elements.dart
      elements/resolved_types.dart    or resolution/types.dart

  and 
     elements/modelx.dart  => elements/resolved_elements_impl.dart
                           or resolution/elements_impl.dart


Thoughts?

https://codereview.chromium.org/2598983002/diff/20001/tests/corelib/iterable_...
File tests/corelib/iterable_generate_test.dart (right):

https://codereview.chromium.org/2598983002/diff/20001/tests/corelib/iterable_...
tests/corelib/iterable_generate_test.dart:63: new
Iterable<Null>.generate(5).elementAt(2);
not sure I follow: why is this change needed?

[Johnni Winther, 2016-12-22 15:58:49]

https://codereview.chromium.org/2598983002/diff/20001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/types.dart (right):

https://codereview.chromium.org/2598983002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/types.dart:7: abstract class DartType {
On 2016/12/22 15:42:03, Siggi Cherem (dart-lang) wrote:
> mmmm... I wonder if we should pick a different name here. We can always use
> prefixes to disambiguate, but since we already have 2 type hierarchies
> (dart_type and kernel) with similar classes (e.g. InterfaceType is defined in
> both) I wonder if 3 would get a bit too confusing?
> 
> I don't yet have good suggestions for a different name, but some things that
> might help:
>   - add a dartdoc to this library and dart_type explaining the difference
> between the two.
>   - add some TODOs to clarify the direction these interfaces are headed into,
> for example:
>       - e.g. the compiler will depend on entities + type entities
>       - dart_type will become just an implementation type
> 
> Other ideas:
>   - maybe rename dart_type.dart to something else?
elements/resolved_types.dart?
>   - create lib/src/entitites/ with entities.dart and types.dart?
>   - since the distinction between entity and element is very subtle, maybe
> rename entity => element, and element => resolvedelement ? More precisely, 
>       ClassEntity                   => ClassElement
>       element_types.InterfaceType   => InterfaceType
>       ClassElement                  => ResolvedClassElement (bc it results
from
> resolution)
>       dart_types.InterfaceType      => ResolvedInterfaceType
> 
>   so in terms of libraries they would respectively be stored as:
>       elements/elements.dart
>       elements/types.dart
>       elements/resolved_elements.dart or resolution/elements.dart
>       elements/resolved_types.dart    or resolution/types.dart
> 
>   and 
>      elements/modelx.dart  => elements/resolved_elements_impl.dart
>                            or resolution/elements_impl.dart
> 
> 
> Thoughts?

Good ideas. I'll pause this CL and make the renaming in a separate CL.

https://codereview.chromium.org/2598983002/diff/20001/tests/corelib/iterable_...
File tests/corelib/iterable_generate_test.dart (right):

https://codereview.chromium.org/2598983002/diff/20001/tests/corelib/iterable_...
tests/corelib/iterable_generate_test.dart:63: new
Iterable<Null>.generate(5).elementAt(2);
On 2016/12/22 15:42:03, Siggi Cherem (dart-lang) wrote:
> not sure I follow: why is this change needed?

From a different CL. Removed.

[Johnni Winther, 2017-01-04 09:52:39]

https://codereview.chromium.org/2598983002/diff/20001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/types.dart (right):

https://codereview.chromium.org/2598983002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/types.dart:7: abstract class DartType {
On 2016/12/22 15:58:49, Johnni Winther wrote:
> On 2016/12/22 15:42:03, Siggi Cherem (dart-lang) wrote:
> > mmmm... I wonder if we should pick a different name here. We can always use
> > prefixes to disambiguate, but since we already have 2 type hierarchies
> > (dart_type and kernel) with similar classes (e.g. InterfaceType is defined
in
> > both) I wonder if 3 would get a bit too confusing?
> > 
> > I don't yet have good suggestions for a different name, but some things that
> > might help:
> >   - add a dartdoc to this library and dart_type explaining the difference
> > between the two.
> >   - add some TODOs to clarify the direction these interfaces are headed
into,
> > for example:
> >       - e.g. the compiler will depend on entities + type entities
> >       - dart_type will become just an implementation type
> > 
> > Other ideas:
> >   - maybe rename dart_type.dart to something else?
> elements/resolved_types.dart?
> >   - create lib/src/entitites/ with entities.dart and types.dart?
> >   - since the distinction between entity and element is very subtle, maybe
> > rename entity => element, and element => resolvedelement ? More precisely, 
> >       ClassEntity                   => ClassElement
> >       element_types.InterfaceType   => InterfaceType
> >       ClassElement                  => ResolvedClassElement (bc it results
> from
> > resolution)
> >       dart_types.InterfaceType      => ResolvedInterfaceType
> > 
> >   so in terms of libraries they would respectively be stored as:
> >       elements/elements.dart
> >       elements/types.dart
> >       elements/resolved_elements.dart or resolution/elements.dart
> >       elements/resolved_types.dart    or resolution/types.dart
> > 
> >   and 
> >      elements/modelx.dart  => elements/resolved_elements_impl.dart
> >                            or resolution/elements_impl.dart
> > 
> > 
> > Thoughts?
> 
> Good ideas. I'll pause this CL and make the renaming in a separate CL.

Dartdoc added.

[Johnni Winther, 2017-01-04 09:59:53]

Description was changed from

==========
Introduce DartType super hierarchy based on Entity
==========

to

==========
Introduce DartType super hierarchy based on Entity

R=sigmund@google.com

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

[Johnni Winther, 2017-01-04 09:59:54]

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