Merge pull request #128 from Workiva/localized_element_selection_logi…

…c_to_symbol_generator FEDX-1213: Localized element selection logic to the SymbolGenerator
Workiva · Jul 9, 2024 · cc62ed7 · cc62ed7
2 parents 6b527bc + 64fd45f
commit cc62ed7
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 62 deletions.
diff --git a/lib/src/scip_visitor.dart b/lib/src/scip_visitor.dart
@@ -62,9 +62,8 @@ class ScipVisitor extends GeneralizingAstVisitor {
   }
 
   void _visitDeclaration(Declaration node) {
-    if (node.declaredElement == null) return;
-
-    final element = node.declaredElement!;
+    final element = _symbolGenerator.elementFor(node);
+    if (element == null) return;
 
     final relationships = relationshipsFor(node, element, _symbolGenerator);
 
@@ -76,17 +75,11 @@ class ScipVisitor extends GeneralizingAstVisitor {
   }
 
   void _visitNormalFormalParameter(NormalFormalParameter node) {
-    final element = node.declaredElement;
+    final element = _symbolGenerator.elementFor(node);
     if (element == null) return;
 
-    // if this parameter is a child of a GenericFunctionType (can be a
-    // typedef, or a function as a parameter), we don't want to index it
-    // as a definition (nothing is defined, just referenced). Return false
-    // and let the [_visitSimpleIdentifier] declare the reference
-    final parentParameter =
-        node.parent?.thisOrAncestorOfType<GenericFunctionType>();
-    if (parentParameter != null) return;
-
+    // if the parameter is a `this.someFieldOnThClass`, we need to register
+    // it as a reference to said field, as well as a declaration of a parameter.
     if (node is FieldFormalParameter) {
       final fieldElement = (element as FieldFormalParameterElement).field;
       _registerAsReference(
@@ -95,46 +88,19 @@ class ScipVisitor extends GeneralizingAstVisitor {
         offset: node.name.offset,
         length: node.name.length,
       );
-      return;
+
+      // non-named parameters are considered 'local' symbols, and when combined
+      // with field formal parameters (this.foo), do not contain a declaration.
+      // if its not named, do not register it as a definition as well.
+      if (!node.isNamed) return;
     }
 
     _registerAsDefinition(element, node);
   }
 
   void _visitSimpleIdentifier(SimpleIdentifier node) {
-    var element = node.staticElement;
-
-    // Both `.loadLibrary()`, and `.call()` are synthetic functions that
-    // have no definition. These should therefore should not be indexed.
-    if (element is FunctionElement && element.isSynthetic) {
-      if ([
-        FunctionElement.LOAD_LIBRARY_NAME,
-        FunctionElement.CALL_METHOD_NAME,
-      ].contains(element.name)) return;
-    }
-
-    // [element] for assignment fields is null. If the parent node
-    // is a `CompoundAssignmentExpression`, we know this node is referring
-    // to an assignment line. In that case, use the read/write element attached
-    // to this node instead of the [node]'s element
-    if (element == null) {
-      final assignmentExpr =
-          node.thisOrAncestorOfType<CompoundAssignmentExpression>();
-      if (assignmentExpr == null) return;
-
-      element = assignmentExpr.readElement ?? assignmentExpr.writeElement;
-    }
-
-    // When the identifier is a field, the analyzer creates synthetic getters/
-    // setters for it. We need to get the backing field.
-    if (element?.isSynthetic == true && element is PropertyAccessorElement) {
-      element = element.variable;
-    }
-
-    // element is null if there's nothing really to do for this node. Example: `void`
-    // TODO: One weird issue found: named parameters of external symbols were element.source
-    //       EX: `color(path, front: Styles.YELLOW);` where `color` comes from the chalk-dart package
-    if (element == null || element.source == null) return;
+    final element = _symbolGenerator.elementFor(node);
+    if (element == null) return;
 
     if (node.inDeclarationContext()) {
       _registerAsDefinition(element, node);
@@ -195,22 +161,22 @@ class ScipVisitor extends GeneralizingAstVisitor {
     List<Relationship>? relationships,
   }) {
     final symbol = _symbolGenerator.symbolFor(element);
-    if (symbol != null) {
-      final meta =
-          getSymbolMetadata(element, element.nameOffset, _analysisErrors);
-      symbols.add(SymbolInformation(
-        symbol: symbol,
-        documentation: meta.documentation,
-        relationships: relationships,
-        signatureDocumentation: meta.signatureDocumentation,
-      ));
+    if (symbol == null) return null;
 
-      occurrences.add(Occurrence(
-        range: _lineInfo.getRange(element.nameOffset, element.nameLength),
-        symbol: symbol,
-        symbolRoles: SymbolRole.Definition.value,
-        diagnostics: meta.diagnostics,
-      ));
-    }
+    final meta =
+        getSymbolMetadata(element, element.nameOffset, _analysisErrors);
+    symbols.add(SymbolInformation(
+      symbol: symbol,
+      documentation: meta.documentation,
+      relationships: relationships,
+      signatureDocumentation: meta.signatureDocumentation,
+    ));
+
+    occurrences.add(Occurrence(
+      range: _lineInfo.getRange(element.nameOffset, element.nameLength),
+      symbol: symbol,
+      symbolRoles: SymbolRole.Definition.value,
+      diagnostics: meta.diagnostics,
+    ));
   }
 }
diff --git a/lib/src/symbol_generator.dart b/lib/src/symbol_generator.dart
@@ -1,3 +1,4 @@
+import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:pubspec_parse/pubspec_parse.dart';
 import 'package:package_config/package_config.dart';
@@ -22,6 +23,75 @@ class SymbolGenerator {
 
   SymbolGenerator(this._packageConfig, this._pubspec);
 
+  /// For a given [AstNode], returns the correlating [Element] type
+  /// that should be used to generate the symbol
+  Element? elementFor(AstNode node) {
+    if (node is Declaration) {
+      return node.declaredElement;
+    } else if (node is NormalFormalParameter) {
+      // if this parameter is a child of a GenericFunctionType (can be a
+      // typedef, or a function as a parameter), we don't want to index it
+      // as a definition (nothing is defined, just referenced). Return false
+      // and let the [_visitSimpleIdentifier] declare the reference
+      final parentParameter =
+          node.parent?.thisOrAncestorOfType<GenericFunctionType>();
+      if (parentParameter != null) return null;
+
+      var element = node.declaredElement;
+      if (element == null) return null;
+
+      return element;
+    } else if (node is SimpleIdentifier) {
+      var element = node.staticElement;
+
+      // Both `.loadLibrary()`, and `.call()` are synthetic functions that
+      // have no definition. These should therefore should not be indexed.
+      if (element is FunctionElement && element.isSynthetic) {
+        if ([
+          FunctionElement.LOAD_LIBRARY_NAME,
+          FunctionElement.CALL_METHOD_NAME,
+        ].contains(element.name)) return null;
+      }
+
+      // [element] for assignment fields is null. If the parent node
+      // is a `CompoundAssignmentExpression`, we know this node is referring
+      // to an assignment line. In that case, use the read/write element attached
+      // to this node instead of the [node]'s element
+      if (element == null) {
+        final assignmentExpr =
+            node.thisOrAncestorOfType<CompoundAssignmentExpression>();
+        if (assignmentExpr == null) return null;
+
+        element = assignmentExpr.readElement ?? assignmentExpr.writeElement;
+      }
+
+      // When the identifier is a field, the analyzer creates synthetic getters/
+      // setters for it. We need to get the backing field.
+      if (element?.isSynthetic == true && element is PropertyAccessorElement) {
+        // The values field on enums is synthetic, and has no explicit definition like
+        // other fields do. Skip indexing for this case.
+        if (element.enclosingElement is EnumElement &&
+            element.name == 'values') {
+          return null;
+        }
+
+        element = element.variable;
+      }
+
+      // element is null if there's nothing really to do for this node. Example: `void`
+      // TODO: One weird issue found: named parameters of external symbols were element.source
+      //       EX: `color(path, front: Styles.YELLOW);` where `color` comes from the chalk-dart package
+      if (element?.source == null) return null;
+
+      return element;
+    }
+
+    display('WARN: Received unknown ast node type in elementFor: '
+        '${node.runtimeType} ($node). Skipping');
+
+    return null;
+  }
+
   /// For a given `Element` returns the scip symbol form.
   ///
   /// Returns [null] if symbol cannot be created for provided element

diff --git a/snapshots/input/basic-project/lib/more.dart b/snapshots/input/basic-project/lib/more.dart
@@ -20,6 +20,7 @@ class Animal with SleepMixin {
   SoundMaker? soundMaker;
 
   Animal(this.name, {required this.type}) {
+    print(AnimalType.values);
     switch (type) {
       case AnimalType.cat:
         soundMaker = () => print('Meow!');

diff --git a/snapshots/output/basic-project/lib/more.dart b/snapshots/output/basic-project/lib/more.dart
@@ -43,6 +43,10 @@
 //  ^^^^^^ reference scip-dart pub dart_test 1.0.0 lib/`more.dart`/Animal#
 //              ^^^^ reference scip-dart pub dart_test 1.0.0 lib/`more.dart`/Animal#name.
 //                                   ^^^^ reference scip-dart pub dart_test 1.0.0 lib/`more.dart`/Animal#type.
+//                                   ^^^^ definition scip-dart pub dart_test 1.0.0 lib/`more.dart`/Animal#`<constructor>`().(type)
+      print(AnimalType.values);
+//    ^^^^^ reference scip-dart pub dart:core 2.19.0 dart:core/`print.dart`/print().
+//          ^^^^^^^^^^ reference scip-dart pub dart_test 1.0.0 lib/`more.dart`/AnimalType#
       switch (type) {
 //            ^^^^ reference scip-dart pub dart_test 1.0.0 lib/`more.dart`/Animal#type.
         case AnimalType.cat:

diff --git a/snapshots/output/basic-project/lib/other.dart b/snapshots/output/basic-project/lib/other.dart
@@ -23,10 +23,13 @@
 //         ^^^^ reference local 0
       required this.value,
 //                  ^^^^^ reference scip-dart pub dart_test 1.0.0 lib/`other.dart`/Foo#value.
+//                  ^^^^^ definition scip-dart pub dart_test 1.0.0 lib/`other.dart`/Foo#`<constructor>`().(value)
       required this.value2,
 //                  ^^^^^^ reference scip-dart pub dart_test 1.0.0 lib/`other.dart`/Foo#value2.
+//                  ^^^^^^ definition scip-dart pub dart_test 1.0.0 lib/`other.dart`/Foo#`<constructor>`().(value2)
       this.value3,
 //         ^^^^^^ reference scip-dart pub dart_test 1.0.0 lib/`other.dart`/Foo#value3.
+//         ^^^^^^ definition scip-dart pub dart_test 1.0.0 lib/`other.dart`/Foo#`<constructor>`().(value3)
     }) {
       print(_far);
 //    ^^^^^ reference scip-dart pub dart:core 2.19.0 dart:core/`print.dart`/print().