solves NPE in RemoveUnnecessary.java

[homberghp]

The NPE is thrown in java/java.hints/src/org/netbeans/modules/java/hints/control/RemoveUnnecessary.java:117, tp.getLeaf().getKind() does not verify that the return value of tp.getLeaf() is non-null;

The solution tests for non-null-ness and continues the while loop if the return value is null, thereby avoiding the NPE and continuing with the next iteration in the while loop.

---

^Add meaningful description above

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

• are all your own work, and you have the right to contribute them.
• are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

1. Was this PR correctly labeled, did the right tests run? When did they run?
2. Is this PR squashed?
3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

[mbien]

I had a note in my stash to find a reproducer for this - but never ended up finding one. But I agree we should add a defensive check there to prevent the NPE.

[lahodaj]

I am sorry, but this is not right. I don't think the leaf can be null, by construction. The (future) leaf is always dereferenced by the TreePath constructor: https://github.com/openjdk/jdk/blob/3b32f6a8ec37338764d3e6713247ff96e49bf5b3/src/jdk.compiler/share/classes/com/sun/source/util/TreePath.java#L118

TreePath.getParentPath() may return null, and that will be the culprit here. I think there should be a test for tp == null, and probably return null;. Testcase is:

    public void testStandaloneReturn() throws Exception {
        HintTest.create()
                .input("""
                       package test;
                       public class Test {
                       }
                       return;
                       """,
                       false)
                .run(RemoveUnnecessary.class)
                .assertWarnings();
    }

[homberghp]

I should have saved the error message that NetBeans produced via a bulb at the bottom status line.
That exactly pointed to the switch(tp.getLeaf().getKind()) statement on line 117,
with the message that getLeaf() returns null.
A defensive check if tp == null and then return or continue probably also will not hurt. Either way the next iteration of the loop will find the while condition false when tp==null;

If null is an acceptable return value for the method, then the suggestion by mbien is probably the better one. I chose to follow the pattern inside the case labels which tend to continue the loop.

netbeans/java/java.hints/src/org/netbeans/modules/java/hints/control/RemoveUnnecessary.java

Lines 108 to 134 in 049b5ed

	private static ErrorDescription unnecessaryReturnContinue(HintContext ctx, Tree targetLoop, String key, boolean isReturn) {
	TreePath tp = ctx.getPath();
	OUTER: while (tp != null && !TreeUtilities.CLASS_TREE_KINDS.contains(tp.getLeaf().getKind())) {
	Tree current = tp.getLeaf();
	List<? extends StatementTree> statements;
	tp = tp.getParentPath();
	switch (tp.getLeaf().getKind()) {
	case METHOD: {
	if (targetLoop != null) return null; //TODO: unnecessary continue - can happen?
	MethodTree mt = (MethodTree) tp.getLeaf();
	if (mt.getReturnType() == null) {
	if (mt.getName().contentEquals("<init>"))
	break OUTER;//constructor
	else
	return null; //a method without a return type - better ignore
	}
	TypeMirror tm = ctx.getInfo().getTrees().getTypeMirror(new TreePath(tp, mt.getReturnType()));
	if (tm == null || tm.getKind() != TypeKind.VOID) return null;
	break OUTER;
	}
	case LAMBDA_EXPRESSION: {

[mbien]

please add the testcase @lahodaj posted in #8575 (comment) to RemoveUnnecessaryTest

edit: feel free to squash and force push

[homberghp]

I am sorry, but this is not right. I don't think the leaf can be null, by construction. The (future) leaf is always dereferenced by the TreePath constructor: https://github.com/openjdk/jdk/blob/3b32f6a8ec37338764d3e6713247ff96e49bf5b3/src/jdk.compiler/share/classes/com/sun/source/util/TreePath.java#L118

TreePath.getParentPath() may return null, and that will be the culprit here. I think there should be a test for tp == null, and probably return null;. Testcase is:

    public void testStandaloneReturn() throws Exception {
        HintTest.create()
                .input("""
                       package test;
                       public class Test {
                       }
                       return;
                       """,
                       false)
                .run(RemoveUnnecessary.class)
                .assertWarnings();
    }

Your test points exactly at the culprit, it becomes red, the true colour of a useful test, when I comment the solution out.
So your solution (and added test) it is.

Thanks mbien and lahodaj.

[homberghp]

I should pay better attention when squashing ;-((
I hope I did not break anything.

[mbien]

I should pay better attention when squashing ;-((

@homberghp can you run this line on your local branch:

git pull --rebase --autostash [email protected]:apache/netbeans master

This will remove all merge commits and move your commit on top of everything.

then you force push into this PR:

git push -f [email protected]:homberghp/netbeans npeinRemoveUnnecessary:npeinRemoveUnnecessary

now your PR should only contain one commit with your changes

[homberghp]

thank you, I needed that hint.

[homberghp]

As far as git is concerned I am still a newcomer, but thankfully I have good and patient teachers.
Thx.

[mbien]

As far as git is concerned I am still a newcomer, but thankfully I have good and patient teachers. Thx.

no worries. Many other projects don't require to squash before merge because the committer who integrates would squash using github. But there were problems with that in past, so we decided to merge commits as-is.

[mbien]

looks good. @lahodaj thanks for the reproducer and the help.

will merge once CI is green

[lahodaj]

Looks good to me.