Implement a task to compute the dependencies between static variables

Implement a task to compute the dependencies between static variables

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

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

[Brian Wilkerson, 2015-08-27 18:09:07]

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

[Paul Berry, 2015-08-27 18:56:08]

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

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:10529: * an entire library or class.
Since our notion of what "isStatic" means differs from the situations where the
"static" keyword is used in Dart, consider adding some text like this to avoid
confusion:

"Top level variables are considered static.  Local variables are never
considered static, even if they are inside a top level function."

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

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:25: class DeclarationFinder extends
RecursiveAstVisitor {
I'm concerned about the execution speed of this class, since it recursively
visits the entire AST, even parts of it that are distant from the target
element.

Why not use NodeLocator instead?  It is smart about pruning the search to just
the parts of the AST that overlap with the searched-for location, so it should
be much faster.

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:46: }
Were you intending to add:

throw new DeclarationFoundException();

here?  Assuming you were, consider changing the code so that the found
declaration is stored inside the DeclarationFoundException itself, rather than
in the visitor class.  That way the data flow is clearer, since the data we are
searching for rides along with the exception class that we are using to unwind
the stack when it is found.

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:53: static VariableDeclaration
find(AstNode node, VariableElement element) {
I'm not comfortable with the way this function is handling error conditions. 
There are two sources of error conditions:

1. an unexpected exception occurs while traversing the AST structure.  In this
case I think it would make sense to simply let the exception bubble up the stack
(as it would in any other code that wasn't being tricky with exceptions).  But
instead we turn the exception into a log entry and return null, which makes it
harder to notice, and may cause the caller to misbehave in a way that won't be
easily traceable to the source of the problem.

2. the node is not found.  In this case I think we should throw an exception,
because it is an exceptional circumstance (this should never happen in the
absence of a bug in the task model).  But instead we return null.  I'm less
bothered by this case than the previous one (since if it ever arises, I think it
will be relatively easy to guess based on the null return value that the node
wasn't found), but I think raising an exception would be even better since there
would be no guesswork required.

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/dart_test.dart (right):

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:136:
test_perform_find_constants_weak() {
It looks like this test is now the same as the previous one.  Consolidate them
into a single test.

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:184: test_perform_library_weak() {
Same issue here.

[Brian Wilkerson, 2015-08-27 20:43:54]

Changes made and uploaded, in case you want to take another look.

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

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:10529: * an entire library or class.
I inserted the rest of the quote from the DLS (with the appropriate reference).

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

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:25: class DeclarationFinder extends
RecursiveAstVisitor {
> Why not use NodeLocator instead?

Because I didn't think of it. You're right, it would be better.

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:46: }
moot

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:53: static VariableDeclaration
find(AstNode node, VariableElement element) {
moot

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/dart_test.dart (right):

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:136:
test_perform_find_constants_weak() {
Done

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:184: test_perform_library_weak() {
Done

[Paul Berry, 2015-08-27 21:23:14]

lgtm

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

https://codereview.chromium.org/1320923003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/element.dart:10529: * an entire library or class.
On 2015/08/27 20:43:54, Brian Wilkerson wrote:
> I inserted the rest of the quote from the DLS (with the appropriate
reference).

Wow, I didn't realize the language spec actually defined "static" differently
from "uses static keyword".  Thanks for the clarification.

[Leaf, 2015-08-28 17:44:48]

lgtm

[Brian Wilkerson, 2015-08-28 18:42:19]

Committed patchset #2 (id:20001) manually as
7ce07af57ec4e44b6016e0d7484669a99d335d49 (presubmit successful).