From 0131b875dc4d4c22d57b41ba50511d7c602951d8 Mon Sep 17 00:00:00 2001 From: Dmitry Zhifarsky Date: Sun, 25 Apr 2021 21:56:47 +0300 Subject: [PATCH 1/3] feat: add avoid non null assertion rule --- CHANGELOG.md | 4 + README.md | 1 + doc/rules/avoid_non_null_assertion.md | 57 ++++++++++++ .../rules/avoid_non_null_assertion_rule.dart | 69 ++++++++++++++ lib/src/obsoleted/rules_factory.dart | 3 + .../avoid_non_null_assertion_test.dart | 91 +++++++++++++++++++ .../examples/example.dart | 31 +++++++ 7 files changed, 256 insertions(+) create mode 100644 doc/rules/avoid_non_null_assertion.md create mode 100644 lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart create mode 100644 test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart create mode 100644 test/obsoleted/rules/avoid_non_null_assertion/examples/example.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d38ecd158..020ad7fc06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +* Add static code diagnostic `avoid-non-null-assertion`. + ## 3.1.0 * Add excludes for a separate rule. diff --git a/README.md b/README.md index 2db32aa10f..166d880e10 100644 --- a/README.md +++ b/README.md @@ -324,6 +324,7 @@ Rules configuration is [described here](#configuring-a-rules-entry). ### Common +- [avoid-non-null-assertion](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/avoid_non_null_assertion.md) - [avoid-unused-parameters](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/avoid_unused_parameters.md) - [binary-expression-operand-order](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/binary_expression_operand_order.md)   ![Has auto-fix](https://img.shields.io/badge/-has%20auto--fix-success) - [double-literal-format](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/double_literal_format.md)   ![Has auto-fix](https://img.shields.io/badge/-has%20auto--fix-success) diff --git a/doc/rules/avoid_non_null_assertion.md b/doc/rules/avoid_non_null_assertion.md new file mode 100644 index 0000000000..ccc0ed31da --- /dev/null +++ b/doc/rules/avoid_non_null_assertion.md @@ -0,0 +1,57 @@ +# Avoid non null assertion + +## Rule id + +avoid-non-null-assertion + +## Description + +Warns when non null assertion operator (**!** or “bang” operator) is used for a property access or method invocation. The operator check works at runtime and it may fail and throw a runtime exception. + +The rule ignores the index `[]` operator on the Map class because it's considered the idiomatic way to access a known-present element in a map with `[]!` according to [the docs](https://dart.dev/null-safety/understanding-null-safety#the-map-index-operator-is-nullable). + +Use this rule if you want to avoid possible unexpected runtime exceptions. + +### Example + +Bad: + +```dart +class Test { + String? field; + + Test? object; + + void method() { + field!.contains('other'); // LINT + + object!.field!.contains('other'); // LINT + + final map = {'key': 'value'}; + map['key']!.contains('other'); + + object!.method(); // LINT + } +} +``` + +Good: + +```dart +class Test { + String? field; + + Test? object; + + void method() { + field?.contains('other'); + + object?.field?.contains('other'); + + final map = {'key': 'value'}; + map['key']!.contains('other'); + + object?.method(); + } +} +``` diff --git a/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart b/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart new file mode 100644 index 0000000000..3f487262b1 --- /dev/null +++ b/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart @@ -0,0 +1,69 @@ +import 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/token.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; + +import '../../models/issue.dart'; +import '../../models/severity.dart'; +import '../../utils/node_utils.dart'; +import '../../utils/rule_utils.dart'; +import 'obsolete_rule.dart'; + +class AvoidNonNullAssertionRule extends ObsoleteRule { + static const String ruleId = 'avoid-non-null-assertion'; + static const _documentationUrl = 'https://git.io/JO5Ju'; + + static const _failure = 'Avoid using non null assertion.'; + + AvoidNonNullAssertionRule({Map config = const {}}) + : super( + id: ruleId, + documentationUrl: Uri.parse(_documentationUrl), + severity: readSeverity(config, Severity.warning), + excludes: readExcludes(config), + ); + + @override + Iterable check(ResolvedUnitResult source) { + final visitor = _Visitor(); + + source.unit?.visitChildren(visitor); + + return visitor.expressions + .map((expression) => createIssue( + rule: this, + location: nodeLocation( + node: expression, + source: source, + withCommentOrMetadata: true, + ), + message: _failure, + )) + .toList(growable: false); + } +} + +class _Visitor extends RecursiveAstVisitor { + final _expressions = []; + + Iterable get expressions => _expressions; + + @override + void visitPostfixExpression(PostfixExpression node) { + super.visitPostfixExpression(node); + + if (node.operator.type == TokenType.BANG && + !_isMapIndexOperator(node.operand)) { + _expressions.add(node); + } + } + + bool _isMapIndexOperator(Expression operand) { + if (operand is IndexExpression) { + final type = operand.target?.staticType; + return type != null && type.isDartCoreMap; + } + + return false; + } +} diff --git a/lib/src/obsoleted/rules_factory.dart b/lib/src/obsoleted/rules_factory.dart index 42c471fd0b..b2e869c66a 100644 --- a/lib/src/obsoleted/rules_factory.dart +++ b/lib/src/obsoleted/rules_factory.dart @@ -1,4 +1,5 @@ import '../rules/rule.dart'; +import 'rules/avoid_non_null_assertion_rule.dart'; import 'rules/avoid_preserve_whitespace_false.dart'; import 'rules/avoid_returning_widgets_rule.dart'; import 'rules/avoid_unused_parameters.dart'; @@ -21,6 +22,8 @@ import 'rules/prefer_trailing_comma.dart'; import 'rules/provide_correct_intl_args.dart'; final _implementedRules = )>{ + AvoidNonNullAssertionRule.ruleId: (config) => + AvoidNonNullAssertionRule(config: config), AvoidPreserveWhitespaceFalseRule.ruleId: (config) => AvoidPreserveWhitespaceFalseRule(config: config), AvoidReturningWidgets.ruleId: (config) => diff --git a/test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart b/test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart new file mode 100644 index 0000000000..7508aa1e2a --- /dev/null +++ b/test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart @@ -0,0 +1,91 @@ +@TestOn('vm') +import 'dart:io'; + +import 'package:analyzer/dart/analysis/utilities.dart'; +import 'package:dart_code_metrics/src/models/severity.dart'; +import 'package:dart_code_metrics/src/obsoleted/models/internal_resolved_unit_result.dart'; +import 'package:dart_code_metrics/src/obsoleted/rules/avoid_non_null_assertion_rule.dart'; +import 'package:test/test.dart'; + +const _examplePath = + 'test/obsoleted/rules/avoid_non_null_assertion/examples/example.dart'; + +void main() { + group('AvoidNonNullAssertion', () { + test('initialization', () async { + final path = File(_examplePath).absolute.path; + final sourceUrl = Uri.parse(path); + + // ignore: deprecated_member_use + final parseResult = await resolveFile(path: path); + + final issues = + AvoidNonNullAssertionRule().check(InternalResolvedUnitResult( + sourceUrl, + parseResult!.content!, + parseResult.unit!, + )); + + expect( + issues.every( + (issue) => issue.ruleId == 'avoid-non-null-assertion', + ), + isTrue, + ); + expect( + issues.every((issue) => issue.severity == Severity.warning), + isTrue, + ); + }); + + test('reports about found issues', () async { + final path = File(_examplePath).absolute.path; + final sourceUrl = Uri.parse(path); + + // ignore: deprecated_member_use + final parseResult = await resolveFile(path: path); + + final issues = + AvoidNonNullAssertionRule().check(InternalResolvedUnitResult( + sourceUrl, + parseResult!.content!, + parseResult.unit!, + )); + + expect( + issues.map((issue) => issue.location.start.offset), + equals([70, 208, 208, 460]), + ); + expect( + issues.map((issue) => issue.location.start.line), + equals([7, 15, 15, 27]), + ); + expect( + issues.map((issue) => issue.location.start.column), + equals([5, 5, 5, 5]), + ); + expect( + issues.map((issue) => issue.location.end.offset), + equals([76, 215, 222, 467]), + ); + expect( + issues.map((issue) => issue.location.text), + equals([ + 'field!', + 'object!', + 'object!.field!', + 'object!', + ]), + ); + expect( + issues.map((issue) => issue.message), + equals([ + 'Avoid using non null assertion.', + 'Avoid using non null assertion.', + 'Avoid using non null assertion.', + 'Avoid using non null assertion.', + ]), + ); + }); + }); +} diff --git a/test/obsoleted/rules/avoid_non_null_assertion/examples/example.dart b/test/obsoleted/rules/avoid_non_null_assertion/examples/example.dart new file mode 100644 index 0000000000..a5f43627f2 --- /dev/null +++ b/test/obsoleted/rules/avoid_non_null_assertion/examples/example.dart @@ -0,0 +1,31 @@ +class Test { + String? field; + + Test? object; + + void method() { + field!.contains('other'); // LINT + + field?.replaceAll('from', 'replace'); + + if (filed != null) { + field.split(' '); + } + + object!.field!.contains('other'); // LINT + + object?.field?.contains('other'); + + final field = object?.field; + if (field != null) { + field.contains('other'); + } + + final map = {'key': 'value'}; + map['key']!.contains('other'); + + object!.method(); // LINT + + object?.method(); + } +} From cddb25c5442503ed5779e2f196e8f069a4f5b72b Mon Sep 17 00:00:00 2001 From: Dmitry Zhifarsky Date: Sun, 25 Apr 2021 23:02:34 +0300 Subject: [PATCH 2/3] test: migrate rule tests to the new approach --- .../rules/avoid_non_null_assertion_rule.dart | 1 + .../avoid_non_null_assertion_test.dart | 84 +++++-------------- 2 files changed, 22 insertions(+), 63 deletions(-) diff --git a/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart b/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart index 3f487262b1..7ee225c72e 100644 --- a/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart +++ b/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart @@ -61,6 +61,7 @@ class _Visitor extends RecursiveAstVisitor { bool _isMapIndexOperator(Expression operand) { if (operand is IndexExpression) { final type = operand.target?.staticType; + return type != null && type.isDartCoreMap; } diff --git a/test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart b/test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart index 7508aa1e2a..a36e6de72c 100644 --- a/test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart +++ b/test/obsoleted/rules/avoid_non_null_assertion/avoid_non_null_assertion_test.dart @@ -1,90 +1,48 @@ @TestOn('vm') -import 'dart:io'; - -import 'package:analyzer/dart/analysis/utilities.dart'; import 'package:dart_code_metrics/src/models/severity.dart'; -import 'package:dart_code_metrics/src/obsoleted/models/internal_resolved_unit_result.dart'; import 'package:dart_code_metrics/src/obsoleted/rules/avoid_non_null_assertion_rule.dart'; import 'package:test/test.dart'; +import '../../../helpers/rule_test_helper.dart'; + const _examplePath = 'test/obsoleted/rules/avoid_non_null_assertion/examples/example.dart'; void main() { group('AvoidNonNullAssertion', () { test('initialization', () async { - final path = File(_examplePath).absolute.path; - final sourceUrl = Uri.parse(path); - - // ignore: deprecated_member_use - final parseResult = await resolveFile(path: path); - - final issues = - AvoidNonNullAssertionRule().check(InternalResolvedUnitResult( - sourceUrl, - parseResult!.content!, - parseResult.unit!, - )); + final unit = await RuleTestHelper.resolveFromFile(_examplePath); + final issues = AvoidNonNullAssertionRule().check(unit); - expect( - issues.every( - (issue) => issue.ruleId == 'avoid-non-null-assertion', - ), - isTrue, - ); - expect( - issues.every((issue) => issue.severity == Severity.warning), - isTrue, + RuleTestHelper.verifyInitialization( + issues: issues, + ruleId: 'avoid-non-null-assertion', + severity: Severity.warning, ); }); test('reports about found issues', () async { - final path = File(_examplePath).absolute.path; - final sourceUrl = Uri.parse(path); - - // ignore: deprecated_member_use - final parseResult = await resolveFile(path: path); - - final issues = - AvoidNonNullAssertionRule().check(InternalResolvedUnitResult( - sourceUrl, - parseResult!.content!, - parseResult.unit!, - )); - - expect( - issues.map((issue) => issue.location.start.offset), - equals([70, 208, 208, 460]), - ); - expect( - issues.map((issue) => issue.location.start.line), - equals([7, 15, 15, 27]), - ); - expect( - issues.map((issue) => issue.location.start.column), - equals([5, 5, 5, 5]), - ); - expect( - issues.map((issue) => issue.location.end.offset), - equals([76, 215, 222, 467]), - ); - expect( - issues.map((issue) => issue.location.text), - equals([ + final unit = await RuleTestHelper.resolveFromFile(_examplePath); + final issues = AvoidNonNullAssertionRule().check(unit); + + RuleTestHelper.verifyIssues( + issues: issues, + startOffsets: [70, 208, 208, 460], + startLines: [7, 15, 15, 27], + startColumns: [5, 5, 5, 5], + endOffsets: [76, 215, 222, 467], + locationTexts: [ 'field!', 'object!', 'object!.field!', 'object!', - ]), - ); - expect( - issues.map((issue) => issue.message), - equals([ + ], + messages: [ 'Avoid using non null assertion.', 'Avoid using non null assertion.', 'Avoid using non null assertion.', 'Avoid using non null assertion.', - ]), + ], ); }); }); From 82cd2a854b7b94639a3c2575402e77f2f8a7a9c9 Mon Sep 17 00:00:00 2001 From: Dmitry Zhifarsky Date: Mon, 26 Apr 2021 22:03:53 +0300 Subject: [PATCH 3/3] Review fixes --- lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart b/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart index 7ee225c72e..2b07c585f4 100644 --- a/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart +++ b/lib/src/obsoleted/rules/avoid_non_null_assertion_rule.dart @@ -35,7 +35,6 @@ class AvoidNonNullAssertionRule extends ObsoleteRule { location: nodeLocation( node: expression, source: source, - withCommentOrMetadata: true, ), message: _failure, ))