Add support for lambda breakpoints#427
Conversation
|
@testforstephen let me know what you think about this change. May be the name of the breakpoint displayed in UI might need a change. I just named it as lambda breakpoint. |
074ac0a to
a93f9a3
Compare
|
By the way I had to add a new locator implementation to find LambdaExpressions since the JDT.Core doesn't work out of the box since it works with selection offsets |
|
@gayanper Is there any code snippet to demo how this works? |
LambdaDebug.mp4 |
There was a problem hiding this comment.
I tried adding an inline breakpoint on a lambda, it works well with current implementation. This is a good start for inline breakpoint support. But I'm not sure whether current approach can be extended to other inline breakpoint case. I'm thinking whether there is a systematic way to handle inline breakpoint.
...g.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java
Outdated
Show resolved
Hide resolved
I think we can add support for that as well. The same class we use to search for Lambda expression can be evolve into finding MethodInvocations at AST level at the given column and create the breakpoint for that particular method like we use for method header breakpoints and lambda. I think i can work on it if you think that will add value. I have not come across java IDEs provide such a feature. So may be it will be really good one to have. |
...rosoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/LambdaExpressionLocator.java
Outdated
Show resolved
Hide resolved
We can release the lambda inline breakpoint first. Currently, inline breakpoints are not so obvious to new users. I believe one day we need to support BreakpointLocationsRequest microsoft/vscode-java-debug#1193, which will hint user the possible inline breakpoints in a line. |
Thats on my radar as well ;) |
9e7a20a to
6eef4e8
Compare
There was a problem hiding this comment.
Found two bugs with the snippet list.stream().map(▮name -> name).forEach((▮item) -> System.out.println(item)):
- If adding 2+ inline breakpoints in the same line, only one is added successfully.
- The second inline breakpoint is always recognized as
lambda$1, but it's supposed to belambda$2.
...rosoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/LambdaExpressionLocator.java
Outdated
Show resolved
Hide resolved
6eef4e8 to
a22dcd0
Compare
The first issue still exists. The reason is current BreakpointManager uses LineNumber to check if breakpoints are the same. For the inline breakpoints on the same line, the BreakpointManager will add them as the same inline breakpoint. And the optimized lambda search works for single line lambdas, but does not work for multi-line lambdas. For mult-line lambda, one idea is to limit to the available range from the lambda start position to the body start position. |
|
@testforstephen i will look at this in the evening. Did a quick correction in the morning when i saw that i was using wrong column variable. But i think for multi line spanning we need another fix |
The support for method header breakpoints was extended to support lambda breakpoints using the vscode inline breakpoint feature.
Only search for lambda when the breakpoing is an inline breakpoint. Add support for any column position within lambda expression.
|
@testforstephen i was wondering if we need to send back the column to client as well ? |
Are there any clients that can consume the returned column info? If so, it would be good to return the actual column covered by the breakpoint. |
|
The protocol has the support and I thought to have the column in the internal breakpoin to make it unique and use line:column as the map key. Now since we have the column we can send it back. Not sure if vscode will use it for anything or not. |
a62d1fc to
12047f8
Compare
12047f8 to
ec133c3
Compare
...rosoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/LambdaExpressionLocator.java
Show resolved
Hide resolved
|
|
||
| if ((startLine == endLine && column >= startColumn && column <= endColumn && line == startLine) | ||
| || (startLine != endLine && line >= startLine && line <= endLine | ||
| && (column >= startColumn || column <= endColumn))) { |
There was a problem hiding this comment.
this condition only works for expression-style lambda with 2 lines.
There was a problem hiding this comment.
I tested for
Arrays.asList(1, 2, 3).stream().map(name -> name).forEach(🔴(item) -> {
System.out.println(item + 1 + 2 + 3 + 4 + 5);
System.out.println(item + 1 + 2 + 3 + 4 + 5);
System.out.println(item + 1 + 2 + 3 + 4 + 5);
System.out.println(item + 1 + 2 + 3 + 4 + 5);
});
and
Arrays.asList(1, 2, 3).stream().map(name -> name).forEach(
(item) -> 🔴{
System.out.println(item + 1 + 2 + 3 + 4 + 5);
System.out.println(item + 1 + 2 + 3 + 4 + 5);
System.out.println(item + 1 + 2 + 3 + 4 + 5);
System.out.println(item + 1 + 2 + 3 + 4 + 5);
});
Its working, can you give a example snippet ?
There was a problem hiding this comment.
Case 1:
This will hit lambda entry on line 21.
Case 2:
This will hit lambda entry on line 22.
(startLine != endLine && line >= startLine && line <= endLine
&& (column >= startColumn || column <= endColumn))
It is not accurate to compare the column number for multi-line lambda. It might be better to compare the offset.
There was a problem hiding this comment.
True. But I couldn’t find a method to calculate offset by providing the line and column. But will check more to see if i have missed any such method. JDT should have such methods, not sure if they are part of the API.
There was a problem hiding this comment.
You can use this.compilationUnit#getPosition(line, column) to get the offset.
There was a problem hiding this comment.
Thanks. I think i missed it. The problem with current condition is I didn’t intend to support blocks. But still if we only do boundry check and ignore middle lines it will work when we have multi lines. But position is much simpler. I will switch to that.
There was a problem hiding this comment.
For the scenario 1 as user what you would expect ?
There was a problem hiding this comment.
For the scenario 1 as user what you would expect ?
I expect adding a breakpoint on (Line 22, Column 24), and if Column 24 breakpoint is not supported, then fallback to a line breakpoint on Line 22.
If it's inside a block body, I'll treat it as a normal function body and not fall back to the Lambda entry breakpoint.
There was a problem hiding this comment.
LambdaExpression.getBody() will return either a Block or an Expression, which we can use to determine which lambda it is.
If it's an expression-style lambda, it's ok to enable Lambda breakpoint if the offset is within the whole LambdaExpression range.
If it's a block-style lambda, I would enable Lambda breakpoint only if the offset is in [LambdaStart, LambdaBodyStart).
Just my two cents.
There was a problem hiding this comment.
Now keeping mind the future expansion of inline breakpoints for method invocations, I decided to restrict the lambda breakpoints by
- only support for expressions
- the inline breakpoint should be before the expression
This way we will not break any functionality nor complicate breakpoint logic in future when we introduce method invocation inline breakpoints. And for lambda blocks we can always use line breakpoints which is much clear and straight forward.
WDYT ?
- Only support for expressions. - The inline breakpoint should be before the expression start.
f3a7a1e to
6dcb88d
Compare
Since LambdaExpression.getNodeType() returns `ASTNode.LAMBDA_EXPRESSION`, the condition `node.getNodeType() != ASTNode.BLOCK` is always true and i just remove it.
There was a problem hiding this comment.
I agree with simplifying the condition check of lambda breakpoint. Checking for the offset within [lambdaStart, bodyStart] works well for me.
One minor comment is to remove the unncessary nodeType check. Since LambdaExpression.getNodeType() returns ASTNode.LAMBDA_EXPRESSION, the condition node.getNodeType() != ASTNode.BLOCK is always true and i just remove it from the commit.
Others look good to me.
|
Had a try on the typescript inline breakpoint, it would show an extra cursor on the target column when hitting on inline breakpoint. It's a better experience we can leverage.
|
|
The support for method header breakpoints was extended to support
lambda breakpoints using the vscode inline breakpoint feature.