FED-194 Fully implement hooks exhaustive dependencies diagnostic, add tests

tools/analyzer_plugin/README.md

@@ -107,6 +107,21 @@ This script sets up a symlink to point to the original plugin directory (replaci
     * If you need to get the source for a replacement, use `sourceFile.getText(node.offset, node.end)`.
 
 ### Debugging the Plugin
+
+#### Showing Debug Info
+
+Via `AnalyzerDebugHelper`, it's possible for diagnostics to show debug information for specific locations in a file
+as infos, which can be helpful to develop/troubleshoot
+issues without having to attach a debugger.
+
+Some parts of the plugin show this debug info based on the presence of a comment anywhere in the file.
+Currently-available debug info:
+- `// debug: over_react_boilerplate`- shows how over_react boilerplate will be parsed/detected by the over_react
+  builder and analyzer functionality dealing with component declarations
+- `// debug: over_react_metrics` - shows performance data on how long diagnostics took to run
+- `// debug: over_react_exhaustive_deps` - shows info on how dependencies were detected/interpreted
+
+#### Attaching a Debugger
 The dev experience when working on this plugin isn't ideal (See the `analyzer_plugin` debugging docs [for more information](https://github.com/dart-lang/sdk/blob/master/pkg/analyzer_plugin/doc/tutorial/debugging.md)), but it's possible debug and see logs from the plugin.
 
 These instructions are currently for JetBrains IDEs (IntelliJ, WebStorm, etc.) only.

tools/analyzer_plugin/analysis_options.yaml

@@ -21,6 +21,8 @@ analyzer:
     prefer_single_quotes: ignore
     type_annotate_public_apis: ignore
     prefer_final_locals: ignore
+    # This is less readable in some cases, and isn't worth it.
+    join_return_with_assignment: ignore
 
 linter:
   rules:

tools/analyzer_plugin/lib/src/analysis_options/parse.dart

@@ -3,6 +3,7 @@ import 'package:yaml/yaml.dart';
 
 /// Parses the given yaml and returns over react analyzer plugin configuration options.
 PluginAnalysisOptions? processAnalysisOptionsFile(String fileContents) {
+  // TODO catch parse errors, add test coverage
   final yaml = loadYamlNode(fileContents);
   if (yaml is YamlMap) {
     return _parseAnalysisOptions(yaml);
@@ -15,14 +16,31 @@ PluginAnalysisOptions? processAnalysisOptionsFile(String fileContents) {
 PluginAnalysisOptions? _parseAnalysisOptions(YamlMap yaml) {
   final dynamic overReact = yaml['over_react'];
   if (overReact is YamlMap) {
-    final errors = _parseErrors(overReact);
-    return PluginAnalysisOptions(errors: errors);
+    return PluginAnalysisOptions(
+      errors: _parseErrors(overReact),
+      exhaustiveDepsAdditionalHooksPattern: _parseExhaustiveDepsAdditionalHooksPattern(overReact),
+    );
   }
 
   // If there is no `over_react` key in the yaml file, return null.
   return null;
 }
 
+RegExp? _parseExhaustiveDepsAdditionalHooksPattern(YamlMap overReact) {
+  final dynamic exhaustiveDeps = overReact['exhaustive_deps'];
+  if (exhaustiveDeps is! YamlMap) return null;
+
+  final dynamic additionalHooks = exhaustiveDeps['additional_hooks'];
+  if (additionalHooks is! String) return null;
+
+  try {
+    // This will throw if the regex is invalid.
+    return RegExp(additionalHooks);
+  } catch (_) {
+    return null;
+  }
+}
+
 Map<String, AnalysisOptionsSeverity> _parseErrors(YamlMap overReact) {
   final dynamic errors = overReact['errors'];
   if (errors is YamlMap) {

tools/analyzer_plugin/lib/src/analysis_options/plugin_analysis_options.dart

@@ -1,7 +1,12 @@
 /// An object containing the data under the `over_react` key in a analysis_options.yaml file.
 class PluginAnalysisOptions {
   final Map<String, AnalysisOptionsSeverity> errors;
-  PluginAnalysisOptions({required this.errors});
+  final RegExp? exhaustiveDepsAdditionalHooksPattern;
+
+  PluginAnalysisOptions({
+    required this.errors,
+    this.exhaustiveDepsAdditionalHooksPattern,
+  });
 }
 
 enum AnalysisOptionsSeverity {

tools/analyzer_plugin/lib/src/analysis_options/reader.dart

@@ -1,32 +1,31 @@
+import 'package:analyzer/dart/analysis/context_root.dart';
 import 'package:analyzer/dart/analysis/results.dart';
-import 'package:analyzer/file_system/file_system.dart';
-import 'package:over_react_analyzer_plugin/src/analysis_options/plugin_analysis_options.dart';
+import 'package:analyzer/file_system/file_system.dart' as analyzer_fs;
 import 'package:over_react_analyzer_plugin/src/analysis_options/parse.dart';
+import 'package:over_react_analyzer_plugin/src/analysis_options/plugin_analysis_options.dart';
 
-/// An analysis_options.yaml reader that parses the appropriate analysis_options.yaml file for the given
-/// [ResolvedUnitResult] and returns the configuration options for the over react analyzer plugin.
+/// An analysis_options.yaml reader that parses the appropriate analysis_options.yaml file
+/// and returns the configuration options for the over react analyzer plugin.
 ///
 /// The reader uses caching to reduce the number of file reads. If a result is given that uses the same
 /// analysis_options.yaml as a previous result, the reader will return a cache version.
-class AnalysisOptionsReader {
-  final _cachedAnalysisOptions = <String, PluginAnalysisOptions?>{};
+class PluginOptionsReader {
+  final _cachedOptions = <String, PluginAnalysisOptions?>{};
 
-  PluginAnalysisOptions? getAnalysisOptionsForResult(ResolvedUnitResult result) {
-    final file = result.session.analysisContext.contextRoot.optionsFile;
-    if (file != null) {
-      return _getAnalysisOptionForFilePath(file);
-    }
+  PluginAnalysisOptions? getOptionsForContextRoot(ContextRoot root) {
+    final file = root.optionsFile;
+    if (file == null) return null;
 
-    // There is no analysis_options.yaml, so return null.
-    return null;
+    return _getOptionsFromOptionsFile(file);
   }
 
-  PluginAnalysisOptions? _getAnalysisOptionForFilePath(File file) {
-    return _cachedAnalysisOptions.putIfAbsent(file.path, () => _readAnalysisOptionForFilePath(file));
-  }
+  PluginAnalysisOptions? getOptionsForResult(ResolvedUnitResult result) =>
+      getOptionsForContextRoot(result.session.analysisContext.contextRoot);
 
-  PluginAnalysisOptions? _readAnalysisOptionForFilePath(File file) {
-    final fileContents = file.readAsStringSync();
-    return processAnalysisOptionsFile(fileContents);
+  PluginAnalysisOptions? _getOptionsFromOptionsFile(analyzer_fs.File file) {
+    return _cachedOptions.putIfAbsent(file.path, () {
+      if (!file.exists) return null;
+      return processAnalysisOptionsFile(file.readAsStringSync());
+    });
   }
 }

tools/analyzer_plugin/lib/src/assist/refs/add_create_ref.dart

@@ -87,8 +87,9 @@ void addUseOrCreateRef(
   }
 
   if (refTypeToReplace == RefTypeToReplace.callback) {
+    if (createRefField == null) return;
     // Its a callback ref - meaning there is an existing field we need to update.
-    final declOfVarRefIsAssignedTo = lookUpVariable(createRefField, result.unit);
+    final declOfVarRefIsAssignedTo = lookUpVariable(createRefField, result.unit!);
     if (declOfVarRefIsAssignedTo == null) return;
 
     final nodeToReplace =

tools/analyzer_plugin/lib/src/async_plugin_apis/diagnostic.dart

@@ -39,18 +39,21 @@ import 'package:analyzer_plugin/protocol/protocol.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart';
 import 'package:analyzer_plugin/protocol/protocol_generated.dart' as plugin;
 import 'package:analyzer_plugin/protocol/protocol_generated.dart';
+
 // ignore: implementation_imports
 import 'package:analyzer_plugin/src/utilities/fixes/fixes.dart';
 import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
 import 'package:meta/meta.dart';
 import 'package:over_react_analyzer_plugin/src/async_plugin_apis/error_severity_provider.dart';
 import 'package:over_react_analyzer_plugin/src/analysis_options/error_severity_provider.dart';
 import 'package:over_react_analyzer_plugin/src/analysis_options/reader.dart';
+import 'package:over_react_analyzer_plugin/src/diagnostic/analyzer_debug_helper.dart';
 import 'package:over_react_analyzer_plugin/src/diagnostic_contributor.dart';
 import 'package:over_react_analyzer_plugin/src/error_filtering.dart';
+import 'package:over_react_analyzer_plugin/src/util/pretty_print.dart';
 
 mixin DiagnosticMixin on ServerPlugin {
-  final AnalysisOptionsReader _analysisOptionsReader = AnalysisOptionsReader();
+  PluginOptionsReader get pluginOptionsReader;
 
   List<DiagnosticContributor> getDiagnosticContributors(String path);
 
@@ -62,7 +65,7 @@ mixin DiagnosticMixin on ServerPlugin {
   /// Computes errors based on an analysis result, notifies the analyzer, and
   /// then returns the list of errors.
   Future<List<AnalysisError>> getAllErrors(ResolvedUnitResult analysisResult) async {
-    final analysisOptions = _analysisOptionsReader.getAnalysisOptionsForResult(analysisResult);
+    final analysisOptions = pluginOptionsReader.getOptionsForResult(analysisResult);
 
     try {
       // If there is no relevant analysis result, notify the analyzer of no errors.
@@ -93,7 +96,7 @@ mixin DiagnosticMixin on ServerPlugin {
   Future<plugin.EditGetFixesResult> handleEditGetFixes(plugin.EditGetFixesParams parameters) async {
     // We want request errors to propagate if they throw
     final request = await _getFixesRequest(parameters);
-    final analysisOptions = _analysisOptionsReader.getAnalysisOptionsForResult(request.result);
+    final analysisOptions = pluginOptionsReader.getOptionsForResult(request.result);
 
     try {
       final generator = _DiagnosticGenerator(
@@ -118,11 +121,11 @@ mixin DiagnosticMixin on ServerPlugin {
     return DartFixesRequestImpl(resourceProvider, offset, [], result);
   }
 
-  // from DartFixesMixin
+// from DartFixesMixin
 //  List<AnalysisError> _getErrors(int offset, ResolvedUnitResult result) {
 //    LineInfo lineInfo = result.lineInfo;
 //    int offsetLine = lineInfo.getLocation(offset).lineNumber;
-  // these errors don't include ones from the plugin, which doesn't seem right...
+// these errors don't include ones from the plugin, which doesn't seem right...
 //    return result.errors.where((AnalysisError error) {
 //      int errorLine = lineInfo.getLocation(error.offset).lineNumber;
 //      return errorLine == offsetLine;
@@ -135,6 +138,8 @@ mixin DiagnosticMixin on ServerPlugin {
 /// a given result unit or fixes request.
 @sealed
 class _DiagnosticGenerator {
+  static final _metricsDebugCommentPattern = getDebugCommentPattern('over_react_metrics');
+
   /// Initialize a newly created errors generator to use the given
   /// [contributors].
   _DiagnosticGenerator(this.contributors, {required ErrorSeverityProvider errorSeverityProvider})
@@ -203,15 +208,26 @@ class _DiagnosticGenerator {
     // TODO: collect data how long this takes.
     List<FluentComponentUsage> getUsages() => _usages ??= getAllComponentUsages(unitResult.unit!);
 
+    /// A mapping of diagnostic names to their durations, in microseconds.
+    final diagnosticMetrics = <String, int>{};
+
+    final metricsDebugFlagMatch = _metricsDebugCommentPattern.firstMatch(unitResult.content ?? '');
+
+    final totalStopwatch = Stopwatch()..start();
+    final disabledCheckStopwatch = Stopwatch()..start();
+
     for (final contributor in contributors) {
+      disabledCheckStopwatch.start();
       final isEveryCodeDisabled = contributor.codes.every(
         (e) => _errorSeverityProvider.isCodeDisabled(e.name),
       );
+      disabledCheckStopwatch.stop();
       if (isEveryCodeDisabled) {
         // Don't compute errors if all of the codes provided by the contributor are disabled
         continue;
       }
 
+      final contributorStopwatch = Stopwatch()..start();
       try {
         if (contributor is ComponentUsageDiagnosticContributor) {
           await contributor.computeErrorsForUsages(unitResult, collector, getUsages());
@@ -221,6 +237,31 @@ class _DiagnosticGenerator {
       } catch (exception, stackTrace) {
         notifications.add(PluginErrorParams(false, exception.toString(), stackTrace.toString()).toNotification());
       }
+      contributorStopwatch.stop();
+      if (metricsDebugFlagMatch != null) {
+        diagnosticMetrics[contributor.runtimeType.toString()] = contributorStopwatch.elapsedMicroseconds;
+      }
+    }
+
+    totalStopwatch.stop();
+    if (metricsDebugFlagMatch != null) {
+      String formatMicroseconds(int microseconds) =>
+          '${(microseconds / Duration.microsecondsPerMillisecond).toStringAsFixed(3)}ms';
+      String asPercentageOfTotal(int microseconds) =>
+          '${(microseconds / totalStopwatch.elapsedMicroseconds * 100).toStringAsFixed(1)}%';
+
+      final message = 'OverReact Analyzer Plugin diagnostic metrics (current file): ' +
+          prettyPrint(<String, String>{
+            ...diagnosticMetrics.map((name, microseconds) =>
+                MapEntry(name, '${formatMicroseconds(microseconds)} (${asPercentageOfTotal(microseconds)})')),
+            'Total': formatMicroseconds(totalStopwatch.elapsedMicroseconds),
+            'Diagnostic code disabled checks': formatMicroseconds(disabledCheckStopwatch.elapsedMicroseconds),
+            'Loop overhead (Total - SUM(Diagnostics))': formatMicroseconds(totalStopwatch.elapsedMicroseconds -
+                disabledCheckStopwatch.elapsedMicroseconds -
+                diagnosticMetrics.values.fold(0, (a, b) => a + b)),
+          });
+      AnalyzerDebugHelper(unitResult, collector, enabled: true).logWithLocation(
+          message, unitResult.location(offset: metricsDebugFlagMatch.start, end: metricsDebugFlagMatch.end));
     }
 
     final filteredErrors = _configureErrorSeverities(

tools/analyzer_plugin/lib/src/diagnostic/analyzer_debug_helper.dart

@@ -35,3 +35,14 @@ class AnalyzerDebugHelper {
     collector.addError(code, location, errorMessageArgs: [message]);
   }
 }
+
+/// Returns a pattern that matches a file with a `// debug:` comment with the given [debugCode].
+///
+/// Useful for conditionally enabling [AnalyzerDebugHelper] infos based on the presence of a comment
+/// in a file.
+///
+/// For example, `getDebugCommentPattern('foo')` will match files containing the following comments:
+///
+/// - `// debug: foo`
+/// - `//debug:foo,bar`
+RegExp getDebugCommentPattern(String debugCode) => RegExp(r'//\s*debug:.*\b' + RegExp.escape(debugCode) + r'\b');

tools/analyzer_plugin/lib/src/diagnostic/boilerplate_validator.dart

@@ -12,6 +12,8 @@ import 'package:over_react_analyzer_plugin/src/over_react_builder_parsing.dart'
 import 'package:over_react_analyzer_plugin/src/util/boilerplate_utils.dart';
 import 'package:source_span/source_span.dart';
 
+import 'analyzer_debug_helper.dart';
+
 const _errorDesc = 'Something is malformed in your component boilerplate.';
 // TODO: Add a more detailed description about the types of things our validator catches
 // <editor-fold desc="Error Documentation Details">
@@ -59,7 +61,7 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
   static final invalidGeneratedPartFixKind = FixKind(errorCode.name, 200, 'Fix generated part directive');
   static final unnecessaryGeneratedPartFixKind = FixKind(errorCode.name, 200, 'Remove generated part directive');
 
-  static final _debugFlagPattern = RegExp(r'debug:.*\bover_react_boilerplate\b');
+  static final _debugCommentPattern = getDebugCommentPattern('over_react_boilerplate');
 
   // TODO(nullsafety) come back and clean up fields
 
@@ -89,7 +91,7 @@ class BoilerplateValidatorDiagnostic extends DiagnosticContributor {
   /// Also returns whether the component has valid over_react declarations, which is useful in determining whether to
   /// validate the generated part directive.
   Future<bool> _computeBoilerplateErrors(ResolvedUnitResult result, DiagnosticCollector collector) async {
-    final debugMatch = _debugFlagPattern.firstMatch(result.content!);
+    final debugMatch = _debugCommentPattern.firstMatch(result.content!);
     final debug = debugMatch != null;
     if (debug) {
       collector.addError(debugCode, result.location(offset: debugMatch!.start, end: debugMatch.end),