diff --git a/CHANGELOG.md b/CHANGELOG.md index 3979f04abf..6248f580c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * **Breaking Change:** cli arguments `--fatal-unused` and `--fatal-warnings` activate by default. * chore: restrict `analyzer` version to `>=2.8.0 <3.3.0`. +* feat: add static code diagnostic `avoid-collection-methods-with-unrelated-types` ## 4.11.0 diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart b/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart index 7e9cba5f06..c45012da33 100644 --- a/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart +++ b/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart @@ -1,6 +1,7 @@ import 'models/rule.dart'; import 'rules_list/always_remove_listener/always_remove_listener_rule.dart'; import 'rules_list/avoid_border_all/avoid_border_all_rule.dart'; +import 'rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule.dart'; import 'rules_list/avoid_dynamic/avoid_dynamic_rule.dart'; import 'rules_list/avoid_global_state/avoid_global_state_rule.dart'; import 'rules_list/avoid_ignoring_return_values/avoid_ignoring_return_values_rule.dart'; @@ -48,6 +49,8 @@ import 'rules_list/provide_correct_intl_args/provide_correct_intl_args_rule.dart final _implementedRules = )>{ AlwaysRemoveListenerRule.ruleId: (config) => AlwaysRemoveListenerRule(config), AvoidBorderAllRule.ruleId: (config) => AvoidBorderAllRule(config), + AvoidCollectionMethodsWithUnrelatedTypesRule.ruleId: (config) => + AvoidCollectionMethodsWithUnrelatedTypesRule(config), AvoidDynamicRule.ruleId: (config) => AvoidDynamicRule(config), AvoidGlobalStateRule.ruleId: (config) => AvoidGlobalStateRule(config), AvoidIgnoringReturnValuesRule.ruleId: (config) => diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule.dart b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule.dart new file mode 100644 index 0000000000..7f82b05727 --- /dev/null +++ b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule.dart @@ -0,0 +1,50 @@ +// ignore_for_file: public_member_api_docs + +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:collection/collection.dart'; + +import '../../../../../utils/dart_types_utils.dart'; +import '../../../../../utils/node_utils.dart'; +import '../../../lint_utils.dart'; +import '../../../models/internal_resolved_unit_result.dart'; +import '../../../models/issue.dart'; +import '../../../models/severity.dart'; +import '../../models/common_rule.dart'; +import '../../rule_utils.dart'; + +part 'visitor.dart'; + +class AvoidCollectionMethodsWithUnrelatedTypesRule extends CommonRule { + static const String ruleId = 'avoid-collection-methods-with-unrelated-types'; + + static const _warning = 'Avoid collection methods with unrelated types.'; + + AvoidCollectionMethodsWithUnrelatedTypesRule( + [Map config = const {}]) + : super( + id: ruleId, + severity: readSeverity(config, Severity.warning), + excludes: readExcludes(config), + ); + + @override + Iterable check(InternalResolvedUnitResult source) { + final visitor = _Visitor(); + + source.unit.visitChildren(visitor); + + return visitor.expressions + .map((expression) => createIssue( + rule: this, + location: nodeLocation( + node: expression, + source: source, + ), + message: _warning, + )) + .toList(growable: false); + } +} diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/visitor.dart b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/visitor.dart new file mode 100644 index 0000000000..9469595398 --- /dev/null +++ b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/visitor.dart @@ -0,0 +1,112 @@ +part of 'avoid_collection_methods_with_unrelated_types_rule.dart'; + +class _Visitor extends RecursiveAstVisitor { + final _expressions = []; + + Iterable get expressions => _expressions; + + // for `operator []` and `operator []=` + @override + void visitIndexExpression(IndexExpression node) { + super.visitIndexExpression(node); + + final mapType = _getMapTypeElement(node.target?.staticType); + _addIfNotSubType(node.index.staticType, mapType?.first, node); + } + + // for things like `map.containsKey` + @override + void visitMethodInvocation(MethodInvocation node) { + super.visitMethodInvocation(node); + + final mapType = _getMapTypeElement(node.target?.staticType); + final listType = _getListTypeElement(node.target?.staticType); + final setType = _getSetTypeElement(node.target?.staticType); + final iterableType = _getIterableTypeElement(node.target?.staticType); + final argType = node.argumentList.arguments.singleOrNull?.staticType; + + switch (node.methodName.name) { + case 'containsKey': + _addIfNotSubType(argType, mapType?.first, node); + break; + case 'remove': + _addIfNotSubType(argType, mapType?.first, node); + _addIfNotSubType(argType, listType, node); + _addIfNotSubType(argType, setType, node); + break; + case 'lookup': + _addIfNotSubType(argType, setType, node); + break; + case 'containsValue': + _addIfNotSubType(argType, mapType?[1], node); + break; + case 'contains': + _addIfNotSubType(argType, iterableType, node); + break; + case 'containsAll': + case 'removeAll': + case 'retainAll': + final argAsIterableParamType = _getIterableTypeElement(argType); + _addIfNotSubType(argAsIterableParamType?.type, setType, node); + break; + case 'difference': + case 'intersection': + final argAsSetParamType = _getSetTypeElement(argType); + _addIfNotSubType(argAsSetParamType?.type, setType, node); + break; + } + } + + void _addIfNotSubType( + DartType? childType, + _TypedClassElement? parentElement, + Expression node, + ) { + if (parentElement != null && + childType != null && + childType.asInstanceOf(parentElement.element) == null) { + _expressions.add(node); + } + } + + List<_TypedClassElement>? _getMapTypeElement(DartType? type) => + _getTypeArgElements(getSupertypeMap(type)); + + _TypedClassElement? _getIterableTypeElement(DartType? type) => + _getTypeArgElements(getSupertypeIterable(type))?.singleOrNull; + + _TypedClassElement? _getListTypeElement(DartType? type) => + _getTypeArgElements(getSupertypeList(type))?.singleOrNull; + + _TypedClassElement? _getSetTypeElement(DartType? type) => + _getTypeArgElements(getSupertypeSet(type))?.singleOrNull; + + List<_TypedClassElement>? _getTypeArgElements(DartType? type) { + if (type == null || type is! ParameterizedType) { + return null; + } + + final typeArgElements = type.typeArguments + .map((typeArg) { + final element = typeArg.element; + + return element is ClassElement + ? _TypedClassElement(typeArg, element) + : null; + }) + .whereNotNull() + .toList(); + if (typeArgElements.length < type.typeArguments.length) { + return null; + } + + return typeArgElements; + } +} + +class _TypedClassElement { + final DartType type; + final ClassElement element; + + _TypedClassElement(this.type, this.element); +} diff --git a/lib/src/utils/dart_types_utils.dart b/lib/src/utils/dart_types_utils.dart index 1e53ea372c..fee5c179ee 100644 --- a/lib/src/utils/dart_types_utils.dart +++ b/lib/src/utils/dart_types_utils.dart @@ -1,19 +1,46 @@ // ignore_for_file: public_member_api_docs import 'package:analyzer/dart/element/type.dart'; +import 'package:collection/collection.dart'; bool isIterableOrSubclass(DartType? type) => - _isIterable(type) || _isSubclassOfIterable(type); + _checkSelfOrSupertypes(type, (t) => t?.isDartCoreIterable ?? false); -bool _isIterable(DartType? type) => type?.isDartCoreIterable ?? false; +bool isListOrSubclass(DartType? type) => + _checkSelfOrSupertypes(type, (t) => t?.isDartCoreList ?? false); -bool _isSubclassOfIterable(DartType? type) => - type is InterfaceType && type.allSupertypes.any(_isIterable); +bool isMapOrSubclass(DartType? type) => + _checkSelfOrSupertypes(type, (t) => t?.isDartCoreMap ?? false); -bool isListOrSubclass(DartType? type) => - _isList(type) || _isSubclassOfList(type); +DartType? getSupertypeIterable(DartType? type) => + _getSelfOrSupertypes(type, (t) => t?.isDartCoreIterable ?? false); + +DartType? getSupertypeList(DartType? type) => + _getSelfOrSupertypes(type, (t) => t?.isDartCoreList ?? false); + +DartType? getSupertypeSet(DartType? type) => + _getSelfOrSupertypes(type, (t) => t?.isDartCoreSet ?? false); + +DartType? getSupertypeMap(DartType? type) => + _getSelfOrSupertypes(type, (t) => t?.isDartCoreMap ?? false); + +bool _checkSelfOrSupertypes( + DartType? type, + bool Function(DartType?) predicate, +) => + predicate(type) || + (type is InterfaceType && type.allSupertypes.any(predicate)); -bool _isList(DartType? type) => type?.isDartCoreList ?? false; +DartType? _getSelfOrSupertypes( + DartType? type, + bool Function(DartType?) predicate, +) { + if (predicate(type)) { + return type; + } + if (type is InterfaceType) { + return type.allSupertypes.firstWhereOrNull(predicate); + } -bool _isSubclassOfList(DartType? type) => - type is InterfaceType && type.allSupertypes.any(_isList); + return null; +} diff --git a/test/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule_test.dart b/test/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule_test.dart new file mode 100644 index 0000000000..1925c8babb --- /dev/null +++ b/test/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule_test.dart @@ -0,0 +1,136 @@ +import 'package:dart_code_metrics/src/analyzers/lint_analyzer/models/severity.dart'; +import 'package:dart_code_metrics/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/avoid_collection_methods_with_unrelated_types_rule.dart'; +import 'package:test/test.dart'; + +import '../../../../../helpers/rule_test_helper.dart'; + +const _examplePath = + 'avoid_collection_methods_with_unrelated_types/examples/example.dart'; + +void main() { + group('AvoidCollectionMethodsWithUnrelatedTypesRule', () { + test('initialization', () async { + final unit = await RuleTestHelper.resolveFromFile(_examplePath); + final issues = AvoidCollectionMethodsWithUnrelatedTypesRule().check(unit); + + RuleTestHelper.verifyInitialization( + issues: issues, + ruleId: 'avoid-collection-methods-with-unrelated-types', + severity: Severity.warning, + ); + }); + + test('reports about found issues', () async { + final unit = await RuleTestHelper.resolveFromFile(_examplePath); + final issues = AvoidCollectionMethodsWithUnrelatedTypesRule().check(unit); + + RuleTestHelper.verifyIssues( + issues: issues, + startLines: [ + 6, + 9, + 12, + 15, + 18, + 27, + 32, + 35, + 38, + 41, + 48, + 50, + 55, + 58, + 61, + 67, + 74, + 77, + 80, + 83, + 86, + 89, + 92, + 95, + ], + startColumns: [ + 5, + 16, + 5, + 5, + 5, + 5, + 16, + 5, + 5, + 5, + 14, + 5, + 5, + 5, + 5, + 5, + 5, + 5, + 5, + 5, + 5, + 5, + 5, + 5, + ], + locationTexts: [ + 'primitiveMap["str"]', + 'primitiveMap["str"]', + 'primitiveMap.containsKey("str")', + 'primitiveMap.containsValue(100)', + 'primitiveMap.remove("str")', + 'inheritanceMap[Flower()]', + 'inheritanceMap[Flower()]', + 'inheritanceMap.containsKey(Flower())', + 'inheritanceMap.containsValue(DogImplementsAnimal())', + 'inheritanceMap.remove(Flower())', + 'myMap["str"]', + 'myMap.containsKey("str")', + '[1, 2, 3].contains("str")', + 'Iterable.generate(10).contains("str")', + '{1, 2, 3}.contains("str")', + 'primitiveList.remove("str")', + 'primitiveSet.contains("str")', + 'primitiveSet.containsAll(Iterable.empty())', + 'primitiveSet.difference({})', + 'primitiveSet.intersection({})', + 'primitiveSet.lookup("str")', + 'primitiveSet.remove("str")', + 'primitiveSet.removeAll(Iterable.empty())', + 'primitiveSet.retainAll(Iterable.empty())', + ], + messages: [ + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + 'Avoid collection methods with unrelated types.', + ], + ); + }); + }); +} diff --git a/test/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/examples/example.dart b/test/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/examples/example.dart new file mode 100644 index 0000000000..9f2c62c726 --- /dev/null +++ b/test/src/analyzers/lint_analyzer/rules/rules_list/avoid_collection_methods_with_unrelated_types/examples/example.dart @@ -0,0 +1,107 @@ +void main() { + { + final primitiveMap = Map(); + + primitiveMap[42] = "value"; + primitiveMap["str"] = "value"; // LINT + + final a1 = primitiveMap[42]; + final a2 = primitiveMap["str"]; // LINT + + primitiveMap.containsKey(42); + primitiveMap.containsKey("str"); // LINT + + primitiveMap.containsValue("value"); + primitiveMap.containsValue(100); // LINT + + primitiveMap.remove(42); + primitiveMap.remove("str"); // LINT + } + + { + final inheritanceMap = Map(); + + inheritanceMap[Animal()] = "value"; + inheritanceMap[HumanExtendsAnimal()] = "value"; + inheritanceMap[DogImplementsAnimal()] = "value"; + inheritanceMap[Flower()] = "value"; // LINT + + final b1 = inheritanceMap[Animal()]; + final b2 = inheritanceMap[HumanExtendsAnimal()]; + final b3 = inheritanceMap[DogImplementsAnimal()]; + final b4 = inheritanceMap[Flower()]; // LINT + + inheritanceMap.containsKey(DogImplementsAnimal()); + inheritanceMap.containsKey(Flower()); // LINT + + inheritanceMap.containsValue(Flower()); + inheritanceMap.containsValue(DogImplementsAnimal()); // LINT + + inheritanceMap.remove(DogImplementsAnimal()); + inheritanceMap.remove(Flower()); // LINT + } + + // sub-class of `Map`, `Iterable` etc should also be supported + { + final myMap = MyMapSubClass(); + var c1 = myMap[42]; + var c2 = myMap["str"]; // LINT + myMap.containsKey(42); + myMap.containsKey("str"); // LINT + } + + { + [1, 2, 3].contains(42); + [1, 2, 3].contains("str"); // LINT + + Iterable.generate(10).contains(42); + Iterable.generate(10).contains("str"); // LINT + + {1, 2, 3}.contains(42); + {1, 2, 3}.contains("str"); // LINT + } + + { + final primitiveList = [10, 20, 30]; + primitiveList.remove(20); + primitiveList.remove("str"); // LINT + } + + { + final primitiveSet = {10, 20, 30}; + + primitiveSet.contains(42); + primitiveSet.contains("str"); // LINT + + primitiveSet.containsAll(Iterable.empty()); + primitiveSet.containsAll(Iterable.empty()); // LINT + + primitiveSet.difference({}); + primitiveSet.difference({}); // LINT + + primitiveSet.intersection({}); + primitiveSet.intersection({}); // LINT + + primitiveSet.lookup(42); + primitiveSet.lookup("str"); // LINT + + primitiveSet.remove(42); + primitiveSet.remove("str"); // LINT + + primitiveSet.removeAll(Iterable.empty()); + primitiveSet.removeAll(Iterable.empty()); // LINT + + primitiveSet.retainAll(Iterable.empty()); + primitiveSet.retainAll(Iterable.empty()); // LINT + } +} + +class Animal {} + +class HumanExtendsAnimal extends Animal {} + +class DogImplementsAnimal implements Animal {} + +class Flower {} + +class MyMapSubClass extends Map {} diff --git a/website/docs/rules/common/avoid-collection-methods-with-unrelated-types.md b/website/docs/rules/common/avoid-collection-methods-with-unrelated-types.md new file mode 100644 index 0000000000..e0f932a146 --- /dev/null +++ b/website/docs/rules/common/avoid-collection-methods-with-unrelated-types.md @@ -0,0 +1,69 @@ +# Avoid collection methods with unrelated types + +## Rule id {#rule-id} + +avoid-collection-methods-with-unrelated-types + +## Severity {#severity} + +Warning + +## Description {#description} + +Avoid using collection methods with unrelated types, such as accessing a map of integers using a string key. + +This lint has been requested for a long time: Follow [this link](https://github.com/dart-lang/linter/issues/1307) to see the details. + +Related: Dart's built-in `list_remove_unrelated_type` and `iterable_contains_unrelated_type`. + +### Example {#example} + +Bad: + +```dart +final map = Map(); +map["str"] = "value"; // LINT +var a = map["str"]; // LINT +map.containsKey("str"); // LINT +map.containsValue(42); // LINT +map.remove("str"); // LINT + +Iterable.empty().contains("str"); // LINT + +List().remove("str"); // LINT + +final set = {10, 20, 30}; +set.contains("str"); // LINT +set.containsAll(Iterable.empty()); // LINT +set.difference({}); // LINT +primitiveSet.intersection({}); // LINT +set.lookup("str"); // LINT +primitiveList.remove("str"); // LINT +set.removeAll(Iterable.empty()); // LINT +set.retainAll(Iterable.empty()); // LINT +``` + +Good: + +```dart +final map = Map(); +map[42] = "value"; +var a = map[42]; +map.containsKey(42); +map.containsValue("value"); +map.remove(42); + +Iterable.empty().contains(42); + +List().remove(42); + +final set = {10, 20, 30}; +set.contains(42); +set.containsAll(Iterable.empty()); +set.difference({}); +primitiveSet.intersection({}); +set.lookup(42); +primitiveList.remove(42); +set.removeAll(Iterable.empty()); +set.retainAll(Iterable.empty()); +``` diff --git a/website/docs/rules/overview.md b/website/docs/rules/overview.md index 6c64b1dd4e..c2956e4508 100644 --- a/website/docs/rules/overview.md +++ b/website/docs/rules/overview.md @@ -11,6 +11,10 @@ Rules configuration is [described here](../getting-started/configuration#configu ## Common {#common} +- [avoid-collection-methods-with-unrelated-types](./common/avoid-collection-methods-with-unrelated-types.md) + + Avoid using collection methods with unrelated types, such as accessing a map of integers using a string key. + - [avoid-dynamic](./common/avoid-dynamic.md) Warns when `dynamic` type is used as variable type in declaration, return type of a function, etc.