Merge pull request #23 from Workiva/link-2020/iterator-key

Lint to warn users when elements within an iterable lacks a key

over_react_analyzer_plugin/lib/src/diagnostic/iterator_key.dart

@@ -0,0 +1,109 @@
+import 'package:analyzer/dart/analysis/results.dart';
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/syntactic_entity.dart';
+import 'package:over_react_analyzer_plugin/src/diagnostic_contributor.dart';
+import 'package:over_react_analyzer_plugin/src/fluent_interface_util.dart';
+import 'package:over_react_analyzer_plugin/src/util/util.dart';
+
+/// Warn when missing `key` props in iterators/collection literals
+class IteratorKey extends ComponentUsageDiagnosticContributor {
+  static const code = DiagnosticCode(
+    'over_react_missing_key',
+    'Missing "key" prop for element in iterator',
+    AnalysisErrorSeverity.WARNING,
+    AnalysisErrorType.STATIC_WARNING,
+  );
+
+  @override
+  computeErrorsForUsage(ResolvedUnitResult result, DiagnosticCollector collector, FluentComponentUsage usage) async {
+    final arguments = usage.node.argumentList.arguments;
+
+    for (final argument in arguments) {
+      if (argument is ListLiteral) {
+        // 1st case: Any element in a list literal w/o key
+        final list = argument;
+        for (final e in list.elements) {
+          if (e is! InvocationExpression) continue; // Don't need to lint non-elements
+
+          final componentUsage = getComponentUsage(e);
+          if (componentUsage == null) continue;
+          var elementHasKeyProp = _doesElementHaveKeyProp(componentUsage);
+
+          if (!elementHasKeyProp) {
+            // If current element in the list is missing a key prop, add warning & don't bother w/ remaining elements
+            collector.addError(
+              code,
+              result.locationFor(e),
+            );
+          }
+        }
+      } else if (argument is MethodInvocation) {
+        // 2nd case: Element mapping
+        // Look through all method invocations (e.g. .map.toList()) until you find the mapping function
+        MethodInvocation mapStatement;
+        final invokedMethods = _buildInvocationList(argument);
+        for (final method in invokedMethods) {
+          if (method.methodName.name == 'map') {
+            mapStatement = method;
+          }
+        }
+        // If there's no `.map`, there's no elements returned, so nothing to lint for this arg
+        if (mapStatement == null) continue;
+
+        // Get the top level element that's being returned from the map
+        final mapStatementFuncArg = mapStatement.argumentList.arguments.firstOrNull?.tryCast<FunctionExpression>();
+        if (mapStatementFuncArg == null) continue;
+
+        final body = mapStatementFuncArg.body;
+
+        final returnExpression = body?.tryCast<ExpressionFunctionBody>()?.expression ??
+            body
+                ?.tryCast<BlockFunctionBody>()
+                ?.block
+                ?.statements
+                ?.whereType<ReturnStatement>()
+                ?.firstOrNull
+                ?.expression;
+        if (returnExpression is! InvocationExpression) continue; // Don't need to lint non-elements
+
+        final componentUsage = getComponentUsage(returnExpression);
+        if (componentUsage == null) continue;
+
+        var elementHasKeyProp = _doesElementHaveKeyProp(componentUsage);
+
+        if (!elementHasKeyProp) {
+          collector.addError(
+            code,
+            result.locationFor(returnExpression),
+          );
+        }
+      }
+    }
+  }
+
+  bool _doesElementHaveKeyProp(FluentComponentUsage element) {
+    var elementHasKeyProp = false;
+    forEachCascadedProp(element, (lhs, rhs) {
+      if (lhs.propertyName.name == 'key') {
+        elementHasKeyProp = true;
+      }
+    });
+
+    return elementHasKeyProp;
+  }
+
+  List<MethodInvocation> _buildInvocationList(MethodInvocation method) {
+    // A list of all the methods that could possibly be chained to the input method
+    final methodsInvoked = <MethodInvocation>[method];
+    dynamic target = method.target;
+    while (target != null) {
+      if (target is MethodInvocation) {
+        methodsInvoked.add(target);
+        target = target.target;
+      } else {
+        return methodsInvoked;
+      }
+    }
+    return methodsInvoked;
+  }
+}

over_react_analyzer_plugin/lib/src/plugin.dart

@@ -61,6 +61,7 @@ import 'package:over_react_analyzer_plugin/src/diagnostic/incorrect_doc_comment_
 import 'package:over_react_analyzer_plugin/src/diagnostic/dom_prop_types.dart';
 import 'package:over_react_analyzer_plugin/src/diagnostic/duplicate_prop_cascade.dart';
 import 'package:over_react_analyzer_plugin/src/diagnostic/invalid_child.dart';
+import 'package:over_react_analyzer_plugin/src/diagnostic/iterator_key.dart';
 import 'package:over_react_analyzer_plugin/src/diagnostic/missing_cascade_parens.dart';
 import 'package:over_react_analyzer_plugin/src/diagnostic/missing_required_prop.dart';
 import 'package:over_react_analyzer_plugin/src/diagnostic/pseudo_static_lifecycle.dart';
@@ -166,6 +167,7 @@ class OverReactAnalyzerPlugin extends ServerPlugin
       IncorrectDocCommentLocationDiagnostic(),
       ConsumedPropsReturnValueDiagnostic(),
       ForwardOnlyDomPropsToDomBuildersDiagnostic(),
+      IteratorKey(),
     ];
   }
 }

playground/web/iterator_key.dart

@@ -0,0 +1,40 @@
+import 'package:over_react/over_react.dart';
+
+part 'iterator_key.over_react.g.dart';
+
+ReactElement MissingKeyInList() {
+  return (
+      Dom.div()(
+        // Should not throw key warning, all elems have key
+        [(Dom.div()..key='')(), (Dom.div()..key='')()],
+
+        // Raw strings don't need keys, no worries!
+        'b',
+
+        // Should throw key warning, all elements in list should have key
+        ['', Dom.div()()],
+        [Dom.p()(), Dom.p()()],
+        [(Dom.div()..key='')(), Dom.div()(), Dom.div()()],
+      ));
+}
+
+ReactElement MissingKeyInMap() {
+  List coolStrings = ['cool', 'cooler', '0deg kelvin'];
+
+  return (
+      Dom.div()(
+        // Raw strings don't need keys, no worries!
+        coolStrings.map((s) => s),
+
+        // Should warn, Dom.div() needs a key!
+        coolStrings.map((s) => Dom.div()(s)),
+        coolStrings.map((s) => Dom.div()(s)).toList(),
+        coolStrings.map((s) => (Dom.div())(
+          Dom.p()(s),
+        )),
+
+        coolStrings.map((s) => (Dom.div()..key='bruh')( // Should not warn
+          [Dom.p()(s), Dom.p()()], // Should warn
+        )),
+      ));
+}