Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix possible out of bounds exception in switch pattern hint #7973

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

mbien
Copy link
Member

@mbien mbien commented Nov 19, 2024

index can be less than 0

found this issue while testing #7968

this (invalid) snippet:

    public static void main(String[] args) {
        switch (args) {
            case 0 -> {}
         }
    }

will cause:

java.lang.IndexOutOfBoundsException: -1
	at com.sun.tools.javac.util.List.get(List.java:468)
	at org.netbeans.modules.java.hints.jdk.ConvertToSwitchPatternInstanceOf.switchPatternMatchToSwitchNull(ConvertToSwitchPatternInstanceOf.java:281)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) hints labels Nov 19, 2024
@mbien mbien added this to the NB25 milestone Nov 19, 2024
@mbien mbien requested a review from lahodaj November 19, 2024 00:33
@lahodaj
Copy link
Contributor

lahodaj commented Nov 19, 2024

Nice catch. Overall, I agree with the solution, although having a test would be good. Also, we could consider making the code a little bit more understandable, e.g.:

diff --git a/java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOf.java b/java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOf.java
index a295ed153f..f52cd72e33 100644
--- a/java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOf.java
+++ b/java/java.hints/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOf.java
@@ -28,6 +28,7 @@ import com.sun.source.tree.StatementTree;
 import com.sun.source.tree.SwitchTree;
 import com.sun.source.tree.ThrowTree;
 import com.sun.source.tree.Tree;
+import com.sun.source.tree.Tree.Kind;
 import com.sun.source.tree.VariableTree;
 import com.sun.source.util.TreePath;
 import java.util.ArrayList;
@@ -272,18 +273,21 @@ public class ConvertToSwitchPatternInstanceOf {
         }
         Tree expression = ((ParenthesizedTree) switchTree.getExpression()).getExpression();
         Tree parent = (Tree) ctx.getPath().getParentPath().getLeaf();
-        int indexOf;
-        if (parent instanceof BlockTree) {
-            indexOf = ((BlockTree) parent).getStatements().indexOf(switchTree) - 1;
+        int indexOfSwitch;
+        if (parent.getKind() == Kind.BLOCK) {
+            indexOfSwitch = ((BlockTree) parent).getStatements().indexOf(switchTree);
+            if (indexOfSwitch < 1) {
+                return null;
+            }
         } else {
             return null;
         }
-        Tree ifTree = ((BlockTree) parent).getStatements().get(indexOf);
-        if ((!(ifTree instanceof IfTree) || !MatcherUtilities.matches(ctx, new TreePath(ctx.getPath(), ((IfTree) ifTree).getCondition()), "($expr0 == null)", true))
+        Tree ifTree = ((BlockTree) parent).getStatements().get(indexOfSwitch - 1);
+        if (ifTree.getKind() != Kind.IF || !MatcherUtilities.matches(ctx, new TreePath(ctx.getPath(), ((IfTree) ifTree).getCondition()), "($expr0 == null)", true)
                 || !(ctx.getVariables().get("$expr0").getLeaf().toString().equals(expression.toString()))) {
             return null;
         }
-        Fix fix = new FixSwitchPatternMatchToSwitchNull(ctx.getInfo(), ctx.getPath().getParentPath(), indexOf).toEditorFix();
+        Fix fix = new FixSwitchPatternMatchToSwitchNull(ctx.getInfo(), ctx.getPath().getParentPath(), indexOfSwitch).toEditorFix();
         return ErrorDescriptionFactory.forTree(ctx, ifTree, Bundle.ERR_ConvertToSwitchPatternInstanceOf(), fix);
 
     }
@@ -300,11 +304,11 @@ public class ConvertToSwitchPatternInstanceOf {
 
     private static final class FixSwitchPatternMatchToSwitchNull extends JavaFix {
 
-        private final int indexOf;
+        private final int indexOfSwitch;
 
-        public FixSwitchPatternMatchToSwitchNull(CompilationInfo info, TreePath path, int indexOf) {
+        public FixSwitchPatternMatchToSwitchNull(CompilationInfo info, TreePath path, int indexOfSwitch) {
             super(info, path);
-            this.indexOf = indexOf;
+            this.indexOfSwitch = indexOfSwitch;
         }
 
         @Override
@@ -318,9 +322,9 @@ public class ConvertToSwitchPatternInstanceOf {
             TreePath main = ctx.getPath();
             TreeMaker make = wc.getTreeMaker();
             List<Tree> caseNullLabel = new LinkedList<>();
-            SwitchTree switchTree = (SwitchTree) ((BlockTree) main.getLeaf()).getStatements().get(indexOf + 1);
+            SwitchTree switchTree = (SwitchTree) ((BlockTree) main.getLeaf()).getStatements().get(indexOfSwitch);
 
-            Tree ifTree = ((BlockTree) main.getLeaf()).getStatements().get(indexOf);
+            Tree ifTree = ((BlockTree) main.getLeaf()).getStatements().get(indexOfSwitch - 1);
             StatementTree thenStatement = ((IfTree) ifTree).getThenStatement();
             caseNullLabel.add(wc.getTreeMaker().Identifier("null"));
             BlockTree blockTree = (BlockTree)thenStatement;
@@ -328,7 +332,7 @@ public class ConvertToSwitchPatternInstanceOf {
             CaseTree caseMultipleSwitchPatterns = wc.getTreeMaker().CasePatterns(caseNullLabel, statementTree);
             SwitchTree insertSwitchCase = make.insertSwitchCase(switchTree, 0, caseMultipleSwitchPatterns);
             wc.rewrite(switchTree, insertSwitchCase);
-            BlockTree removeBlockStatement = make.removeBlockStatement((BlockTree) main.getLeaf(), indexOf);
+            BlockTree removeBlockStatement = make.removeBlockStatement((BlockTree) main.getLeaf(), indexOfSwitch - 1);
             wc.rewrite(main.getLeaf(), removeBlockStatement);
         }
     }
diff --git a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOfTest.java b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOfTest.java
index 4e25857d72..4d7f7a7a49 100644
--- a/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOfTest.java
+++ b/java/java.hints/test/unit/src/org/netbeans/modules/java/hints/jdk/ConvertToSwitchPatternInstanceOfTest.java
@@ -209,6 +209,24 @@ public class ConvertToSwitchPatternInstanceOfTest extends NbTestCase {
                         + "}\n");
     }
     
+    @Test
+    public void testSwitchWithNullExceptionSwitchIsFirst() throws Exception {
+        HintTest.create()
+                .input("""
+                       package test;
+                       public class Test {
+                           private static void test(Object o) {
+                               switch (o) {
+                                   case String s -> {}
+                               }
+                           }
+                       }
+                       """, false)
+                .sourceLevel("21")
+                .run(ConvertToSwitchPatternInstanceOf.class)
+                .assertWarnings();
+    }
+
     @Test
     public void testSimpleSwitchWithNullNoHint() throws Exception {
         HintTest.create()

 - index can be less than 0 in some situations
 - added test and code simplification suggestions from Jan Lahoda

Co-authored-by: Jan Lahoda <[email protected]>
@mbien mbien force-pushed the fix-switch-hint-ioobe branch from 1c9e957 to 0fa77ff Compare November 19, 2024 13:12
@mbien
Copy link
Member Author

mbien commented Nov 19, 2024

applied the patch and added @lahodaj as co-author

Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@mbien mbien merged commit 7796670 into apache:master Nov 19, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hints Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants