DAS plugins: analyze and report diagnostics in parts

Fixes #61449

The previous code, sure enough, only created a DiagnosticReporter for
the defining unit, and only visited the main unit.

The fix is to create a DiagnosticListener for each unit, set the
DiagnosticReporters correctly and the "current unit" as we go, and then
pull the reported diagnostics from each listener.

Change-Id: I6b645284c58e5e35ccc5b89f30f6ffe97f48296c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/448644
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>

Author: srawlins

pkg/analysis_server_plugin/lib/src/plugin_server.dart

@@ -30,6 +30,7 @@ import 'package:analyzer/src/dart/analysis/analysis_context_collection.dart';
 import 'package:analyzer/src/dart/analysis/analysis_options.dart';
 import 'package:analyzer/src/dart/analysis/byte_store.dart';
 import 'package:analyzer/src/dart/analysis/file_content_cache.dart';
+import 'package:analyzer/src/dart/element/element.dart';
 import 'package:analyzer/src/dart/element/type_system.dart';
 import 'package:analyzer/src/ignore_comments/ignore_info.dart';
 import 'package:analyzer/src/lint/config.dart';
@@ -333,41 +334,47 @@ class PluginServer {
       path,
     );
     if (libraryResult is! ResolvedLibraryResult) {
+      // We only handle analyzing at the library-level. Below, we work through
+      // each of the compilation units found in `libraryResult`.
       return const [];
     }
-    var unitResult = await analysisContext.currentSession.getResolvedUnit(path);
-    if (unitResult is! ResolvedUnitResult) {
-      return const [];
-    }
-    var listener = RecordingDiagnosticListener();
-    var diagnosticReporter = DiagnosticReporter(
-      listener,
-      unitResult.libraryElement.firstFragment.source,
-    );
 
