Resolve obvious deprecation warnings on bleeding edge

Resolve obvious deprecation warnings on bleeding edge

BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/67df07896e42263cd7af9e46e26c4827a9b3dd61

[Leaf, 2016-03-14 22:47:54]

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

[Leaf, 2016-03-14 22:47:56]

Several of the analyzer/lib/src libraries that we reach in and grab are
deprecated in bleeding edge in favor of the public APIs.  This CL eliminates the
obvious uses of these APIs, and marks the remaining uses with a TODO and an
"ignore" comment to make the build green.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:500: var fields = new
List<ConstFieldElementImpl>.from(
There is no ConstFieldElement.  Using FieldElement here seems to work.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:17: show DynamicElementImpl, DynamicTypeImpl,
LocalVariableElementImpl;
There's no DynamicElement, should there be?  

We should probably just be better at keeping around a type provider here to get
rid of DynamicTypeImpl.

LocalVariableElement is an issue - we currently extend it to support our own
temps.  We may need to have a discussion about how we're going to integrate with
the ast.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (left):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart#oldc...
lib/src/utils.dart:476: e.staticType ?? DynamicTypeImpl.instance;
We rely in places on being able to just get the canonincal instance of
DynamicType, which is not available from the DynamicType interface.  Probably we
should just be better about passing around a type provider.

[Brian Wilkerson, 2016-03-14 23:06:23]

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

[Brian Wilkerson, 2016-03-14 23:06:23]

DBC Another possibility would be to import the needed classes from their new
'src' location, possibly with a TODO to resolve the mismatch. (There's some
justification for claiming that a compiler back end should have more access to
the front end than most other clients.)

[Jennifer Messerly, 2016-03-14 23:12:42]

Yeah, +1 to what Brian said, I was wondering that too.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:500: var fields = new
List<ConstFieldElementImpl>.from(
On 2016/03/14 22:47:55, Leaf wrote:
> There is no ConstFieldElement.  Using FieldElement here seems to work.

seems to still be there?
https://github.com/dart-lang/sdk/blob/3a8bc819451290e1763ab46626809d1c469c8da...

But I'm fine with FieldElement.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:17: show DynamicElementImpl, DynamicTypeImpl,
LocalVariableElementImpl;
On 2016/03/14 22:47:55, Leaf wrote:
> There's no DynamicElement, should there be?  
> 
> We should probably just be better at keeping around a type provider here to
get
> rid of DynamicTypeImpl.

We use this in one place:

    // type literal
    if (element is ClassElement ||
        element is DynamicElementImpl ||
        element is FunctionTypeAliasElement) {

I believe all of that can be replaced with `element is TypeDefiningElement` ...
assuming that DynamicElementImpl implements it.


> LocalVariableElement is an issue - we currently extend it to support our own
> temps.  We may need to have a discussion about how we're going to integrate
with
> the ast.
 
That shouldn't be too hard to change. Assuming that `new LocalVariableElement`
works, we can make a synthetic one and put it in a HashSet.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/reify_c...
File lib/src/codegen/reify_coercions.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/reify_c...
lib/src/codegen/reify_coercions.dart:17: import '../utils.dart' show
getStaticType;
not sure if we want to try keeping this sorted (ish). Looks like "../" was
before the others. Doesn't matter to me

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (left):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart#oldc...
lib/src/utils.dart:476: e.staticType ?? DynamicTypeImpl.instance;
On 2016/03/14 22:47:55, Leaf wrote:
> We rely in places on being able to just get the canonincal instance of
> DynamicType, which is not available from the DynamicType interface.  Probably
we
> should just be better about passing around a type provider.

Is there a reason? I'd prefer to use the well known `dynamic`. It's painful to
pass around TypeProviders... (and TypeSystems ...)  Better, IMO, would be to
offer an easy way to get it.

Funny enough, you could do it already with something terrible like:

    e.getAncestor((a) => a is
CompilationUnit).element.library.context.typeProvider

... ouch :)

Brainstorming here a bit ... the problem really is filling in `null`. That's not
supposed to happen after resolution, AFAIK. (see
StaticTypeAnalyzer._recordStaticType), so this might be a bug we've been working
around unknowingly.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart#newc...
lib/src/utils.dart:25: //ignore: DEPRECATED_MEMBER_USE
oh, I was confused because it didn't look deprecated:
https://github.com/dart-lang/sdk/blob/3a8bc819451290e1763ab46626809d1c469c8da...

I guess you're saying it's non-public. Yeah, it seems good to move off the
non-public APIs if possible

[Jennifer Messerly, 2016-03-14 23:12:58]

oops and LGTM!

[Leaf, 2016-03-14 23:58:35]

On 2016/03/14 23:06:23, Brian Wilkerson wrote:
> DBC Another possibility would be to import the needed classes from their new
> 'src' location, possibly with a TODO to resolve the mismatch. (There's some
> justification for claiming that a compiler back end should have more access to
> the front end than most other clients.)

Yeah, I'm fine with this too.  I didn't want to silently switch over to reaching
into the new
lib/src locations without having this discussion, but if you both feel like
we're ok with the
current setup I'll switch them over.

[Leaf, 2016-03-14 23:58:42]

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:500: var fields = new
List<ConstFieldElementImpl>.from(
On 2016/03/14 23:12:42, John Messerly wrote:
> On 2016/03/14 22:47:55, Leaf wrote:
> > There is no ConstFieldElement.  Using FieldElement here seems to work.
> 
> seems to still be there?
>
https://github.com/dart-lang/sdk/blob/3a8bc819451290e1763ab46626809d1c469c8da...
> 
> But I'm fine with FieldElement.

There's a ConstFieldElementImpl (which is in the private deprecated API), but
there's no corresponding public ConstFieldElement.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:17: show DynamicElementImpl, DynamicTypeImpl,
LocalVariableElementImpl;
On 2016/03/14 23:12:42, John Messerly wrote:
> On 2016/03/14 22:47:55, Leaf wrote:
> > There's no DynamicElement, should there be?  
> > 
> > We should probably just be better at keeping around a type provider here to
> get
> > rid of DynamicTypeImpl.
> 
> We use this in one place:
> 
>     // type literal
>     if (element is ClassElement ||
>         element is DynamicElementImpl ||
>         element is FunctionTypeAliasElement) {
> 
> I believe all of that can be replaced with `element is TypeDefiningElement`
...
> assuming that DynamicElementImpl implements it.

It does.  But so does TypeParameterElement, which wouldn't currently be caught
by this check.  But that looks to me like it might be a bug?

> 
> 
> > LocalVariableElement is an issue - we currently extend it to support our own
> > temps.  We may need to have a discussion about how we're going to integrate
> with
> > the ast.
>  
> That shouldn't be too hard to change. Assuming that `new LocalVariableElement`
> works, we can make a synthetic one and put it in a HashSet.

Ok, good to know.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/reify_c...
File lib/src/codegen/reify_coercions.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/reify_c...
lib/src/codegen/reify_coercions.dart:17: import '../utils.dart' show
getStaticType;
On 2016/03/14 23:12:42, John Messerly wrote:
> not sure if we want to try keeping this sorted (ish). Looks like "../" was
> before the others. Doesn't matter to me

Done.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (left):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart#oldc...
lib/src/utils.dart:476: e.staticType ?? DynamicTypeImpl.instance;
On 2016/03/14 23:12:42, John Messerly wrote:
> On 2016/03/14 22:47:55, Leaf wrote:
> > We rely in places on being able to just get the canonincal instance of
> > DynamicType, which is not available from the DynamicType interface. 
Probably
> we
> > should just be better about passing around a type provider.
> 
> Is there a reason? I'd prefer to use the well known `dynamic`. It's painful to
> pass around TypeProviders... (and TypeSystems ...)  Better, IMO, would be to
> offer an easy way to get it.
> 
> Funny enough, you could do it already with something terrible like:
> 
>     e.getAncestor((a) => a is
> CompilationUnit).element.library.context.typeProvider
> 
> ... ouch :)
> 
> Brainstorming here a bit ... the problem really is filling in `null`. That's
not
> supposed to happen after resolution, AFAIK. (see
> StaticTypeAnalyzer._recordStaticType), so this might be a bug we've been
working
> around unknowingly.
> 
> 

I think it's just a question about the analyzer API design - should the public
API commit to the canonical instance being available statically, or does it want
to reserve the right to implement it in some other way?

We should probably look in and see if we actually need this function at some
point.  Added a TODO.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart#newc...
lib/src/utils.dart:25: //ignore: DEPRECATED_MEMBER_USE
On 2016/03/14 23:12:42, John Messerly wrote:
> oh, I was confused because it didn't look deprecated:
>
https://github.com/dart-lang/sdk/blob/3a8bc819451290e1763ab46626809d1c469c8da...
> 
> I guess you're saying it's non-public. Yeah, it seems good to move off the
> non-public APIs if possible

src/generated/element.dart is deprecated.  I can move us over to
src/dart/element/element.dart, and will do so if we decide we want to keep
reaching in, but I wanted to first call out the places where we are reaching our
tendrils into the analyzer internals.

[Brian Wilkerson, 2016-03-15 00:14:17]

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:17: show DynamicElementImpl, DynamicTypeImpl,
LocalVariableElementImpl;
> > I believe all of that can be replaced with `element is TypeDefiningElement`
> ...
> > assuming that DynamicElementImpl implements it.
> 
> It does.  But so does TypeParameterElement, which wouldn't currently be caught
> by this check.  But that looks to me like it might be a bug?

I made it that way because a type parameter defines a new type that is visible
in the scope of the generic member. If that's the wrong way to think of it we
can talk about changing it, but it would be a breaking change.

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (left):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/utils.dart#oldc...
lib/src/utils.dart:476: e.staticType ?? DynamicTypeImpl.instance;
> ... this might be a bug we've been working around unknowingly.

That does sound suspicious, and we should look into it. I found another case
earlier today, where VariableElement (IIRC) is documented as being able to have
a `null` type when one isn't declared. That sounds wrong to me too.

[Leaf, 2016-03-15 00:35:14]

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:17: show DynamicElementImpl, DynamicTypeImpl,
LocalVariableElementImpl;
On 2016/03/15 00:14:16, Brian Wilkerson wrote:
> > > I believe all of that can be replaced with `element is
TypeDefiningElement`
> > ...
> > > assuming that DynamicElementImpl implements it.
> > 
> > It does.  But so does TypeParameterElement, which wouldn't currently be
caught
> > by this check.  But that looks to me like it might be a bug?
> 
> I made it that way because a type parameter defines a new type that is visible
> in the scope of the generic member. If that's the wrong way to think of it we
> can talk about changing it, but it would be a breaking change.

Sorry - I meant that it might be a bug in the code generator (since at first
glance it looked to me like we might actually want this code path to fire on a
TypeParameter).  I don't immediately see any reason why TypeParameterElement
shouldn't implement TypeDefiningElement.  In some sense I suppose it doesn't
"define" a type, but it looks like TypeDefiningElement serves as the common
interface for elements which correspond to types, which a TypeParameterElement
certainly does.

[Jennifer Messerly, 2016-03-15 16:23:19]

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1797063002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:17: show DynamicElementImpl, DynamicTypeImpl,
LocalVariableElementImpl;
On 2016/03/15 00:35:14, Leaf wrote:
> On 2016/03/15 00:14:16, Brian Wilkerson wrote:
> > > > I believe all of that can be replaced with `element is
> TypeDefiningElement`
> > > ...
> > > > assuming that DynamicElementImpl implements it.
> > > 
> > > It does.  But so does TypeParameterElement, which wouldn't currently be
> caught
> > > by this check.  But that looks to me like it might be a bug?
> > 
> > I made it that way because a type parameter defines a new type that is
visible
> > in the scope of the generic member. If that's the wrong way to think of it
we
> > can talk about changing it, but it would be a breaking change.
> 
> Sorry - I meant that it might be a bug in the code generator (since at first
> glance it looked to me like we might actually want this code path to fire on a
> TypeParameter).  I don't immediately see any reason why TypeParameterElement
> shouldn't implement TypeDefiningElement.  In some sense I suppose it doesn't
> "define" a type, but it looks like TypeDefiningElement serves as the common
> interface for elements which correspond to types, which a TypeParameterElement
> certainly does.

+1, I think it's a bug in the code generator. You can probably reproduce the bug
with:

class C<T> { m() => print(T); }
main() => new C<int>().m();

[Leaf, 2016-03-15 17:15:38]

Description was changed from

==========
Resolve obvious deprecation warnings on bleeding edge

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

to

==========
Resolve obvious deprecation warnings on bleeding edge

BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/67df07896e42263cd7af9e46e26c...
==========

[Leaf, 2016-03-15 17:15:39]

Committed patchset #5 (id:70001) manually as
67df07896e42263cd7af9e46e26c4827a9b3dd61 (presubmit successful).