Fix DDC errors on DDC

Fix DDC errors on DDC

Eating our own dogfood...

Some errors still in upstream packages, but this doesn't count Paul's recent CL.

> dart -c bin/devc.dart -i bin/devc.dart

package         AE   IMO  IRCE   STE     LOC
analyzer         0    10     0    12  111627
shelf            0     0     1     0    1415
http_parser      0     1     1     0    1473
source_span      2     0     0     0     708
yaml             7     5     0     1    3850
dart_style       2     0     0     0    4577
html             0    14     1   108   12307
csslib           3     0     0     6   10227
utf              0     0     0     2    1516
------------ ----- ----- ----- ----- -------
package         AE   IMO  IRCE   STE     LOC
total           14    30     3   129  175897
%             0.01  0.02  0.00  0.07     100

Where:
  AE:   AnalyzerError
  IMO:  InvalidMethodOverride
  IRCE: InvalidRuntimeCheckError
  STE:  StaticTypeError
  LOC:  LinesOfCode


BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/058ae7cb6817ed83ddb2153be57478eb8235fbf4

[vsm, 2015-06-01 21:38:43]

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

[Jennifer Messerly, 2015-06-01 21:52:11]

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dart
File lib/src/checker/checker.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dar...
lib/src/checker/checker.dart:230: var element = variable.element as
PropertyInducingElement;
random Q, do we always prefer this pattern over:

PropertyInducingElement element = variable.element;

?

reason I ask, it's 7 chars shorter ("var_as_"), which is almost 10% of line
length or 3.5 indent levels

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dar...
lib/src/checker/checker.dart:244: var method = member.element;
instead of changing the if structure, maybe

  var method = (member as MethodDeclaration).element;

then no assert needed

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:201: var varElement = variable.element as
PropertyInducingElement;
hmm, this one almost seems like feedback for analyzer. Because fields and local
variables are mashed together, there's no type safe way to access the
field/local variable elements.

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:239: var enclosingElement =
methodElement.enclosingElement as ClassElement;
not sure if it's any better, but another way to express this:

var classElem = (methodElement as ClassMemberElement).enclosingElement;

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2065: (member as
PropertyAccessorElement).variable;
This is going to be an analyzer hint because this cast is unnecessary, right?
That will be kind of annoying to see in the editor all the time (actually, won't
it break our presubmit too?)

Makes me think we need to fix DDC to understand promotion rules.

[vsm, 2015-06-01 21:59:37]

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2065: (member as
PropertyAccessorElement).variable;
On 2015/06/01 21:52:10, John Messerly wrote:
> This is going to be an analyzer hint because this cast is unnecessary, right?
> That will be kind of annoying to see in the editor all the time (actually,
won't
> it break our presubmit too?)
> 
> Makes me think we need to fix DDC to understand promotion rules.

Actually, this one is an AnalyzerError.  Because member is modified in the body
of the if, no promotion.  This appears to be the letter of the law in the
spec....

[Paul Berry, 2015-06-01 22:00:45]

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1153: fillDynamicTypeArgs((element as
dynamic).type, types));
I'm curious why this change was necessary.  It seems counter-intuitive.

https://codereview.chromium.org/1160223006/diff/1/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/js/printer.dart#new...
lib/src/js/printer.dart:1269: bool visitClassDeclaration(ClassDeclaration node)
=> false;
Very cool that DDC caught this!  Incidentally, we have talked in the past about
making a lint heuristic to detect this sort of mistake, but I couldn't find
anything in the bug system about it.  I'm not sure if we decided it was
infeasible for some reason or just forgot about it :)

[Jennifer Messerly, 2015-06-01 22:17:38]

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1153: fillDynamicTypeArgs((element as
dynamic).type, types));
On 2015/06/01 22:00:45, Paul Berry wrote:
> I'm curious why this change was necessary.  It seems counter-intuitive.

I don't think there's a common supertype of ClassElement and
FunctionTypeAliasElement that has the type getter. (Element does not have it).
This one trips me up occasionally.

And probably inference is causing `var element` to infer as Element, as that's
what SimpleIdentifier's staticElement is typed as.

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2065: (member as
PropertyAccessorElement).variable;
On 2015/06/01 21:59:36, vsm wrote:
> On 2015/06/01 21:52:10, John Messerly wrote:
> > This is going to be an analyzer hint because this cast is unnecessary,
right?
> > That will be kind of annoying to see in the editor all the time (actually,
> won't
> > it break our presubmit too?)
> > 
> > Makes me think we need to fix DDC to understand promotion rules.
> 
> Actually, this one is an AnalyzerError.  Because member is modified in the
body
> of the if, no promotion.  This appears to be the letter of the law in the
> spec....

oh, wow. crazy stuff.

small style nit: when "if" wraps to mutliline, usually use { ... } at that point

[vsm, 2015-06-01 22:44:20]

thanks- ptal

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dart
File lib/src/checker/checker.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dar...
lib/src/checker/checker.dart:230: var element = variable.element as
PropertyInducingElement;
On 2015/06/01 21:52:10, John Messerly wrote:
> random Q, do we always prefer this pattern over:
> 
> PropertyInducingElement element = variable.element;
> 
> ?
> 
> reason I ask, it's 7 chars shorter ("var_as_"), which is almost 10% of line
> length or 3.5 indent levels

I lean toward the rhs version - it makes it explicit that there is a cast going
on and that cast would appear in generated JS.

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dar...
lib/src/checker/checker.dart:244: var method = member.element;
On 2015/06/01 21:52:10, John Messerly wrote:
> instead of changing the if structure, maybe
> 
>   var method = (member as MethodDeclaration).element;
> 
> then no assert needed