-    var currentUnit = RuleContextUnit(
-      file: unitResult.file,
-      content: unitResult.content,
-      diagnosticReporter: diagnosticReporter,
-      unit: unitResult.unit,
-    );
-    var allUnits = [
+    var diagnosticListeners = {
       for (var unitResult in libraryResult.units)
-        RuleContextUnit(
-          file: unitResult.file,
-          content: unitResult.content,
-          diagnosticReporter: diagnosticReporter,
-          unit: unitResult.unit,
+        unitResult: RecordingDiagnosticListener(),
+    };
+
+    RuleContextUnit? definingContextUnit;
+    var definingUnit =
+        (libraryResult.element as LibraryElementImpl).definingCompilationUnit;
+    var allUnits = <RuleContextUnit>[];
+
+    for (var unitResult in libraryResult.units) {
+      var contextUnit = RuleContextUnit(
+        file: unitResult.file,
+        content: unitResult.content,
+        diagnosticReporter: DiagnosticReporter(
+          diagnosticListeners[unitResult]!,
+          unitResult.libraryElement.firstFragment.source,
         ),
-    ];
+        unit: unitResult.unit,
+      );
+      allUnits.add(contextUnit);
+      if (unitResult.unit.declaredFragment == definingUnit) {
+        definingContextUnit = contextUnit;
+      }
+    }
+    // Just a fallback value. We shouldn't get into this situation, but this is
+    // a safe default.
+    definingContextUnit ??= allUnits.first;
 
     // TODO(srawlins): Enable timing similar to what the linter package's
-    // `benchhmark.dart` script does.
+    // `benchmark.dart` script does.
     var nodeRegistry = RuleVisitorRegistryImpl(enableTiming: false);
 
     var context = RuleContextWithResolvedResults(
       allUnits,
-      currentUnit,
+      definingContextUnit,
       libraryResult.element.typeProvider,
       libraryResult.element.typeSystem as TypeSystemImpl,
       // TODO(srawlins): Support 'package' parameter.
@@ -386,56 +393,72 @@ class PluginServer {
       var rules = Registry.ruleRegistry.enabled(
         configuration.diagnosticConfigs,
       );
-      for (var rule in rules) {
-        rule.reporter = diagnosticReporter;
-        // TODO(srawlins): Enable timing similar to what the linter package's
-        // `benchmark.dart` script does.
-        rule.registerNodeProcessors(nodeRegistry, context);
-      }
+
       for (var code in rules.expand((r) => r.diagnosticCodes)) {
         pluginCodeMapping.putIfAbsent(code, () => configuration.name);
         severityMapping.putIfAbsent(
           code,
           () => _configuredSeverity(configuration, code),
         );
       }
-    }
 
-    context.currentUnit = currentUnit;
-    currentUnit.unit.accept(
-      AnalysisRuleVisitor(nodeRegistry, shouldPropagateExceptions: true),
-    );
+      for (var rule in rules) {
+        // TODO(srawlins): Enable timing similar to what the linter package's
+        // `benchmark.dart` script does.
+        rule.registerNodeProcessors(nodeRegistry, context);
+      }
 
-    var ignoreInfo = IgnoreInfo.forDart(unitResult.unit, unitResult.content);
-    var diagnostics = listener.diagnostics.where((e) {
-      var pluginName = pluginCodeMapping[e.diagnosticCode];
-      if (pluginName == null) {
-        // If [e] is somehow not mapped, something is wrong; but don't mark it
-        // as ignored.
-        return true;
+      // Now to perform the actual analysis.
+      for (var currentUnit in allUnits) {
+        for (var rule in rules) {
+          rule.reporter = currentUnit.diagnosticReporter;
+        }
+
+        context.currentUnit = currentUnit;
+        currentUnit.unit.accept(
+          AnalysisRuleVisitor(nodeRegistry, shouldPropagateExceptions: true),
+        );
       }
-      return !ignoreInfo.ignored(e, pluginName: pluginName);
-    });
+    }
+
+    // TODO(srawlins): Support `AnalysisRuleVisitor.afterLibrary`. See how it is
+    // used in `library_analyzer.dart`.
 
     // The list of the `AnalysisError`s and their associated
     // `protocol.AnalysisError`s.
-    var diagnosticsAndProtocolErrors = [
-      for (var diagnostic in diagnostics)
-        (
+    var diagnosticsAndProtocolErrors =
+        <({Diagnostic diagnostic, protocol.AnalysisError protocolError})>[];
+
+    diagnosticListeners.forEach((unitResult, listener) {
+      var ignoreInfo = IgnoreInfo.forDart(unitResult.unit, unitResult.content);
+      var diagnostics = listener.diagnostics.where((e) {
+        var pluginName = pluginCodeMapping[e.diagnosticCode];
+        if (pluginName == null) {
+          // If [e] is somehow not mapped, something is wrong; but don't mark it
+          // as ignored.
+          return true;
+        }
+        return !ignoreInfo.ignored(e, pluginName: pluginName);
+      });
+
+      for (var diagnostic in diagnostics) {
+        diagnosticsAndProtocolErrors.add((
           diagnostic: diagnostic,
           protocolError: protocol.AnalysisError(
             severityMapping[diagnostic.diagnosticCode] ??
                 protocol.AnalysisErrorSeverity.INFO,
             protocol.AnalysisErrorType.STATIC_WARNING,
-            _locationFor(currentUnit.unit, path, diagnostic),
+            _locationFor(unitResult.unit, unitResult.path, diagnostic),
             diagnostic.message,
             diagnostic.diagnosticCode.name,
             correction: diagnostic.correctionMessage,
             // TODO(srawlins): Use a valid value here.
             hasFix: true,
           ),
-        ),
-    ];
+        ));
+      }
+    });
+
     _recentState[path] = (
       analysisContext: analysisContext,
       errors: [...diagnosticsAndProtocolErrors],

pkg/analysis_server_plugin/test/src/lint_rules.dart

@@ -72,6 +72,31 @@ class NoDoublesCustomSeverityRule extends AnalysisRule {
   }
 }
 
+class NoReferencesToStringsRule extends AnalysisRule {
+  static const LintCode code = LintCode(
+    'no_references_to_strings',
+    'No references to Strings',
+  );
+
+  NoReferencesToStringsRule()
+    : super(
+        name: 'no_references_to_strings',
+        description: 'No references to Strings',
+      );
+
+  @override
+  DiagnosticCode get diagnosticCode => code;
+
+  @override
+  void registerNodeProcessors(
+    RuleVisitorRegistry registry,
+    RuleContext context,
+  ) {
+    var visitor = _NoReferencesToStringsVisitor(this, context);
+    registry.addSimpleIdentifier(this, visitor);
+  }
+}
+
 class _NoBoolsVisitor extends SimpleAstVisitor<void> {
   final AnalysisRule rule;
 
@@ -93,3 +118,18 @@ class _NoDoublesVisitor extends SimpleAstVisitor<void> {
     rule.reportAtNode(node);
   }
 }
+
+class _NoReferencesToStringsVisitor extends SimpleAstVisitor<void> {
+  final AnalysisRule rule;
+
+  final RuleContext context;
+
+  _NoReferencesToStringsVisitor(this.rule, this.context);
+
+  @override
+  void visitSimpleIdentifier(SimpleIdentifier node) {
+    if (node.staticType?.isDartCoreString ?? false) {
+      rule.reportAtNode(node);
+    }
+  }
+}

pkg/analysis_server_plugin/test/src/plugin_server_test.dart

@@ -34,14 +34,16 @@ class PluginServerTest extends PluginServerTestBase {
 
   String get filePath => join(packagePath, 'lib', 'test.dart');
 
+  String get file2Path => join(packagePath, 'lib', 'test2.dart');
+
   String get packagePath => convertPath('/package1');
 
   StreamQueue<protocol.AnalysisErrorsParams> get _analysisErrorsParams {
     return StreamQueue(
       channel.notifications
           .where((n) => n.event == protocol.ANALYSIS_NOTIFICATION_ERRORS)
           .map((n) => protocol.AnalysisErrorsParams.fromNotification(n))
-          .where((p) => p.file == filePath),
+          .where((p) => p.file == filePath || p.file == file2Path),
     );
   }
 
@@ -229,7 +231,11 @@ bool b = false;
     expect(details.name, 'No Literals Plugin');
     expect(
       details.lintRules,
-      unorderedEquals(['no_doubles', 'no_doubles_custom_severity']),
+      unorderedEquals([
+        'no_doubles',
+        'no_doubles_custom_severity',
+        'no_references_to_strings',
+      ]),
     );
     expect(details.warningRules, unorderedEquals(['no_bools']));
     expect(details.fixes, hasLength(1));
@@ -314,6 +320,87 @@ bool b = false;
     _expectAnalysisError(params.errors.single, message: 'No bools message');
   }
 
+  Future<void> test_updateContent_addOverlay_affectedPart() async {
+    writeAnalysisOptionsWithPlugin({'no_references_to_strings': 'enable'});
+    newFile(filePath, '''
+part 'test2.dart';
+int s = 7;
+''');
+    newFile(file2Path, '''
+part of 'test.dart';
+void f() {
+  print(s);
+}
+''');
+    await channel.sendRequest(
+      protocol.AnalysisSetContextRootsParams([contextRoot]),
+    );
+
+    var paramsQueue = _analysisErrorsParams;
+    var params = await paramsQueue.next; // test.dart
+    expect(params.errors, isEmpty);
+    params = await paramsQueue.next; // test2.dart
+    expect(params.errors, isEmpty);
+
+    await channel.sendRequest(
+      protocol.AnalysisUpdateContentParams({
+        filePath: protocol.AddContentOverlay('''
+part 'test2.dart';
+String s = "hello";
+'''),
+      }),
+    );
+
+    params = await paramsQueue.next;
+    expect(params.errors, hasLength(1));
+    _expectAnalysisError(
+      params.errors.single,
+      message: 'No references to Strings',
+    );
+  }
+
+  Future<void> test_updateContent_addOverlay_affectedLibrary() async {
+    writeAnalysisOptionsWithPlugin({'no_references_to_strings': 'enable'});
+    newFile(filePath, '''
+int s = 7;
+void f() {
+  print(s);
+}
+''');
+    newFile(file2Path, '''
+import 'test.dart';
+void f() {
+  print(s);
+}
+''');
+    await channel.sendRequest(
+      protocol.AnalysisSetContextRootsParams([contextRoot]),
+    );
+
+    var paramsQueue = _analysisErrorsParams;
+    var params = await paramsQueue.next; // test.dart
+    expect(params.errors, isEmpty);
+    params = await paramsQueue.next; // test2.dart
+    expect(params.errors, isEmpty);
+
+    await channel.sendRequest(
+      protocol.AnalysisUpdateContentParams({
+        filePath: protocol.AddContentOverlay('''
+String s = "hello";
+'''),
+      }),
+    );
+
+    params = await paramsQueue.next; // test.dart
+    params = await paramsQueue.next; // test2.dart
+    expect(params.errors, hasLength(1));
+    expect(params.file, file2Path);
+    _expectAnalysisError(
+      params.errors.single,
+      message: 'No references to Strings',
+    );
+  }
+
   Future<void> test_updateContent_removeOverlay() async {
     writeAnalysisOptionsWithPlugin();
     newFile(filePath, 'bool b = false;');
@@ -509,6 +596,7 @@ class _NoLiteralsPlugin extends Plugin {
     registry.registerWarningRule(NoBoolsRule());
     registry.registerLintRule(NoDoublesRule());
     registry.registerLintRule(NoDoublesCustomSeverityRule());
+    registry.registerLintRule(NoReferencesToStringsRule());
     registry.registerFixForRule(NoBoolsRule.code, _WrapInQuotes.new);
     registry.registerAssist(_InvertBoolean.new);
   }