Task to infer static variables

Task to infer static variables

R=leafp@google.com, paulberry@google.com

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

[Brian Wilkerson, 2015-08-29 00:25:11]

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

[Paul Berry, 2015-08-29 01:14:55]

lgtm assuming the circularity issue is addressed.

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:2443: class
InferStaticVariableTypesInUnitTask extends SourceBasedAnalysisTask {
Since this task has dependencies which are other static variable types, it might
result in a circularity.  So this task needs to contain:

  @override
  bool get handlesDependencyCycles => true;

and its implementation of internalPerform() needs to check for a non-null value
of dependencyCycle.

Unlike in ComputeConstantValueTask, which can simply generate an error for
dependency cycles, I believe we need to have well-defined semantics for
dependency cycles.  IIRC, the way we decided to handle this was: infer the type
of each variable in the cycle assuming the other variables in the cycle have
type "dynamic", and then assign the types of all the variables in one go.

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:2587:
visitor.initForIncrementalResolution();
It seems weird that we both manually initialize the visitor for incremental
resolution (by calling visitor.prepareToResolveMembersInClass()) and tell it to
initialize itself for incremental resolution.  As a future improvement, consider
making resolutionContext a parameter to initForIncrementalResolution() so that
the visitor takes care of all the initialization in one call.

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:18: if (element is
PropertyAccessorElementImpl) {
I realize this is just code motion, so feel free to defer this to a later CL if
you'd prefer.

Could this function ever get called for a non-PropertyAccessorElementImpl in the
absence of analyzer bugs?  If not, please add an assertion so that checked mode
unit tests will fail if this function ever gets passed an unexpected type.

Similarly for the other "if" tests in this function.

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:41: if (element is
ExecutableElementImpl) {
Similar concerns in this function.

[Brian Wilkerson, 2015-08-31 15:08:52]

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:2443: class
InferStaticVariableTypesInUnitTask extends SourceBasedAnalysisTask {
Sorry, I should have included this in a comment, but I'm aware of the need to
handle circularities and plan on doing so in a follow-on CL.

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:2587:
visitor.initForIncrementalResolution();
The method initForIncrementalResolution() is also called from the incremental
resolver, and I didn't want to re-factor that code as part of this CL, so I left
it as it was. I certainly don't mind if someone wants to look for a cleaner way
to structure these methods at a later time.

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:18: if (element is
PropertyAccessorElementImpl) {
Each of the 'if' statements in these methods is there to avoid needing to have
an explicit cast somewhere later. Unfortunately assert statements don't have the
same effect.

[Paul Berry, 2015-08-31 15:50:59]

lgtm

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:2443: class
InferStaticVariableTypesInUnitTask extends SourceBasedAnalysisTask {
On 2015/08/31 15:08:52, Brian Wilkerson wrote:
> Sorry, I should have included this in a comment, but I'm aware of the need to
> handle circularities and plan on doing so in a follow-on CL.

Acknowledged.

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:18: if (element is
PropertyAccessorElementImpl) {
On 2015/08/31 15:08:52, Brian Wilkerson wrote:
> Each of the 'if' statements in these methods is there to avoid needing to have
> an explicit cast somewhere later. Unfortunately assert statements don't have
the
> same effect.

I understand that.  I am not suggesting replacing the "if" tests with
assertions.  I'm suggesting *adding* assertions so that bugs won't go unnoticed.

For example, you could do:

assert(element is PropertyAccessorImpl);
if (element is PropertyAccessorImpl) {
  ...
}

or:

if (element is PropertyAccessorImpl) {
  ...
} else {
  assert(false);
}

Both of these alternatives would have the advantage of extra unit test strength,
without sacrificing runtime performance or robustness in unchecked code.

[Leaf, 2015-08-31 17:46:03]

lgtm

[Brian Wilkerson, 2015-08-31 18:35:38]

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1323653002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:18: if (element is
PropertyAccessorElementImpl) {
While I understand the testing value, I really don't like either alternative.
They both clutter the code; the first worse than the second because of the
duplication of the condition. I'll add the second form, but under protest.

[Brian Wilkerson, 2015-08-31 18:36:47]

Committed patchset #1 (id:1) manually as
1463fb3ff2d0e132acc9bcb1a3044d945a5d58c5 (presubmit successful).