Skip to content

Fix possible CCE during auto completion of invalid code #9173

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.

Sign up for GitHub

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

Merged
ebarboni merged 1 commit into from
Feb 4, 2026
+1 −1
Merged

Fix possible CCE during auto completion of invalid code #9173

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion java/java.completion/src/org/netbeans/modules/java/completion/JavaCompletionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5996,7 +5996,7 @@ private Set<? extends TypeMirror> getSmartTypesImpl(Env env) throws IOException
path = new TreePath(path, mid);
TypeMirror typeMirror = controller.getTrees().getTypeMirror(path);
final ExecutableType midTM = typeMirror != null && typeMirror.getKind() == TypeKind.EXECUTABLE ? (ExecutableType) typeMirror : null;
final ExecutableElement midEl = midTM == null ? null : (ExecutableElement) controller.getTrees().getElement(path);
final ExecutableElement midEl = midTM != null && controller.getTrees().getElement(path) instanceof ExecutableElement ee ? ee : null;
Comment on lines 5997 to +5999
Copy link
Member Author

@mbien mbien Jan 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

midEl and midTM are only used in

netbeans/java/java.completion/src/org/netbeans/modules/java/completion/JavaCompletionTask.java

Line 6427 in 2fba977

ExecutableType meth = e == prototypeSym && prototype != null ? prototype : (ExecutableType) asMemberOf(e, type, types);

which is ok with any combination of null.

Copy link
Contributor

@lahodaj lahodaj Jan 30, 2026

Choose a reason for hiding this comment

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

Using instanceof on Elements is formally speaking not correct (getKind() should be used), as one implementation type can implement multiple API interfaces. Although I think use of instanceof will be accepted more and more. And it is not so bad for Element, as I don't recall implementation classes where this would be a real problem. (It is definitely a problem for certain TypeMirrors - the implementation type ClassType implements both DeclaredType and ErrorType API interfaces, and this effectively cannot be changed while keeping all other things working reasonably. There are also some rare Trees where this is true.)

Copy link
Member

@neilcsmith-net neilcsmith-net Jan 30, 2026

Choose a reason for hiding this comment

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

Wouldn't mind seeing an example of how this should be written like this. I think I understand what you're getting at but it seems very similar to what was there already.

We don't have a utility that does this check somewhere? Take TreePath, Kind and expected output Class?

Copy link
Member Author

@mbien mbien Jan 30, 2026

Choose a reason for hiding this comment

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

tbh 70% of the reason i wrote it that way is because the new instanceof get-check-and-assign pattern can be put into a one-liner which is the common style throughout the whole class. You could do something similar with Optional but it is less elegant IMO. (but of course this shouldn't be an excuse to write broken code ;))

I did have the Kind vs implements issue in the back of my head since @lahodaj explained it to me once, but I believe in this particular case it probably wouldn't matter the way the two variables are used.

Having TypeMirror of kind Executable but the Element of a type other than ExecutableElement sounds to me like inconsistent state? I suppose thats why the original code casted it without kind check?

Copy link
Member

@neilcsmith-net neilcsmith-net Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I assumed the point was that this should be checking the kind from the element being casted? Although like you say the current situation seems inconsistent. I've looked through a few reports and stack traces before we've had with CCE, and I just get the feeling it might be better to verify types as well as kind before casting. I wondered if we had a utility somewhere already that does that. Bonus if it also checks for null first. Could simplify a few bits of this while keeping the logic in one place.

Copy link
Contributor

@lahodaj lahodaj Jan 30, 2026

Choose a reason for hiding this comment

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

I don't think we have a utility to fuse kind check and cast (although we could have - esp. for trees, as there's Kind.asInterface). Given we don't really support arbitrary Java Language Model (javax.lang.model), but only the one produced by javac, I think using instanceof for ExecutableElement is OKish.

FTR, the lines above show the kind check:

TypeMirror typeMirror = controller.getTrees().getTypeMirror(path);
final ExecutableType midTM = typeMirror != null && typeMirror.getKind() == TypeKind.EXECUTABLE ? (ExecutableType) typeMirror : null;

as Michael says, it is not easy to this without the auxiliary variable, esp. not without duplicating the getTypeMirror call. But, with source level >= 21, we could do:

final ExecutableElement midEl = midTM != null && controller.getTrees().getElement(path) instanceof Element ee && ee.getKind() == ElementKind.METHOD ? (ExecutableElement) ee : null;

As for the inconsistency, the attribution of erroneous code sometimes produces "funny" results - in particular, javac has no real ExecutableElement instance for really erroneous methods, its all either ClassSymbol or specialized subclases of Symbol, but not MethodSymbol/ExecutableElement, I think. I think I had some brief periods where I looked at possibilities to change this, but there I never found a viable solution.

switch (mid.getKind()) {
case MEMBER_SELECT: {
String name = ((MemberSelectTree) mid).getIdentifier().toString();
Expand Down