diff --git a/over_react_analyzer_plugin/lib/src/diagnostic/iterator_key.dart b/over_react_analyzer_plugin/lib/src/diagnostic/iterator_key.dart new file mode 100644 index 000000000..9062d291f --- /dev/null +++ b/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; + } +} diff --git a/over_react_analyzer_plugin/lib/src/plugin.dart b/over_react_analyzer_plugin/lib/src/plugin.dart index c3c1a2e12..b9a14e7e8 100644 --- a/over_react_analyzer_plugin/lib/src/plugin.dart +++ b/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(), ]; } } diff --git a/playground/web/iterator_key.dart b/playground/web/iterator_key.dart new file mode 100644 index 000000000..9dc23eb5e --- /dev/null +++ b/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 + )), + )); +}