[#1954] InvertedIfElseConstructsInspector: resolved false positives (…

…operands types are not considered)
kalessil · Sep 19, 2024 · 3b958a5 · 3b958a5
1 parent 83ae086
commit 3b958a5
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 13 deletions.
diff --git a/.../kalessil/phpStorm/phpInspectionsEA/inspectors/ifs/InvertedIfElseConstructsInspector.java b/.../kalessil/phpStorm/phpInspectionsEA/inspectors/ifs/InvertedIfElseConstructsInspector.java
@@ -11,12 +11,10 @@
 import com.jetbrains.php.lang.lexer.PhpTokenTypes;
 import com.jetbrains.php.lang.psi.PhpPsiElementFactory;
 import com.jetbrains.php.lang.psi.elements.*;
+import com.jetbrains.php.lang.psi.resolve.types.PhpType;
 import com.kalessil.phpStorm.phpInspectionsEA.openApi.BasePhpElementVisitor;
 import com.kalessil.phpStorm.phpInspectionsEA.openApi.BasePhpInspection;
-import com.kalessil.phpStorm.phpInspectionsEA.utils.ExpressionSemanticUtil;
-import com.kalessil.phpStorm.phpInspectionsEA.utils.MessagesPresentationUtil;
-import com.kalessil.phpStorm.phpInspectionsEA.utils.OpenapiTypesUtil;
-import com.kalessil.phpStorm.phpInspectionsEA.utils.PhpLanguageUtil;
+import com.kalessil.phpStorm.phpInspectionsEA.utils.*;
 import org.jetbrains.annotations.NotNull;
 
 /*
@@ -92,11 +90,20 @@ public void visitPhpElse(@NotNull Else elseStatement) {
                                     }
                                     /* if managed to extract condition, then proceed with reporting */
                                     if (extractedCondition != null) {
+                                        final Project project = holder.getProject();
+                                        /* false-positive: variable return types or failing to resolve types */
+                                        if (extractedCondition instanceof PhpTypedElement) {
+                                            final PhpType resolved = OpenapiResolveUtil.resolveType((PhpTypedElement) extractedCondition, project);
+                                            if (resolved == null || resolved.size() != 1) {
+                                                return;
+                                            }
+                                        }
+
                                         final String newCondition = String.format("%s !== %s", left.getText(), right.getText());
                                         holder.registerProblem(
                                                 elseStatement.getFirstChild(),
                                                 MessagesPresentationUtil.prefixWithEa(message),
-                                                new NormalizeWorkflowFix(holder.getProject(), (GroupStatement) ifBody, (GroupStatement) elseBody, extractedCondition, newCondition)
+                                                new NormalizeWorkflowFix(project, (GroupStatement) ifBody, (GroupStatement) elseBody, extractedCondition, newCondition)
                                         );
                                     }
                                 }

diff --git a/testData/fixtures/ifs/if-inverted-condition-else-normalization.fixed.php b/testData/fixtures/ifs/if-inverted-condition-else-normalization.fixed.php
@@ -1,9 +1,12 @@
 <?php
 
-if (expr()) { c(); d(); }
+function returnsBool(): bool { return true; }
+function returnsNullableBool(): ?bool { return true; }
+
+if (returnsBool()) { c(); d(); }
 else { a(); b(); }
 
-if (expr()) { c(); d(); }
+if (returnsBool()) { c(); d(); }
 else { a(); b(); }
 
 if (!true)
@@ -19,7 +22,7 @@
 if (true === true) { c(); d(); }
 else { a(); b(); }
 
-if (false !== expr()) { c(); d(); }
+if (false !== returnsBool()) { c(); d(); }
 else { a(); b(); }
 
 /* not supported: must follow PSR and use `{}` */
@@ -35,4 +38,9 @@
 else {}
 
 if (!empty([])) {}
-else {}
+else {}
+
+if (false === returnsNullableBool())
+{ a(); b(); }
+else
+{ c(); d(); }
diff --git a/testData/fixtures/ifs/if-inverted-condition-else-normalization.php b/testData/fixtures/ifs/if-inverted-condition-else-normalization.php
@@ -1,11 +1,14 @@
 <?php
 
-if (!expr())
+function returnsBool(): bool { return true; }
+function returnsNullableBool(): ?bool { return true; }
+
+if (!returnsBool())
 { a(); b(); }
 <weak_warning descr="[EA] The if-else workflow is driven by inverted conditions, consider avoiding invertions.">else</weak_warning>
 { c(); d(); }
 
-if ( ! expr() )
+if ( ! returnsBool() )
 { a(); b(); }
 <weak_warning descr="[EA] The if-else workflow is driven by inverted conditions, consider avoiding invertions.">else</weak_warning>
 { c(); d(); }
@@ -29,7 +32,7 @@
 <weak_warning descr="[EA] The if-else workflow is driven by inverted conditions, consider avoiding invertions.">else</weak_warning>
 { c(); d(); }
 
-if (false === expr())
+if (false === returnsBool())
 { a(); b(); }
 <weak_warning descr="[EA] The if-else workflow is driven by inverted conditions, consider avoiding invertions.">else</weak_warning>
 { c(); d(); }
@@ -47,4 +50,9 @@
 else {}
 
 if (!empty([])) {}
-else {}
+else {}
+
+if (false === returnsNullableBool())
+{ a(); b(); }
+else
+{ c(); d(); }