MessageSend incorrectly uses "ThisReference" as receiver for static imports and methods from enclosing classes

[I-Al-Istannen]

Hey,

I am trying to accurately model static imports and ran into a problem. Consider the following code:

import static java.lang.System.lineSeparator;
public class Test {
    public void outerMethod() {}
    public static void staticOuterMethod() {}

    public class Inner {
        void entrypoint() {
            entrypoint();
            outerMethod();
            staticOuterMethod();
            lineSeparator();
        }
    }
}

The entrypoint method contains four method calls without an explicit receiver.
Currently JDT sets it to ThisReferene in every call:

messageSend = {MessageSend@2966} "outerMethod()"
 receiver = {ThisReference@2972} ""

messageSend = {MessageSend@3088} "staticOuterMethod()"
 receiver = {ThisReference@3092} ""

messageSend = {MessageSend@3103} "lineSeparator()"
 receiver = {ThisReference@3108} ""

IMHO the receiver should differ:

1. ✔️ for entrypoint() the correct receiver is ThisReference
2. ❎ for outerMethod() the correct receiver is the implicit instance bound by the non-static inner class. This should probably be modeled as an implicit qualified this reference?
3. ❎ for staticOuterMethod() the correct receiver is Test
4. ❎ for lineSeparator() the correct receiver is java.lang.System

Additionally, receiver.isImplicitThis() returns true in all of these cases.

As far as I can see this is intentional but I don't understand why this decision was made. It looks wrong to me?

eclipse.jdt.core/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/parser/Parser.java

Line 5585 in 3515713

// when the name is only an identifier...we have a message send to "this" (implicit)

[stephan-herrmann]

As far as I can see this is intentional but I don't understand why this decision was made. It looks wrong to me?

As you found the correct location inside the parser, you halfway answered this yourself: this is the parser, which has no idea, whether the target method is static or not.

Later, when the method is resolved, it is not a good idea to invent synthetic AST to represent the static receiver.

Couldn't you infer all the information you need from the resolved MethodBinding instead? I.e., if it isStatic() then declaringClass will represent that receiver class.

As for outerMethod() the compiler cares about the outer business mainly during code generation, see BlockScope.getEmulationPath(). But at that point there is no need to persist this information other than in the resulting byte code.

BTW: You are referring to the internal compiler AST (which is not API), but I thought spoon uses the public DOM AST?

[I-Al-Istannen]

Later, when the method is resolved, it is not a good idea to invent synthetic AST to represent the static receiver.

Is there any specific reason? I was planning (and do) just that in my PR. I currently insert a fake type referene / this reference where appropriate. It felt nicer than having a wrong this reference in your AST. I am curious whether your concerns apply to my changes to Spoon as well :)

Couldn't you infer all the information you need from the resolved MethodBinding instead? I.e., if it isStatic() then declaringClass will represent that receiver class.

I suppose so. I currently check whether actualReceiverType and receiver.resolvedType match. If they don't we have one of the lower three cases (AFICT). I then need to disambiguate them and replace the receiver by something useful. Currently I just created a reference to the resolved type, but that breaks for outer methods. I guess I could check whether the method is static and manually re-implement some parts of getEmulationPath. I should probably replace it with a qualified this reference rather than a synthetic field access, as spoon is a bit higher level.

I had just hoped I could circumvent doing all of this myself, as this is yet another hack on the pile of JDT-workarounds. I understand why your parser isn't happy to disambiguate that and it seems like later stages don't really care about the AST layout and clean things up themselves when needed (once, for codegen).

BTW: You are referring to the internal compiler AST (which is not API), but I thought spoon uses the public DOM AST?

I wish. I have never looked at the official API, so I can't say much about it. Last I heard the public API did not provide all the necessary information to construct a usable Spoon-AST. Spoon is very often used without large chunks of the classpath and tries to build a usable AST with quite a lot of information. Maybe that has changed by now, but this decision was made over a decade before I became affiliated with the project (or started Programming) :)
I can't give you an accurate statement for why it was done this way, but the time it would take to migrate is probably measured in years by now.

[stephan-herrmann]

@I-Al-Istannen it's been a while since. Is this issue still relevant for you, or is accessing such information from bindings rather than AST a viable option for you? I.e., can we close this issue? :)

[I-Al-Istannen]

I don't remember what we did for this to be honest, it truly has been a while. Looking at the linked issue past-me said he hacked around this on the spoon side. I sadly don't particularly trust that guy, but let's take his word.

I think we can close this :)