Fix possible CCE during auto completion of invalid code

[mbien]

javac was observed to return Elements of types other than ExecutableElement for paths with TypeMirror kind EXECUTABLE in some cases.

adds an instanceof guard instead of assuming ExecutableElement

fixes #9172

[mbien]

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.

[lahodaj]

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.)

[neilcsmith-net]

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?

[mbien]

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?

[neilcsmith-net]

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.

[lahodaj]

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.

[lkishalmi]

Seems to be right.

[lahodaj]

Looks OK.

[lahodaj]

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.)