use VariableElement instead of VariableElementImpl in _isStateless

use VariableElement instead of VariableElementImpl in _isStateless

R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/1bb565f1a52eca9eb1bc5b00263962e653cfbee8

[Jennifer Messerly, 2015-04-03 23:12:51]

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

[Jennifer Messerly, 2015-04-03 23:13:58]

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

[Brian Wilkerson, 2015-04-04 15:58:24]

LGTM

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

https://codereview.chromium.org/1056183003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1700: if (e is VariableElement &&
!e.isSynthetic) {
This is a good first step, but doesn't fix the problem. If 'e' is an instance of
ParameterMember it will pass this 'is' check, but will fail the 'is' check below
(line 1702).

We really need to address this issue in the Element API, but one ugly work
around would be to add the following before line 1702:
  // TODO Remove the following statement and convert the condition below to use
the public classes
  // once _isPotentiallyMutated does not require an implementation class.
  if (e is Member) {
    e = e.baseElement;
  }

[Jennifer Messerly, 2015-04-06 15:19:22]

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

https://codereview.chromium.org/1056183003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1700: if (e is VariableElement &&
!e.isSynthetic) {
On 2015/04/04 15:58:24, Brian Wilkerson wrote:
> This is a good first step, but doesn't fix the problem. If 'e' is an instance
of
> ParameterMember it will pass this 'is' check, but will fail the 'is' check
below
> (line 1702).
> 
> We really need to address this issue in the Element API, but one ugly work
> around would be to add the following before line 1702:
>   // TODO Remove the following statement and convert the condition below to
use
> the public classes
>   // once _isPotentiallyMutated does not require an implementation class.
>   if (e is Member) {
>     e = e.baseElement;
>   }

Sounds good, but how can I get a ParameterMember here to test it? This code is
used when compiling a Dart method cascade, such as:

    x..add(1)..add(2)

How can I make "x" be a ParameterMember? I tried:

class Generic<T extends List<int>> {
  test_generic_type_parameter(T x) {
    x..add(1)..add(2)..add(3)..add(4);
  }
}

class Derived extends Generic<List<int>> {
  test_generic_type_parameter(List<int> x) {
    x..add(1)..add(2)..add(3)..add(4);
  }
}

Neither case was able to cause a problem.

[Brian Wilkerson, 2015-04-06 15:23:05]

Still LGTM

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

https://codereview.chromium.org/1056183003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1700: if (e is VariableElement &&
!e.isSynthetic) {
> How can I make "x" be a ParameterMember? I tried:

I don't think you can. My comment was based on a misunderstanding on my part of
how this code was being used, and should be ignored. It's fine the way it is.

[Jennifer Messerly, 2015-04-06 15:46:31]

thank you!

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

https://codereview.chromium.org/1056183003/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1700: if (e is VariableElement &&
!e.isSynthetic) {
On 2015/04/06 15:23:05, Brian Wilkerson wrote:
> > How can I make "x" be a ParameterMember? I tried:
> 
> I don't think you can. My comment was based on a misunderstanding on my part
of
> how this code was being used, and should be ignored. It's fine the way it is.

Ah that's good. I think I still like your variation `if (e is Member) e =
e.baseMember;` better as it seems cleaner and that way if someone later reused
this code, we'll be safer :). I'll also make the comment clear that this
function (_isStateless) is intended to be conservative, and should not be relied
on to correctly detect all cases.

[Jennifer Messerly, 2015-04-06 15:56:48]

I've added some comments, WDYT?

[Brian Wilkerson, 2015-04-06 16:03:23]

LGTM

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

https://codereview.chromium.org/1056183003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2210: // isPotentiallyMutated is available on
VariableElement.
I think this support was just published as 0.24.1.

[Jennifer Messerly, 2015-04-06 16:13:09]

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

[Jennifer Messerly, 2015-04-06 16:16:33]

nice! I'm not sure who necessarily owns the issue, now that Siggi is off doing
other things, but I filed https://github.com/dart-lang/dev_compiler/issues/124