Implemented loop on hash inspection

Fixes #1748
Camelcade · Mar 16, 2018 · 441032a · 441032a
1 parent 5c67adb
commit 441032a
Show file tree

Hide file tree

Showing 6 changed files with 148 additions and 0 deletions.
diff --git a/resources/META-INF/plugin.xml b/resources/META-INF/plugin.xml
@@ -42,6 +42,7 @@
      <ul>
       <li>Assignments alignment now supports a <code>Perl::Tidy</code> mode - alignment on consecutive lines, by <a href="https://github.com/Camelcade/Perl5-IDEA/issues/1745">@ElMoselYEE</a></li>
       <li>Comments are now aligned on consecutive lines, configurable</li>
+      <li>Loop on hash inspection, <a href="https://github.com/Camelcade/Perl5-IDEA/issues/1748">#1748</a></li>
      </ul>
      </p>
      <p>Fixes:
@@ -945,6 +946,10 @@
                      shortName="IdentifierInspection" displayName="Correct identifiers" groupName="Perl5"
                      enabledByDefault="true" level="ERROR"
                      implementationClass="com.perl5.lang.perl.idea.inspections.PerlIdentifierInspection"/>
+    <localInspection language="Perl5"
+                     shortName="HashLoopInspection" displayName="Loop on hash" groupName="Perl5"
+                     enabledByDefault="true" level="ERROR"
+                     implementationClass="com.perl5.lang.perl.idea.inspections.PerlHashLoopInspection"/>
 
     <intentionAction>
       <className>com.perl5.lang.perl.idea.intentions.StatementToCompoundIntention</className>

