Delegating AST Visitor and Linter refactoring.

Delegating AST Visitor and Linter refactoring.

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42923

[pquitslund, 2015-01-15 04:14:07]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[pquitslund, 2015-01-15 04:14:08]

Try as I might I just can't get gcl not to barf on resolver.dart.

Resorting to good old fashioned diff.

Happy to show you in person in the office tomorrow as well.


Index: pkg/analyzer/lib/src/generated/resolver.dart
===================================================================
--- pkg/analyzer/lib/src/generated/resolver.dart	(revision 42890)
+++ pkg/analyzer/lib/src/generated/resolver.dart	(working copy)
@@ -8,6 +8,7 @@
 import 'dart:collection';
 
 import 'package:analyzer/src/generated/utilities_collection.dart';
+import 'package:analyzer/src/generated/visitors.dart';
 
 import 'ast.dart';
 import 'constant.dart';
@@ -4470,54 +4471,6 @@
 }
 
 
-/// Traverses a library's worth of dart code at a time to generate lint
warnings
-/// over the set of sources.
-///
-/// See [LintCode].
-class LintGenerator {
-
-  /// A global container for contributed verifiers.
-  static final List<LintVerifier> VERIFIERS = <LintVerifier>[];
-
-  final Iterable<CompilationUnit> _compilationUnits;
-  final AnalysisErrorListener _errorListener;
-  final Iterable<LintVerifier> _verifiers;
-
-  LintGenerator(this._compilationUnits, this._errorListener,
-      [Iterable<LintVerifier> verifiers])
-      : _verifiers = verifiers != null ? verifiers : VERIFIERS;
-
-  void generate() {
-    TimeCounter_TimeCounterHandle timeCounter =
-        PerformanceStatistics.lint.start();
-    try {
-      _compilationUnits.forEach((cu) {
-        if (cu.element != null) {
-          _generate(cu, cu.element.source);
-        }
-      });
-    } finally {
-      timeCounter.stop();
-    }
-  }
-
-  void _generate(CompilationUnit unit, Source source) {
-    ErrorReporter errorReporter = new ErrorReporter(_errorListener, source);
-    _verifiers.forEach((verifier) {
-      verifier.reporter = errorReporter;
-      return unit.accept(verifier);
-    });
-  }
-}
-
-/// Implementers contribute lint warnings via the provided error [reporter].
-abstract class LintVerifier extends RecursiveAstVisitor<Object> {
-  /// Used to report lint warnings.
-  /// NOTE: this is set by the framework before visit begins.
-  ErrorReporter reporter;
-}
-
-
 /**
  * Instances of the class {@code HtmlTagInfo} record information about the tags
used in an HTML
  * file.
@@ -9249,6 +9202,57 @@
   }
 }
 
+/// Implementers contribute lint warnings via the provided error [reporter].
+abstract class Linter {
+  /// Used to report lint warnings.
+  /// NOTE: this is set by the framework before visit begins.
+  ErrorReporter reporter;
+
+  /// Return a visitor to be passed to compilation units to perform lint
+  /// analysis.
+  /// Lint errors are reported via this [Linter]'s error [reporter].
+  AstVisitor getVisitor();
+}
+
+/// Traverses a library's worth of dart code at a time to generate lint
warnings
+/// over the set of sources.
+///
+/// See [LintCode].
+class LintGenerator {
+
+  /// A global container for contributed linters.
+  static final List<Linter> LINTERS = <Linter>[];
+
+  final Iterable<CompilationUnit> _compilationUnits;
+  final AnalysisErrorListener _errorListener;
+  final Iterable<Linter> _linters;
+
+  LintGenerator(this._compilationUnits, this._errorListener,
+      [Iterable<Linter> linters])
+      : _linters = linters != null ? linters : LINTERS;
+
+  void generate() {
+    TimeCounter_TimeCounterHandle timeCounter =
+        PerformanceStatistics.lint.start();
+    try {
+      _compilationUnits.forEach((cu) {
+        if (cu.element != null) {
+          _generate(cu, cu.element.source);
+        }
+      });
+    } finally {
+      timeCounter.stop();
+    }
+  }
+
+  void _generate(CompilationUnit unit, Source source) {
+    ErrorReporter errorReporter = new ErrorReporter(_errorListener, source);
+    _linters.forEach((l) => l.reporter = errorReporter);
+    Iterable<AstVisitor> visitors = _linters.map((l) => l.getVisitor());
+    unit.accept(new DelegatingAstVisitor(visitors.where((v) => v != null)));
+  }
+}
+
 /**
  * This class is used to replace uses of `HashMap<String, ExecutableElement>`
which are not as
  * performant as this class.

[scheglov, 2015-01-15 06:00:38]

LGTM

[Brian Wilkerson, 2015-01-15 14:59:54]

LGTM

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/visitors.dart (right):

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/visitors.dart:9: /// Instances delegate calls to
visit methods to all [delegates] before calling
I think the style guide suggests something closer to

An [AstVisitor] that delegates calls...

(But I might be wrong.)

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/engine_test.dart (right):

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/engine_test.dart:4135: class
LintGeneratorTest_Linter extends Linter with SimpleAstVisitor<Object> {
Note that the original class name was only chosen because in Java the class
Verifier was an inner class. (That's the only time you'll find underscores in
type names.) Up to you whether to change it.

[pquitslund, 2015-01-15 18:07:30]

Thanks!

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/visitors.dart (right):

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/visitors.dart:9: /// Instances delegate calls to
visit methods to all [delegates] before calling
On 2015/01/15 14:59:54, Brian Wilkerson wrote:
> I think the style guide suggests something closer to
> 
> An [AstVisitor] that delegates calls...
> 
> (But I might be wrong.)

Nope.  You're right.  Fixed.

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/engine_test.dart (right):

https://codereview.chromium.org/813483007/diff/180001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/engine_test.dart:4135: class
LintGeneratorTest_Linter extends Linter with SimpleAstVisitor<Object> {
On 2015/01/15 14:59:54, Brian Wilkerson wrote:
> Note that the original class name was only chosen because in Java the class
> Verifier was an inner class. (That's the only time you'll find underscores in
> type names.) Up to you whether to change it.

Thanks.  Were this Java, it would be an inner class so I'll stick with it for
now.

[pquitslund, 2015-01-15 18:09:48]

Committed patchset #11 (id:200001) manually as r42923 (presubmit successful).