Lint to prefer x.isNotEmpty vs. !x.isEmpty (#143).

[email protected]

Review URL: https://codereview.chromium.org//1419373009 .

Author: pq

lib/src/ast.dart

@@ -31,18 +31,30 @@ import 'package:analyzer/src/generated/ast.dart'
         TypeParameter,
         VariableDeclaration;
 import 'package:analyzer/src/generated/element.dart'
-    show Element, ParameterElement, PropertyAccessorElement;
+    show
+        Element,
+        GeneralizingElementVisitor,
+        ParameterElement,
+        PropertyAccessorElement;
 import 'package:analyzer/src/generated/scanner.dart'
     show Keyword, KeywordToken, KeywordTokenWithComment, Token;
 import 'package:linter/src/util.dart';
 
+/// Returns direct children of [parent].
+List<Element> getChildren(Element parent, [String name]) {
+  List<Element> children = <Element>[];
+  visitChildren(parent, (Element element) {
+    if (name == null || element.displayName == name) {
+      children.add(element);
+    }
+  });
+  return children;
+}
+
 /// Returns the most specific AST node appropriate for associating errors.
 AstNode getNodeToAnnotate(Declaration node) {
   AstNode mostSpecific = _getNodeToAnnotate(node);
-  if (mostSpecific != null) {
-    return mostSpecific;
-  }
-  return node;
+  return mostSpecific ?? node;
 }
 
 /// Returns `true` if the keyword associated with this token is `final` or
@@ -131,6 +143,12 @@ bool isValidDartIdentifier(String id) => !isKeyWord(id) && isIdentifier(id);
 /// Returns `true` if the keyword associated with this token is `var`.
 bool isVar(Token token) => isKeyword(token, Keyword.VAR);
 
+/// Uses [processor] to visit all of the children of [element].
+/// If [processor] returns `true`, then children of a child are visited too.
+void visitChildren(Element element, ElementProcessor processor) {
+  element.visitChildren(new _ElementVisitorAdapter(processor));
+}
+
 bool _checkForSimpleGetter(MethodDeclaration getter, Expression expression) {
   if (expression is SimpleIdentifier) {
     var staticElement = expression.staticElement;
@@ -224,3 +242,21 @@ AstNode _getNodeToAnnotate(Declaration node) {
   }
   return null;
 }
+
+/// An [Element] processor function type.
+/// If `true` is returned, children of [element] will be visited.
+typedef bool ElementProcessor(Element element);
+
+/// A [GeneralizingElementVisitor] adapter for [ElementProcessor].
+class _ElementVisitorAdapter extends GeneralizingElementVisitor {
+  final ElementProcessor processor;
+  _ElementVisitorAdapter(this.processor);
+
+  @override
+  void visitElement(Element element) {
+    bool visitChildren = processor(element);
+    if (visitChildren == true) {
+      element.visitChildren(this);
+    }
+  }
+}

lib/src/linter.dart

@@ -98,7 +98,7 @@ class DartLinter implements AnalysisErrorListener {
           } on Exception catch (e) {
             reporter.exception(new LinterException(e.toString()));
           }
-          if (rule._locationInfo != null && !rule._locationInfo.isEmpty) {
+          if (rule._locationInfo != null && rule._locationInfo.isNotEmpty) {
             results.addAll(rule._locationInfo);
             rule._locationInfo.clear();
           }
@@ -399,7 +399,7 @@ class SourceLinter implements DartLinter, AnalysisErrorListener {
           } on Exception catch (e) {
             reporter.exception(new LinterException(e.toString()));
           }
-          if (rule._locationInfo != null && !rule._locationInfo.isEmpty) {
+          if (rule._locationInfo != null && rule._locationInfo.isNotEmpty) {
             results.addAll(rule._locationInfo);
             rule._locationInfo.clear();
           }

lib/src/rules.dart

@@ -20,6 +20,7 @@ import 'package:linter/src/rules/non_constant_identifier_names.dart';
 import 'package:linter/src/rules/one_member_abstracts.dart';
 import 'package:linter/src/rules/package_api_docs.dart';
 import 'package:linter/src/rules/package_prefixed_library_names.dart';
+import 'package:linter/src/rules/prefer_is_not_empty.dart';
 import 'package:linter/src/rules/pub/package_names.dart';
 import 'package:linter/src/rules/slash_for_doc_comments.dart';
 import 'package:linter/src/rules/super_goes_last.dart';
@@ -37,6 +38,7 @@ final Registry ruleRegistry = new Registry()
   ..register(new LibraryNames())
   ..register(new LibraryPrefixes())
   ..register(new NonConstantIdentifierNames())
+  ..register(new PreferIsNotEmpty())
   ..register(new OneMemberAbstracts())
   ..register(new PackageApiDocs())
   ..register(new PackagePrefixedLibraryNames())

lib/src/rules/prefer_is_not_empty.dart

@@ -0,0 +1,102 @@
+// Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library linter.src.rules.prefer_is_not_empty;
+
+import 'package:analyzer/src/generated/ast.dart'
+    show
+        AstNode,
+        AstVisitor,
+        PrefixExpression,
+        PrefixedIdentifier,
+        PropertyAccess,
+        SimpleAstVisitor,
+        SimpleIdentifier;
+import 'package:analyzer/src/generated/element.dart' show Element;
+import 'package:analyzer/src/generated/scanner.dart' show TokenType;
+import 'package:linter/src/ast.dart';
+import 'package:linter/src/linter.dart';
+
+const desc = 'Use isNotEmpty for Iterables and Maps.';
+
+const details = '''
+**PREFER** `x.isNotEmpty` to `!x.isEmpty` for Iterables and Maps.
+
+When testing whether an iterable or map is empty, prefer `isNotEmpty` over
+`!isEmpty`.
+
+**GOOD:**
+```
+if (todo.isNotEmpty) {
+  sendResults(request, todo.isEmpty);
+}
+```
+
+**BAD:**
+```
+if (!sources.isEmpty) {
+  process(sources);
+}
+```
+''';
+
+class PreferIsNotEmpty extends LintRule {
+  PreferIsNotEmpty()
+      : super(
+            name: 'prefer_is_not_empty',
+            description: desc,
+            details: details,
+            group: Group.style);
+
+  @override
+  AstVisitor getVisitor() => new Visitor(this);
+}
+
+class Visitor extends SimpleAstVisitor {
+  final LintRule rule;
+  Visitor(this.rule);
+
+  @override
+  visitSimpleIdentifier(SimpleIdentifier identifier) {
+    AstNode isEmptyAccess = null;
+    SimpleIdentifier isEmptyIdentifier = null;
+
+    AstNode parent = identifier.parent;
+    if (parent is PropertyAccess) {
+      isEmptyIdentifier = parent.propertyName;
+      isEmptyAccess = parent;
+    } else if (parent is PrefixedIdentifier) {
+      isEmptyIdentifier = parent.identifier;
+      isEmptyAccess = parent;
+    }
+
+    if (isEmptyIdentifier == null) {
+      return;
+    }
+
+    // Should be "isEmpty".
+    Element propertyElement = isEmptyIdentifier.bestElement;
+    if (propertyElement == null || 'isEmpty' != propertyElement.name) {
+      return;
+    }
+    // Should have "isNotEmpty".
+    Element propertyTarget = propertyElement.enclosingElement;
+    if (propertyTarget == null ||
+        getChildren(propertyTarget, 'isNotEmpty').isEmpty) {
+      return;
+    }
+    // Should be in PrefixExpression.
+    if (isEmptyAccess.parent is! PrefixExpression) {
+      return;
+    }
+    PrefixExpression prefixExpression =
+        isEmptyAccess.parent as PrefixExpression;
+    // Should be !
+    if (prefixExpression.operator.type != TokenType.BANG) {
+      return;
+    }
+
+    rule.reportLint(prefixExpression);
+  }
+}

lib/src/sdk.dart

@@ -92,6 +92,7 @@ class Iterator<E> {
 abstract class Iterable<E> {
   Iterator<E> get iterator;
   bool get isEmpty;
+  bool get isNotEmpty;
 }
 
 abstract class List<E> implements Iterable<E> {
@@ -100,10 +101,14 @@ abstract class List<E> implements Iterable<E> {
   void operator []=(int index, E value);
   Iterator<E> get iterator => null;
   void clear();
+  bool get isEmpty;
+  bool get isNotEmpty;
 }
 
 abstract class Map<K, V> extends Object {
   Iterable<K> get keys;
+  bool get isEmpty;
+  bool get isNotEmpty;
 }
 
 external bool identical(Object a, Object b);

test/rules/prefer_is_not_empty.dart

@@ -0,0 +1,13 @@
+// Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+bool lne = ![].isEmpty; //LINT [12:11]
+bool mne = !{}.isEmpty; //LINT
+bool ine = !iterable.isEmpty; //LINT
+
+bool le = [].isEmpty;
+bool me = {}.isEmpty;
+bool ie = iterable.isEmpty;
+
+Iterable get iterable => [];