It's actually on isStatic, but done there.

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:239: var enclosingElement =
methodElement.enclosingElement as ClassElement;
On 2015/06/01 21:52:10, John Messerly wrote:
> not sure if it's any better, but another way to express this:
> 
> var classElem = (methodElement as ClassMemberElement).enclosingElement;

This actually fails at runtime when methodElement is
PropertyAccessorElementImpl.

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1153: fillDynamicTypeArgs((element as
dynamic).type, types));
On 2015/06/01 22:17:38, John Messerly wrote:
> On 2015/06/01 22:00:45, Paul Berry wrote:
> > I'm curious why this change was necessary.  It seems counter-intuitive.
> 
> I don't think there's a common supertype of ClassElement and
> FunctionTypeAliasElement that has the type getter. (Element does not have it).
> This one trips me up occasionally.
> 
> And probably inference is causing `var element` to infer as Element, as that's
> what SimpleIdentifier's staticElement is typed as.

Right, I don't think we have a common interface type with the "type" getter.

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2065: (member as
PropertyAccessorElement).variable;
On 2015/06/01 22:17:38, John Messerly wrote:
> On 2015/06/01 21:59:36, vsm wrote:
> > On 2015/06/01 21:52:10, John Messerly wrote:
> > > This is going to be an analyzer hint because this cast is unnecessary,
> right?
> > > That will be kind of annoying to see in the editor all the time (actually,
> > won't
> > > it break our presubmit too?)
> > > 
> > > Makes me think we need to fix DDC to understand promotion rules.
> > 
> > Actually, this one is an AnalyzerError.  Because member is modified in the
> body
> > of the if, no promotion.  This appears to be the letter of the law in the
> > spec....
> 
> oh, wow. crazy stuff.
> 
> small style nit: when "if" wraps to mutliline, usually use { ... } at that
point

Done.

[Jennifer Messerly, 2015-06-01 23:10:56]

lgtm

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dart
File lib/src/checker/checker.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/checker.dar...
lib/src/checker/checker.dart:230: var element = variable.element as
PropertyInducingElement;
On 2015/06/01 22:44:19, vsm wrote:
> On 2015/06/01 21:52:10, John Messerly wrote:
> > random Q, do we always prefer this pattern over:
> > 
> > PropertyInducingElement element = variable.element;
> > 
> > ?
> > 
> > reason I ask, it's 7 chars shorter ("var_as_"), which is almost 10% of line
> > length or 3.5 indent levels
> 
> I lean toward the rhs version - it makes it explicit that there is a cast
going
> on and that cast would appear in generated JS.

err, wouldn't we generate a cast in JS for both cases?

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:239: var enclosingElement =
methodElement.enclosingElement as ClassElement;
On 2015/06/01 22:44:19, vsm wrote:
> On 2015/06/01 21:52:10, John Messerly wrote:
> > not sure if it's any better, but another way to express this:
> > 
> > var classElem = (methodElement as ClassMemberElement).enclosingElement;
> 
> This actually fails at runtime when methodElement is
> PropertyAccessorElementImpl.

doh. PropertyAccessorElement strikes again.

[vsm, 2015-06-02 00:02:28]

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

[Paul Berry, 2015-06-02 00:55:33]

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

[Paul Berry, 2015-06-02 00:55:34]

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1153: fillDynamicTypeArgs((element as
dynamic).type, types));
On 2015/06/01 22:44:19, vsm wrote:
> On 2015/06/01 22:17:38, John Messerly wrote:
> > On 2015/06/01 22:00:45, Paul Berry wrote:
> > > I'm curious why this change was necessary.  It seems counter-intuitive.
> > 
> > I don't think there's a common supertype of ClassElement and
> > FunctionTypeAliasElement that has the type getter. (Element does not have
it).
> > This one trips me up occasionally.
> > 
> > And probably inference is causing `var element` to infer as Element, as
that's
> > what SimpleIdentifier's staticElement is typed as.
> 
> Right, I don't think we have a common interface type with the "type" getter.

Ah, ok.  I will look into whether it's possible to create a common interface
implemented by ClassElement, DynamicElementImpl, and FunctionTypeAliasElement
(maybe something like "TypeDefiningElement").  That way you could change this
code to a single "is" check and take advantage of type promotion, e.g.:

if (element is TypeDefiningElement) {
  return _emitTypeName(fillDynamicTypeArgs(element.type, types));
}

[Jennifer Messerly, 2015-06-02 16:28:58]

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1160223006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1153: fillDynamicTypeArgs((element as
dynamic).type, types));
On 2015/06/02 00:55:34, Paul Berry wrote:
> On 2015/06/01 22:44:19, vsm wrote:
> > On 2015/06/01 22:17:38, John Messerly wrote:
> > > On 2015/06/01 22:00:45, Paul Berry wrote:
> > > > I'm curious why this change was necessary.  It seems counter-intuitive.
> > > 
> > > I don't think there's a common supertype of ClassElement and
> > > FunctionTypeAliasElement that has the type getter. (Element does not have
> it).
> > > This one trips me up occasionally.
> > > 
> > > And probably inference is causing `var element` to infer as Element, as
> that's
> > > what SimpleIdentifier's staticElement is typed as.
> > 
> > Right, I don't think we have a common interface type with the "type" getter.
> 
> Ah, ok.  I will look into whether it's possible to create a common interface
> implemented by ClassElement, DynamicElementImpl, and FunctionTypeAliasElement
> (maybe something like "TypeDefiningElement").  That way you could change this
> code to a single "is" check and take advantage of type promotion, e.g.:
> 
> if (element is TypeDefiningElement) {
>   return _emitTypeName(fillDynamicTypeArgs(element.type, types));
> }

+1 that would be awesome!