Strong mode local variable inference

Strong mode local variable inference

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

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

[Leaf, 2015-08-26 21:59:28]

leafp@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com

[Leaf, 2015-08-26 22:00:54]

Just a small initial CL to get started and to get some early feedback.  This
adds local variable inference and ports over the relevant tests from DDC.

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:1155: this._options.strongMode =
options.strongMode;
I added these when I was trying to track down how to thread strongMode down to
the StaticTypeAnalyzer.  I don't think they're relevant to the CL, but they do
seem like maybe they should be here?  I can take them back out if not. 
Everything else in this file is just formatter churn.

[Brian Wilkerson, 2015-08-26 22:19:25]

LGTM

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/engine.dart:1155: this._options.strongMode =
options.strongMode;
Good catch! Yes, they should be here.

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1186: if (initializer
== null) return;
nit: we have a group style of never using non-block statements in control
structures

[Leaf, 2015-08-26 22:25:52]

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1186: if (initializer
== null) return;
On 2015/08/26 22:19:25, Brian Wilkerson wrote:
> nit: we have a group style of never using non-block statements in control
> structures

Ok.  Do you guys have a preferred style for this idiom?

if (condA) {return;}
if (condB) {return;}
....

vs

if (condA || 
    condB || 
         ...) {
  return;
}

vs 

if (!condA && !condB && ....) {
...
}
return;

[Brian Wilkerson, 2015-08-26 23:10:28]

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1314793005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1186: if (initializer
== null) return;
Nope. Whichever you think is more readable.

[Leaf, 2015-08-27 00:04:00]

PTAL.  

Restructured to match coding style.

I'm testing for LocalVariableElementImpl instead of LocalVariableElement to get
access to the .type setter (I forgot to run the analyzer on the last CL).  Let
me know if there's a better way to do that.

I also eliminated the test at the end to test for PropertyInducingElement.  I
had carried over that code from the old implementation, but I can't think of
when that could possibly occur - in fact, I think that the test for
LocalVariableElement might preclude this.  Am I missing something there?

Also added a test for inference based on List literals.

[Paul Berry, 2015-08-27 01:28:11]

lgtm

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1190:
(initializer.staticType != _typeProvider.bottomType)) {
Nit: use initializer.staticType.isBottom.

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:13556: void _enterStrongMode() {
Consider renaming this method to setUp() and adding an "@override" declaration. 
That way it will be automatically called just before every test, so you can drop
all the explicit calls to it.

[Brian Wilkerson, 2015-08-27 02:47:20]

LGTM

> I'm testing for LocalVariableElementImpl instead of
> LocalVariableElement to get access to the .type setter (I forgot
> to run the analyzer on the last CL).  Let me know if there's a
> better way to do that.

No, that seems reasonable.

> I also eliminated the test at the end to test for
> PropertyInducingElement.  I had carried over that code from
> the old implementation, but I can't think of when that could
> possibly occur - in fact, I think that the test for
> LocalVariableElement might preclude this.  Am I missing
> something there?

Not that I can think of. Local variables don't have implicit getters or setters,
so this should be right.

[Leaf, 2015-08-27 18:15:26]

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1190:
(initializer.staticType != _typeProvider.bottomType)) {
On 2015/08/27 01:28:11, Paul Berry wrote:
> Nit: use initializer.staticType.isBottom.

Done.

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1314793005/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:13556: void _enterStrongMode() {
On 2015/08/27 01:28:11, Paul Berry wrote:
> Consider renaming this method to setUp() and adding an "@override"
declaration. 
> That way it will be automatically called just before every test, so you can
drop
> all the explicit calls to it.

This reveals an issue, actually.  I made StrongModeTypePropagationTest a
subclass of TypePropagationTest mostly just to get access to the
_findMarkedIdentifier and a few other helpers.  I just realized though that this
results in all of the superclass tests being re-run, which I probably don't want
(and certainly don't want to happen with the strongMode flag set).  

I've moved those helpers up to ResolverTestCase and made
StrongModeTypePropagationTest a direct subclass, which seems like a reasonable
place for them to reside.  I also renamed _assertPropagatedReturnType to
_assertPropagatedAssignedType, since that seems to be are more accurate name. 
WDYT?

[Brian Wilkerson, 2015-08-27 18:18:33]

SGTM / LGTM

[Leaf, 2015-08-27 20:27:15]

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