Fix the step-into target on multi line expression. Fix #519

[gayanper]

The fix try to compare the current line number against the target MethodInvocation start line if the debugger lines doesn't match because the current frame is at a wrapped line even though we are executing the same expression.

[gayanper]

@testforstephen Let me know if you see a better condition to use in this scenario.

[testforstephen]

@testforstephen Let me know if you see a better condition to use in this scenario.

I don't get how this fix works. When I tested the snippet you posted in the issue #519, the stepInTargets DAP request returns no results for step-into target operation. Shouldn't we fix MethodInvocationLocator first?

[gayanper]

@testforstephen Let me know if you see a better condition to use in this scenario.

I don't get how this fix works. When I tested the snippet you posted in the issue #519, the stepInTargets DAP request returns no results for step-into target operation. Shouldn't we fix MethodInvocationLocator first?

Let me check on this, I see the same, probably I missed to commit something

[gayanper]

@testforstephen Let me know if you see a better condition to use in this scenario.

I don't get how this fix works. When I tested the snippet you posted in the issue #519, the stepInTargets DAP request returns no results for step-into target operation. Shouldn't we fix MethodInvocationLocator first?

Let me check on this, I see the same, probably I missed to commit something

Try now @testforstephen

[testforstephen]

The condition check unit.getRoot() == node.getRoot() is not needed. I tested #519 snippet without this check, it worked.

[gayanper]

For some reason for me it start to fail before in shouldVisitNode the end becomes -1 since the on JDT side the start + length of the node becomes equal to the end of the compilation unit's last position. I will try again without this check.

[gayanper]

OK now I found the reason, if you don't have a empty line at the end of the class, that after the last class definitions' brace, then the end getLineNumber becomes -1

[testforstephen]

I can reproduce it, but i would prefer to modify the implementation shouldVisitNode() to fix the problem.

Since the offset range of the ast node is [node.getStartPosition, node.getStartPosition() + node.getLength() - 1], the ending line should be got from unit.getLineNumber(node.getStartPosition() + node.getLength() - 1).

The fix could be as follow:

    private boolean shouldVisitNode(ASTNode node) {
        int start = unit.getLineNumber(node.getStartPosition());
        int end = unit.getLineNumber(node.getStartPosition() + node.getLength() - 1);

        if (line >= start && line <= end) {
            return true;
        }

        return false;
    }

[testforstephen]

Look at the caller, actually we took the first parameter as current, the second one as original. So we should change the signature definition to private boolean isSameLocation(Location current, Location original, MethodInvocation targetStepIn).

[testforstephen]

The last condition check should be (targetStepIn != null && targetStepIn.lineEnd >= current.lineNumber()), which supports multiline expression better.

[testforstephen]

I also found another corner case failure when steping target into List.of("1").

new ArrayList<>(List.of("1"))
      .subList(0, 1)
      .addAll(mergedData());

It looks like the generic method signature (e.g. "(LE;)Ljava/util/List;") we parsed from JDT is slightly different with the one from the JDI (e.g. "(Ljava/lang/Object;)Ljava/util/List;"). We could fix it in another PR.

java-debug/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/StepRequestHandler.java

Lines 330 to 345 in 9bdd997

	private boolean isStoppedAtSelectedMethod(StackFrame frame, MethodInvocation selectedMethod) {
	Method method = frame.location().method();
	if (method != null
	&& Objects.equals(method.name(), selectedMethod.methodName)
	&& (Objects.equals(method.signature(), selectedMethod.methodSignature)
	|| Objects.equals(method.genericSignature(), selectedMethod.methodGenericSignature))) {
	ObjectReference thisObject = frame.thisObject();
	ReferenceType currentType = (thisObject == null) ? method.declaringType() : thisObject.referenceType();
	if ("java.lang.Object".equals(selectedMethod.declaringTypeName)) {
	return true;
	}
	return isSubType(currentType, selectedMethod.declaringTypeName);
	}
	return false;

[testforstephen]

I can reproduce it, but i would prefer to modify the implementation shouldVisitNode() to fix the problem.

Since the offset range of the ast node is [node.getStartPosition, node.getStartPosition() + node.getLength() - 1], the ending line should be got from unit.getLineNumber(node.getStartPosition() + node.getLength() - 1).

The fix could be as follow:

    private boolean shouldVisitNode(ASTNode node) {
        int start = unit.getLineNumber(node.getStartPosition());
        int end = unit.getLineNumber(node.getStartPosition() + node.getLength() - 1);

        if (line >= start && line <= end) {
            return true;
        }

        return false;
    }

[testforstephen]

this doesn't work for multiline method invocation. We should use targetStepIn.lineEnd in the last check, e.g. targetStepIn.lineEnd >= current.lineNumber().

For the use case below, step-into target filterMe doesn't work, it stops at the line of "2", "3", .

new ArrayList<>(List.of("1"))
        .subList(0, 1)
        .addAll(filterMe("1", 
            "2", "3", 
            "4"));

[testforstephen]

I have updated the PR to fix the comments. Now it's good to merge.