diff --git a/resources/inspectionDescriptions/HashLoopInspection.html b/resources/inspectionDescriptions/HashLoopInspection.html
@@ -0,0 +1,42 @@
+<!--
+  ~ Copyright 2015-2017 Alexandr Evstigneev
+  ~
+  ~ Licensed under the Apache License, Version 2.0 (the "License");
+  ~ you may not use this file except in compliance with the License.
+  ~ You may obtain a copy of the License at
+  ~
+  ~ http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing, software
+  ~ distributed under the License is distributed on an "AS IS" BASIS,
+  ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+
+<html>
+<body>
+<p>Port of SawyerX's <a href="https://github.com/xsawyerx/perl-critic-policy-variables-prohibitlooponhash"><code>Perl::Critic::Policy::Variables::ProhibitLoopOnHash</code></a>
+</p>
+
+<p>When "looping over hashes," we mean looping over hash keys or hash values. If you forgot to call keys or values you will accidentally
+  loop over both.</p>
+
+<code>
+  foreach my $foo (%hash) {...} # not ok
+  action() for %hash; # not ok
+  foreach my $foo ( keys %hash ) {...} # ok
+  action() for values %hash; # ok
+</code>
+
+<p>An effort is made to detect expressions:</p>
+
+<code>
+  action() for %hash ? keys %hash : (); # ok
+  action() for %{ $hash{'stuff'} } ? keys %{ $hash{'stuff'} } : (); # ok
+</code>
+
+<p>(Granted, the second example there doesn't make much sense, but I have found a variationo of it in real code.)</p>
+
+</body>
+</html>
diff --git a/resources/messages/PerlBundle.properties b/resources/messages/PerlBundle.properties
@@ -271,6 +271,7 @@ perl.inspection.use.warnings=No warnings pragma found in the file
 perl.inspection.use.strict=No strict pragma found in the file
 perl.inspection.no.exported.entity=Unable to find exported entity
 perl.inspection.no.sub.definition=Unable to find sub definition, declaration, constant definition or typeglob aliasing
+perl.inspection.loop.on.hash=Hash iterating. You've probably missed <code>keys</code> or <code>values</code>
 perl.quickfix.use.vars=Convert to 'our' declaration
 perl.remove.redundant.code=Remove redundant code
 perl.redundant.code=This code is redundant

diff --git a/src/com/perl5/lang/perl/idea/inspections/PerlHashLoopInspection.java b/src/com/perl5/lang/perl/idea/inspections/PerlHashLoopInspection.java
@@ -0,0 +1,65 @@
+/*
+ * Copyright 2015-2017 Alexandr Evstigneev
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.perl5.lang.perl.idea.inspections;
+
+import com.intellij.codeInspection.ProblemsHolder;
+import com.intellij.psi.PsiElement;
+import com.intellij.psi.PsiElementVisitor;
+import com.intellij.psi.tree.TokenSet;
+import com.intellij.psi.util.PsiUtilCore;
+import com.perl5.PerlBundle;
+import com.perl5.lang.perl.psi.PerlVisitor;
+import com.perl5.lang.perl.psi.PsiPerlConditionExpr;
+import com.perl5.lang.perl.psi.PsiPerlForCompound;
+import com.perl5.lang.perl.psi.PsiPerlForStatementModifier;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import static com.perl5.lang.perl.lexer.PerlElementTypesGenerated.HASH_CAST_EXPR;
+import static com.perl5.lang.perl.lexer.PerlElementTypesGenerated.HASH_VARIABLE;
+
+public class PerlHashLoopInspection extends PerlInspection {
+  private static final TokenSet HASH_LIKE_TOKENS = TokenSet.create(
+    HASH_VARIABLE,
+    HASH_CAST_EXPR
+  );
+
+  @NotNull
+  @Override
+  public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+    return new PerlVisitor() {
+      @Override
+      public void visitForStatementModifier(@NotNull PsiPerlForStatementModifier o) {
+        checkExpression(o.getExpr());
+      }
+
+      @Override
+      public void visitForCompound(@NotNull PsiPerlForCompound o) {
+        PsiPerlConditionExpr conditionExpr = o.getConditionExpr();
+        if (conditionExpr != null) {
+          checkExpression(conditionExpr.getExpr());
+        }
+      }
+
+      private void checkExpression(@Nullable PsiElement expr) {
+        if (HASH_LIKE_TOKENS.contains(PsiUtilCore.getElementType(expr))) {
+          registerProblem(holder, expr, PerlBundle.message("perl.inspection.loop.on.hash"));
+        }
+      }
+    };
+  }
+}
diff --git a/test/annotator/PerlAnnotatorTest.java b/test/annotator/PerlAnnotatorTest.java
@@ -33,6 +33,8 @@ protected String getTestDataPath() {
 
   private void doTestUseVarsInspection() {doInspectionTest(PerlUseVarsInspection.class);}
 
+  public void testLoopInHashInspection() {doInspectionTest(PerlHashLoopInspection.class);}
+
   public void testUseVarsInspection() {doTestUseVarsInspection();}
 
   public void testUseVarsEmptyInspection() {doTestUseVarsInspection();}

diff --git a/testData/annotator/perl/loopInHashInspection.code b/testData/annotator/perl/loopInHashInspection.code
@@ -0,0 +1,33 @@
+use strict;
+use warnings;
+use v5.10;
+
+my %hash;
+my $scalar;
+my @array;
+foreach my $element (<error descr="Hash iterating. You've probably missed 'keys' or 'values'">%hash</error>) {}
+for my $element (<error descr="Hash iterating. You've probably missed 'keys' or 'values'">%hash</error>) {}
+say for <error descr="Hash iterating. You've probably missed 'keys' or 'values'">%hash</error>;
+say foreach <error descr="Hash iterating. You've probably missed 'keys' or 'values'">%hash</error>;
+
+foreach my $element (<error descr="Hash iterating. You've probably missed 'keys' or 'values'">%$scalar</error>) {}
+for my $element (<error descr="Hash iterating. You've probably missed 'keys' or 'values'">%$scalar</error>) {}
+say for <error descr="Hash iterating. You've probably missed 'keys' or 'values'">%$scalar</error>;
+say foreach <error descr="Hash iterating. You've probably missed 'keys' or 'values'">%$scalar</error>;
+
+foreach my $element (<error descr="Hash iterating. You've probably missed 'keys' or 'values'">%{say 'hi'}</error>) {}
+for my $element (<error descr="Hash iterating. You've probably missed 'keys' or 'values'">%{say 'hi'}</error>) {}
+say for <error descr="Hash iterating. You've probably missed 'keys' or 'values'">%{say 'hi'}</error>;
+say foreach <error descr="Hash iterating. You've probably missed 'keys' or 'values'">%{say 'hi'}</error>;
+
+
+foreach my $element (@hash{1}) {}
+for my $element (@hash{1}) {}
+say for @hash{1};
+say foreach @hash{1};
+
+foreach my $element (@array) {}
+for my $element (@array) {}
+say for @array;
+say foreach @array;
+