better handling of invocation source positions

[raghav-deepsource]

Currently, Spoon's representation of method invocations sets the source positions in a rather inconvenient way.

Consider a CtInvocation node corresponding to the call to getContextBuilder() in this snippet:

int[] lineSeparatorPositions = jdtTreeBuilder.getContextBuilder().getCompilationUnitLineSeparatorPositions();

If we were to look at the spoon node of getContextBuilder(), we would see that the node's extents encompass a lot more than just that one method call:

//                             v--------------------------------v
int[] lineSeparatorPositions = jdtTreeBuilder.getContextBuilder().getCompilationUnitLineSeparatorPositions();

Would it be a good idea to special case method calls to add extra info, similar to what happens for declarations? Instead of just a normal source position, there may be a CompoundSourcePosition (or something similarly structured) which can also be used to record the position of the content specific to the node:

//                                      The complete node
//                             /--------------------------------\
//                             |                method call     |
//                             v             v------------------v
int[] lineSeparatorPositions = jdtTreeBuilder.getContextBuilder().getCompilationUnitLineSeparatorPositions();

This would be very helpful when we wish to know only the position of one method call in a call chain.

Regarding how this can be done, I have some questions:

Is PositionBuilder.buildPositionElement() where source positions get generated?
Is MessageSend (or one of its super types) the node type one would need to check for when handling invocations?

It seems like MessageSend.nameSourcePosition may be something I can use to do something about this, but I'm not very sure.

[I-Al-Istannen]

I am not exactly sure what you want. The CtExecutableReference of the invocation sadly has no source position and that might be worth fixing.

But I got the following result:

class Foo { char foo(String string) { return string.charAt(20);  } }
                                             ^    ^                     - Target is 'string'

class Foo { char foo(String string) { return string.charAt(20);  } }
                                                    ^        ^          -  Rest is 'charAt(20)'

class Foo { char foo(String string) { return string.charAt(20);  } }
                                                    ^    ^              - Method name is 'charAt'

with the following code (looks a bit golfed but I wanted it to fit in a single function to make it easier for you to test)

  public void foo() {
    BiFunction<String, Integer, String> repeat = (string, count) -> {
      StringBuilder result = new StringBuilder();
      for (int i = 0; i < count; i++) {
        result.append(string);
      }
      return result.toString();
    };

    BiFunction<Integer, Integer, String> lineFromTo = (from, to) ->
        repeat.apply(" ", from)
            + "^"
            + repeat.apply(" ", to - from - 1)
            + "^";

    String code = "class Foo { char foo(String string) { return string.charAt(20);  } }";
    CtClass<?> aClass = Launcher.parseClass(
        code
    );
    CtMethod<?> method = aClass.getMethodsByName("foo").get(0);
    CtReturn<?> aReturn = method.getBody().getStatement(0);
    CtInvocation<?> invocation = (CtInvocation<?>) aReturn.getReturnedExpression();

    SourcePosition targetPosition = invocation.getTarget().getPosition();
    System.out.println(code);
    System.out.printf(
        "%s                     - Target is '%s'%n",
        lineFromTo.apply(targetPosition.getSourceStart(), targetPosition.getSourceEnd()),
        code.substring(targetPosition.getSourceStart(), targetPosition.getSourceEnd() + 1)
    );

    System.out.println();

    SourcePosition invocationPosition = invocation.getPosition();
    System.out.println(code);
    System.out.printf(
        "%s          -  Rest is '%s'%n",
        lineFromTo.apply(targetPosition.getSourceEnd() + 2, invocationPosition.getSourceEnd()),
        code.substring(
            targetPosition.getSourceEnd() + 2, invocationPosition.getSourceEnd() + 1
        )
    );

    if (!invocation.getArguments().isEmpty()) {
      System.out.println();
      System.out.println(code);
      SourcePosition sourcePosition = invocation.getArguments().get(0).getPosition();

      System.out.printf(
          "%s              - Method name is '%s'%n",
          lineFromTo.apply(targetPosition.getSourceEnd() + 2, sourcePosition.getSourceStart() - 2),
          code.substring(targetPosition.getSourceEnd() + 2, sourcePosition.getSourceStart() - 1)
      );
    }
  }

[raghav-deepsource]

The CtExecutableReference of the invocation sadly has no source position and that might be worth fixing

Indeed. I was looking at how source positions were generated and stumbled across PositionBuilder. If logic were to be written to add a source position to reference nodes, would it be done in this class?

[slarse]

So, like @I-Al-Istannen said, the source position of the method call itself is entirely appropriate. The target is part of the method call, as is the executable reference and all of the arguments passed to it.

The place to build the position for the executable reference would be somewhere around JDTTreeBuilder.visit(MessageSend messageSend, BlockScope scope), but I haven't looked into that particular part of the code base. The thing that sets most positions is the ContextBuilder, which uses PositionBuilder internally.

[I-Al-Istannen]

I added a source position to executable references in #5011. I didn't even remember this issue existed. I think we might be able to close this one now :^)

[raghav-deepsource]

Yup, thank you for taking this up!