diff --git a/packages/leancode_lint/CHANGELOG.md b/packages/leancode_lint/CHANGELOG.md index 1ea7f1df..daf14e58 100644 --- a/packages/leancode_lint/CHANGELOG.md +++ b/packages/leancode_lint/CHANGELOG.md @@ -1,6 +1,7 @@ # Unreleased - Add new custom lints: + - [`avoid_direct_collection_equality_checks`](https://github.com/leancodepl/flutter_corelibrary/tree/master/packages/leancode_lint#avoid_direct_collection_equality_checks) - [`avoid_context_read_in_build`](https://github.com/leancodepl/flutter_corelibrary/tree/master/packages/leancode_lint#avoid_context_read_in_build) - [`avoid_build_context_in_blocs`](https://github.com/leancodepl/flutter_corelibrary/tree/master/packages/leancode_lint#avoid_build_context_in_blocs) diff --git a/packages/leancode_lint/README.md b/packages/leancode_lint/README.md index 595cb982..a28f7042 100644 --- a/packages/leancode_lint/README.md +++ b/packages/leancode_lint/README.md @@ -367,6 +367,68 @@ None. +
+avoid_direct_collection_equality_checks + +### `avoid_direct_collection_equality_checks` + +**AVOID** comparing collections directly with `==` or `!=`. + +For `List`, `Set`, and `Map`, `==` compares identity (reference equality), not +contents, so `[1, 2] == [1, 2]` is `false`. Use a content-equality helper +instead. + +The rule offers three quick fixes: one rewriting the comparison to Flutter's +`listEquals`/`setEquals`/`mapEquals` (from `package:flutter/foundation.dart`), +one to `package:collection`'s `ListEquality`/`SetEquality`/`MapEquality`, and one +to `identical` for the cases where an identity comparison is actually intended. +The content-equality fixes add the required import, so each is only offered when +the package owning the analyzed file declares a direct dependency on the package +it imports (`flutter` and `collection` respectively). All fixes negate the result for +`!=`. + +**BAD:** + +```dart +bool sameItems(List a, List b) { + return a == b; +} +``` + +**GOOD:** + +```dart +import 'package:flutter/foundation.dart'; + +bool sameItems(List a, List b) { + return listEquals(a, b); +} +``` + +**GOOD:** + +```dart +import 'package:collection/collection.dart'; + +bool sameItems(List a, List b) { + return const ListEquality().equals(a, b); +} +``` + +**GOOD:** + +```dart +bool isSameList(List a, List b) { + return identical(a, b); +} +``` + +#### Configuration + +None. + +
+
bloc_related_class_naming @@ -1102,6 +1164,7 @@ See linked source code containing explanation in dart doc. --- ## 🛠️ Maintained by LeanCode +
[LeanCode Logo][leancode-landing] diff --git a/packages/leancode_lint/lib/plugin.dart b/packages/leancode_lint/lib/plugin.dart index bf2c4952..e1e1f88d 100644 --- a/packages/leancode_lint/lib/plugin.dart +++ b/packages/leancode_lint/lib/plugin.dart @@ -9,6 +9,7 @@ import 'package:leancode_lint/src/lints/avoid_build_context_in_blocs.dart'; import 'package:leancode_lint/src/lints/avoid_catch_error.dart'; import 'package:leancode_lint/src/lints/avoid_conditional_hooks.dart'; import 'package:leancode_lint/src/lints/avoid_context_read_in_build.dart'; +import 'package:leancode_lint/src/lints/avoid_direct_collection_equality_checks.dart'; import 'package:leancode_lint/src/lints/avoid_single_child_in_multi_child_widget.dart'; import 'package:leancode_lint/src/lints/bloc_related_class_naming.dart'; import 'package:leancode_lint/src/lints/bloc_subclasses_naming.dart'; @@ -68,6 +69,19 @@ final class LeanCodeLintPlugin extends Plugin { ..registerWarningRule(AvoidCatchError()) ..registerWarningRule(AvoidBuildContextInBlocs()) ..registerWarningRule(AvoidConditionalHooks()) + ..registerWarningRule(AvoidDirectCollectionEqualityChecks()) + ..registerFixForRule( + AvoidDirectCollectionEqualityChecks.code, + ReplaceWithFlutterFoundationEqualsFix.new, + ) + ..registerFixForRule( + AvoidDirectCollectionEqualityChecks.code, + ReplaceWithCollectionPackageEqualityFix.new, + ) + ..registerFixForRule( + AvoidDirectCollectionEqualityChecks.code, + ReplaceWithIdenticalFix.new, + ) ..registerWarningRule(HookWidgetDoesNotUseHooks()) ..registerFixForRule( HookWidgetDoesNotUseHooks.code, diff --git a/packages/leancode_lint/lib/src/helpers.dart b/packages/leancode_lint/lib/src/helpers.dart index 296106d8..c7c0fa4a 100644 --- a/packages/leancode_lint/lib/src/helpers.dart +++ b/packages/leancode_lint/lib/src/helpers.dart @@ -1,15 +1,23 @@ import 'package:analysis_server_plugin/edit/dart/correction_producer.dart'; import 'package:analysis_server_plugin/edit/dart/dart_fix_kind_priority.dart'; import 'package:analyzer/analysis_rule/analysis_rule.dart'; +import 'package:analyzer/analysis_rule/pubspec.dart'; import 'package:analyzer/analysis_rule/rule_visitor_registry.dart'; import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/token.dart'; import 'package:analyzer/dart/ast/visitor.dart'; +// `PubPackage`, the only way to reach the parsed pubspec of the package owning +// a file, has no public equivalent. The SDK's own +// `depend_on_referenced_packages` reaches for it the same way. +// ignore: implementation_imports +import 'package:analyzer/src/workspace/pub.dart'; +import 'package:analyzer/workspace/workspace.dart'; import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart'; import 'package:analyzer_plugin/utilities/fixes/fixes.dart'; import 'package:analyzer_plugin/utilities/range_factory.dart'; import 'package:leancode_lint/src/type_checker.dart'; import 'package:leancode_lint/src/utils.dart'; +import 'package:meta/meta.dart'; String typeParametersString( Iterable typeParameters, { @@ -252,6 +260,59 @@ class _HookWidgetBodyVisitor extends SimpleAstVisitor { } } +extension PackageDependencies on ResolvedCorrectionProducer { + bool dependsOnPackage(String packageName) => packageDependsOn( + packageName, + package: sessionHelper.session.analysisContext.contextRoot.workspace + .findPackageFor(file), + filePath: file, + ); +} + +/// Whether [package] declares a direct dependency on [packageName], as seen +/// from the file at [filePath]. +/// +/// A transitive dependency does not count: an import of it resolves today, but +/// breaks as soon as the intermediate package stops depending on +/// [packageName]. The package config cannot tell the two apart, hence the +/// pubspec. Dev dependencies count only outside the package's public +/// directories, since code that ships to consumers cannot rely on them. +/// +/// This mirrors how the SDK's `depend_on_referenced_packages` answers the very +/// same question. +@visibleForTesting +bool packageDependsOn( + String packageName, { + required WorkspacePackage? package, + required String filePath, +}) { + if (package is! PubPackage) { + return false; + } + final pubspec = package.pubspec; + if (pubspec == null) { + return false; + } + + bool declares(Iterable? dependencies) => + dependencies?.any((dep) => dep.name?.text == packageName) ?? false; + + return declares(pubspec.dependencies) || + (!_isInPublicDir(filePath, package) && declares(pubspec.devDependencies)); +} + +/// Mirrors `isInPublicDir` from the SDK's linter. +bool _isInPublicDir(String filePath, WorkspacePackage package) { + final pathContext = package.root.provider.pathContext; + String inRoot(List parts) => + pathContext.joinAll([package.root.path, ...parts]); + + return pathContext.isWithin(inRoot(['lib']), filePath) || + pathContext.isWithin(inRoot(['bin']), filePath) || + filePath == inRoot(['hook', 'build.dart']) || + filePath == inRoot(['hook', 'link.dart']); +} + bool isExpressionExactlyType( Expression expression, String typeName, diff --git a/packages/leancode_lint/lib/src/lints/avoid_direct_collection_equality_checks.dart b/packages/leancode_lint/lib/src/lints/avoid_direct_collection_equality_checks.dart new file mode 100644 index 00000000..26a68388 --- /dev/null +++ b/packages/leancode_lint/lib/src/lints/avoid_direct_collection_equality_checks.dart @@ -0,0 +1,282 @@ +import 'package:analysis_server_plugin/edit/dart/correction_producer.dart'; +import 'package:analysis_server_plugin/edit/dart/dart_fix_kind_priority.dart'; +import 'package:analyzer/analysis_rule/analysis_rule.dart'; +import 'package:analyzer/analysis_rule/rule_context.dart'; +import 'package:analyzer/analysis_rule/rule_visitor_registry.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/token.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:analyzer/error/error.dart'; +import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart'; +import 'package:analyzer_plugin/utilities/fixes/fixes.dart'; +import 'package:analyzer_plugin/utilities/range_factory.dart'; +import 'package:leancode_lint/src/helpers.dart'; + +class AvoidDirectCollectionEqualityChecks extends AnalysisRule { + AvoidDirectCollectionEqualityChecks() + : super(name: code.lowerCaseName, description: code.problemMessage); + + static const code = LintCode( + 'avoid_direct_collection_equality_checks', + 'Avoid comparing {0}s directly with `==` or `!=`. This compares identity, not contents.', + correctionMessage: + 'Use `{1}` or `const {2}().equals` to compare contents, or `identical` if an identity check is intended.', + severity: .WARNING, + ); + + @override + LintCode get diagnosticCode => code; + + @override + void registerNodeProcessors( + RuleVisitorRegistry registry, + RuleContext context, + ) { + registry.addBinaryExpression(this, _Visitor(this)); + } +} + +class _Visitor extends SimpleAstVisitor { + _Visitor(this.rule); + + final AnalysisRule rule; + + @override + void visitBinaryExpression(BinaryExpression node) { + final operator = node.operator.type; + if (operator != TokenType.EQ_EQ && operator != TokenType.BANG_EQ) { + return; + } + + final leftKind = collectionKind(node.leftOperand.staticType); + final rightKind = collectionKind(node.rightOperand.staticType); + if (leftKind == null || leftKind != rightKind) { + return; + } + + rule.reportAtNode( + node, + arguments: [ + leftKind.displayName, + leftKind.flutterFunction, + leftKind.collectionClass, + ], + ); + } +} + +enum CollectionKind { + list('List', flutterFunction: 'listEquals', collectionClass: 'ListEquality'), + set('Set', flutterFunction: 'setEquals', collectionClass: 'SetEquality'), + map('Map', flutterFunction: 'mapEquals', collectionClass: 'MapEquality'); + + const CollectionKind( + this.displayName, { + required this.flutterFunction, + required this.collectionClass, + }); + + final String displayName; + + /// From `package:flutter/foundation.dart`. + final String flutterFunction; + + /// From `package:collection`. + final String collectionClass; +} + +/// Also matches subtypes of `List`, `Set` and `Map`. +CollectionKind? collectionKind(DartType? type) { + if (type is! InterfaceType) { + return null; + } + + final types = [type, ...type.allSupertypes]; + + if (types.any((it) => it.isDartCoreMap)) { + return CollectionKind.map; + } + if (types.any((it) => it.isDartCoreSet)) { + return CollectionKind.set; + } + if (types.any((it) => it.isDartCoreList)) { + return CollectionKind.list; + } + return null; +} + +const _flutterFoundationUri = 'package:flutter/foundation.dart'; +const _collectionUri = 'package:collection/collection.dart'; + +BinaryExpression? _targetBinary(AstNode node) => + node.thisOrAncestorOfType(); + +class ReplaceWithFlutterFoundationEqualsFix extends ResolvedCorrectionProducer { + ReplaceWithFlutterFoundationEqualsFix({required super.context}); + + @override + FixKind get fixKind => const .new( + 'leancode_lint.fix.replaceWithFlutterFoundationEquals', + DartFixKindPriority.standard, + "Replace with '{0}'", + ); + + @override + List? get fixArguments { + final binary = _targetBinary(node)!; + final kind = collectionKind(binary.leftOperand.staticType)!; + return [kind.flutterFunction]; + } + + @override + CorrectionApplicability get applicability => .automatically; + + @override + Future compute(ChangeBuilder builder) async { + final binary = _targetBinary(node); + if (binary == null) { + return; + } + + if (!dependsOnPackage('flutter')) { + return; + } + + final kind = collectionKind(binary.leftOperand.staticType); + if (kind == null) { + return; + } + + final negate = binary.operator.type == TokenType.BANG_EQ; + final left = binary.leftOperand.toSource(); + final right = binary.rightOperand.toSource(); + + await builder.addDartFileEdit(file, (builder) { + builder + ..importLibraryElement(.parse(_flutterFoundationUri)) + ..addReplacement( + range.node(binary), + (builder) => builder.write( + '${negate ? '!' : ''}${kind.flutterFunction}($left, $right)', + ), + ) + ..format(range.node(binary)); + }); + } +} + +class ReplaceWithCollectionPackageEqualityFix + extends ResolvedCorrectionProducer { + ReplaceWithCollectionPackageEqualityFix({required super.context}); + + @override + FixKind get fixKind => const .new( + 'leancode_lint.fix.replaceWithCollectionPackageEquality', + DartFixKindPriority.standard, + "Replace with '{0}'", + ); + + @override + List? get fixArguments { + final binary = _targetBinary(node)!; + final kind = collectionKind(binary.leftOperand.staticType)!; + return [kind.collectionClass]; + } + + @override + CorrectionApplicability get applicability => .automatically; + + @override + Future compute(ChangeBuilder builder) async { + final binary = _targetBinary(node); + if (binary == null) { + return; + } + + if (!dependsOnPackage('collection')) { + return; + } + + final leftType = binary.leftOperand.staticType; + final kind = collectionKind(leftType); + if (kind == null) { + return; + } + + // Resolve the collection's type arguments (e.g. `int` for `List`) so + // the generated constructor is `const ListEquality()` rather than a + // raw `const ListEquality()`, which fails type inference. + final collectionElement = switch (kind) { + .list => typeProvider.listElement, + .set => typeProvider.setElement, + .map => typeProvider.mapElement, + }; + final typeArguments = leftType is InterfaceType + ? leftType.asInstanceOf(collectionElement)?.typeArguments ?? const [] + : const []; + + final negate = binary.operator.type == TokenType.BANG_EQ; + final left = binary.leftOperand.toSource(); + final right = binary.rightOperand.toSource(); + + await builder.addDartFileEdit(file, (builder) { + builder + ..importLibraryElement(.parse(_collectionUri)) + ..addReplacement(range.node(binary), (builder) { + if (negate) { + builder.write('!'); + } + builder.write('const ${kind.collectionClass}'); + if (typeArguments.isNotEmpty) { + builder + ..writeTypes( + typeArguments, + prefix: '<', + shouldWriteDynamic: true, + ) + ..write('>'); + } + builder.write('().equals($left, $right)'); + }) + ..format(range.node(binary)); + }); + } +} + +/// For the cases where an identity comparison is actually intended. +class ReplaceWithIdenticalFix extends ResolvedCorrectionProducer { + ReplaceWithIdenticalFix({required super.context}); + + @override + FixKind get fixKind => const .new( + 'leancode_lint.fix.replaceWithIdentical', + DartFixKindPriority.standard, + "Replace with 'identical'", + ); + + @override + CorrectionApplicability get applicability => .automatically; + + @override + Future compute(ChangeBuilder builder) async { + final binary = _targetBinary(node); + if (binary == null) { + return; + } + + final negate = binary.operator.type == TokenType.BANG_EQ; + final left = binary.leftOperand.toSource(); + final right = binary.rightOperand.toSource(); + + await builder.addDartFileEdit(file, (builder) { + builder + ..addReplacement( + range.node(binary), + (builder) => + builder.write('${negate ? '!' : ''}identical($left, $right)'), + ) + ..format(range.node(binary)); + }); + } +} diff --git a/packages/leancode_lint/test/test_cases/avoid_direct_collection_equality_checks_test.dart b/packages/leancode_lint/test/test_cases/avoid_direct_collection_equality_checks_test.dart new file mode 100644 index 00000000..5a3c5060 --- /dev/null +++ b/packages/leancode_lint/test/test_cases/avoid_direct_collection_equality_checks_test.dart @@ -0,0 +1,167 @@ +import 'package:analyzer_testing/analysis_rule/analysis_rule.dart'; +import 'package:leancode_lint/src/lints/avoid_direct_collection_equality_checks.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +import '../assert_ranges.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AvoidDirectCollectionEqualityChecksTest); + }); +} + +@reflectiveTest +class AvoidDirectCollectionEqualityChecksTest extends AnalysisRuleTest { + @override + void setUp() { + rule = AvoidDirectCollectionEqualityChecks(); + + super.setUp(); + } + + Future test_list_equality_is_marked() async { + await assertDiagnosticsInRanges(''' +bool test(List a, List b) { + return [!a == b!]; +} +'''); + } + + Future test_list_inequality_is_marked() async { + await assertDiagnosticsInRanges(''' +bool test(List a, List b) { + return [!a != b!]; +} +'''); + } + + Future test_list_literals_are_marked() async { + await assertDiagnosticsInRanges(''' +bool test() { + return [![1] == [2]!]; +} +'''); + } + + Future test_set_equality_is_marked() async { + await assertDiagnosticsInRanges(''' +bool test(Set a, Set b) { + return [!a == b!]; +} +'''); + } + + Future test_map_equality_is_marked() async { + await assertDiagnosticsInRanges(''' +bool test(Map a, Map b) { + return [!a == b!]; +} +'''); + } + + Future test_nullable_lists_are_marked() async { + await assertDiagnosticsInRanges(''' +bool test(List? a, List? b) { + return [!a == b!]; +} +'''); + } + + Future test_subtype_of_list_is_marked() async { + await assertDiagnosticsInRanges(''' +abstract class MyList implements List {} + +bool test(MyList a, List b) { + return [!a == b!]; +} +'''); + } + + Future test_local_map_variables_are_marked() async { + await assertDiagnosticsInRanges(''' +bool test() { + final a = {'x': 1}; + final b = {'y': 2}; + return [!a == b!]; +} +'''); + } + + Future test_list_of_constructor_is_marked() async { + await assertDiagnosticsInRanges(''' +bool test() { + final a = [1, 2, 3]; + final b = [1, 2, 3]; + return [!a == List.of(b)!]; +} +'''); + } + + Future test_to_list_conversion_is_marked() async { + await assertDiagnosticsInRanges(''' +bool test(List a, Iterable b) { + return [!a == b.toList()!]; +} +'''); + } + + Future test_to_set_conversion_is_marked() async { + await assertDiagnosticsInRanges(''' +bool test(Set a, Iterable b) { + return [!a == b.toSet()!]; +} +'''); + } + + Future test_comparison_with_null_is_not_marked() async { + await assertNoDiagnostics(''' +bool test(List? a) { + return a == null; +} +'''); + } + + Future test_collection_vs_non_collection_is_not_marked() async { + await assertNoDiagnostics(''' +bool test(List a, Object b) { + return a == b; +} +'''); + } + + Future test_scalar_comparison_is_not_marked() async { + await assertNoDiagnostics(''' +bool test(int a, int b, String c, String d) { + return a == b && c == d; +} +'''); + } + + Future test_different_collection_kinds_are_not_marked() async { + await assertNoDiagnostics(''' +bool test(List a, Set b) { + return a == b; +} +'''); + } + + Future test_custom_class_with_equals_is_not_marked() async { + await assertNoDiagnostics(''' +class Value { + const Value(this.value); + + final int value; + + @override + bool operator ==(Object other) => other is Value && other.value == value; + + @override + int get hashCode => value.hashCode; +} + +bool test(Value a, Value b) { + return a == b; +} +'''); + } +} diff --git a/packages/leancode_lint/test/test_cases/depends_on_package_test.dart b/packages/leancode_lint/test/test_cases/depends_on_package_test.dart new file mode 100644 index 00000000..0065f43e --- /dev/null +++ b/packages/leancode_lint/test/test_cases/depends_on_package_test.dart @@ -0,0 +1,130 @@ +import 'dart:convert'; + +import 'package:analyzer/file_system/memory_file_system.dart'; +import 'package:analyzer/src/context/packages.dart'; +import 'package:analyzer/src/workspace/pub.dart'; +import 'package:analyzer/workspace/workspace.dart'; +import 'package:leancode_lint/src/helpers.dart'; +import 'package:test/test.dart'; + +void main() { + late MemoryResourceProvider provider; + late String rootPath; + + String inRoot(List parts) => + provider.pathContext.joinAll([rootPath, ...parts]); + + WorkspacePackage? buildPackage({ + List dependencies = const [], + List devDependencies = const [], + }) { + String section(String name, List packages) => packages.isEmpty + ? '' + : '$name:\n${packages.map((p) => ' $p: any\n').join()}'; + + provider.newFile( + inRoot(['pubspec.yaml']), + ''' +name: my_app +${section('dependencies', dependencies)}${section('dev_dependencies', devDependencies)}''', + ); + + final packageConfigFile = provider.newFile( + inRoot(['.dart_tool', 'package_config.json']), + jsonEncode({ + 'configVersion': 2, + 'packages': [ + {'name': 'my_app', 'rootUri': '../', 'packageUri': 'lib/'}, + // Resolvable, but not declared by `my_app` unless a test says so. + { + 'name': 'collection', + 'rootUri': '/pub-cache/collection', + 'packageUri': 'lib/', + }, + ], + }), + ); + + return PackageConfigWorkspace( + provider, + rootPath, + packageConfigFile, + Packages.empty, + ).findPackageFor(inRoot(['lib', 'a.dart'])); + } + + setUp(() { + provider = MemoryResourceProvider(); + rootPath = provider.pathContext.join( + provider.pathContext.rootPrefix(provider.pathContext.current), + 'home', + 'my_app', + ); + }); + + test('finds a direct dependency', () { + expect( + packageDependsOn( + 'collection', + package: buildPackage(dependencies: ['collection']), + filePath: inRoot(['lib', 'a.dart']), + ), + isTrue, + ); + }); + + test('does not find a package that is only transitively available', () { + expect( + packageDependsOn( + 'collection', + package: buildPackage(dependencies: ['flutter']), + filePath: inRoot(['lib', 'a.dart']), + ), + isFalse, + ); + }); + + test('ignores dev dependencies for files in public directories', () { + final package = buildPackage(devDependencies: ['collection']); + + for (final path in [ + inRoot(['lib', 'a.dart']), + inRoot(['lib', 'src', 'a.dart']), + inRoot(['bin', 'a.dart']), + inRoot(['hook', 'build.dart']), + ]) { + expect( + packageDependsOn('collection', package: package, filePath: path), + isFalse, + reason: path, + ); + } + }); + + test('accepts dev dependencies elsewhere', () { + final package = buildPackage(devDependencies: ['collection']); + + for (final path in [ + inRoot(['test', 'a_test.dart']), + inRoot(['tool', 'a.dart']), + inRoot(['example', 'lib', 'a.dart']), + ]) { + expect( + packageDependsOn('collection', package: package, filePath: path), + isTrue, + reason: path, + ); + } + }); + + test('returns false when the file has no package', () { + expect( + packageDependsOn( + 'collection', + package: null, + filePath: inRoot(['lib', 'a.dart']), + ), + isFalse, + ); + }); +}