Community
Participate
Working Groups
package workbench; import java.util.stream.Stream; public class App { public static void main(String[] args) { Stream<Integer> stream = Stream.of(1, 2); if (stream.filter(i -> i > 1).) { String msg = "PASS"; System.out.println(msg); } } } Trigger content assist behind stream.filter(i -> i > 1). and not suggestions are shown. But if you remove the statements in the if block, then there will be completions.
Further looking at if-else we see the same issue with more limitations package workbench; import java.util.stream.Stream; public class App { public static void main(String[] args) { Stream<Integer> stream = Stream.of(1, 2); if (stream.filter(i -> i > 1).count() == 0) { String msg = "PASS"; System.out.println(msg); } else if (stream.allMatch(i -> i == 1)) { String msg = "PASS"; System.out.println(msg); } } } Under the if-else no completions at all, not even the completion for stream variable. But as earlier removing the if-else block statements will give you correct bahavior.
Gayan, are you on latest 4.21 build? There were few fixes from Stephan (latest one in bug 574803), and one is still pending (bug 574704).
(In reply to Andrey Loskutov from comment #2) > Gayan, are you on latest 4.21 build? There were few fixes from Stephan > (latest one in bug 574803), and one is still pending (bug 574704). Yes I’m on the latest ibuild release yesterday. I can have a look at this in the evening.
(In reply to Andrey Loskutov from comment #2) > Gayan, are you on latest 4.21 build? There were few fixes from Stephan > (latest one in bug 574803), and one is still pending (bug 574704). bug 574704 has been resolved. Nothing happened yet in bug 574803, though.
(In reply to Gayan Perera from comment #3) > (In reply to Andrey Loskutov from comment #2) > > Gayan, are you on latest 4.21 build? There were few fixes from Stephan > > (latest one in bug 574803), and one is still pending (bug 574704). > > Yes I’m on the latest ibuild release yesterday. I can have a look at this in > the evening. Did you have a chance yet? As a first indicator, what is the parsed AST?
(In reply to Stephan Herrmann from comment #5) > > Did you have a chance yet? > > As a first indicator, what is the parsed AST? Started to look at, but couldn't find a solution. What I observed is there is no CompletionNodeFound exception. I couldn't look at the Parser since i switch to work with the Meta Index issues again.
(In reply to Gayan Perera from comment #6) > (In reply to Stephan Herrmann from comment #5) > > > > Did you have a chance yet? > > > > As a first indicator, what is the parsed AST? > > Started to look at, but couldn't find a solution. What I observed is there > is no CompletionNodeFound exception. Thanks. That could mean (a) there is no completion node, or (b) that node was not visited during resolved. Perhaps the completion node remained an orphan, i.e., wasn't attached into the AST. > I couldn't look at the Parser since i > switch to work with the Meta Index issues again. I understand. If I find the time to look at it I'll first drop a quick comment here.
@Stephan i had a look into this with the code: import java.util.ArrayList; public class Bug574823 { public void foo() { ArrayList<String> ints = new ArrayList<String>(); if(ints.subList(1,1).) { String message = "PASS"; System.out.println(message); } } } The stack looks like this when we are at org.eclipse.jdt.internal.codeassist.complete.CompletionParser.attachOrphanCompletionNode() at line 703 [<CompleteOnMemberAccess:ints.subList(1, 1).>, System] But if we have ints.iterator() instead of ints.subList(1,1) the expression stack doesn't have System. I checked whats happening after System name parsed. Seems like actFromTokenOrSynthetic returns ERROR_ACTION just after the System simple name is parsed. I will check again after a break to see if i find something related to this.
Gayan, why did you removed dependency from bug 574913 to this one?
(In reply to Gayan Perera from comment #8) > The stack looks like this when we are at which stack? > org.eclipse.jdt.internal.codeassist.complete.CompletionParser. > attachOrphanCompletionNode() at line 703 > > [<CompleteOnMemberAccess:ints.subList(1, 1).>, System] > > But if we have ints.iterator() instead of ints.subList(1,1) the expression > stack doesn't have System. > > I checked whats happening after System name parsed. Seems like > actFromTokenOrSynthetic returns ERROR_ACTION just after the System simple > name is parsed. What's happening to the local declaration "message"? Why should (not) having "System" make a difference? Since there's no lambda in sight, I'd expect we set EOF right after the '.' (perhaps including the ')'). Does the assist node remain unattached? Or is it the IfStatement that does / doesn't appear in the parse AST - and could be needed by CompletionEngine?
(In reply to Stephan Herrmann from comment #10) > which stack? Expression stack > > > org.eclipse.jdt.internal.codeassist.complete.CompletionParser. > > attachOrphanCompletionNode() at line 703 > > > > [<CompleteOnMemberAccess:ints.subList(1, 1).>, System] > > > > But if we have ints.iterator() instead of ints.subList(1,1) the expression > > stack doesn't have System. > > > > I checked whats happening after System name parsed. Seems like > > actFromTokenOrSynthetic returns ERROR_ACTION just after the System simple > > name is parsed. > > What's happening to the local declaration "message"? It's not inside the expression stack, but its part of the recovered block. Recovered block: { } Recovered local variable: String message; > Why should (not) having "System" make a difference? Basically the we assume the top item in the expression stack is a enclosing AST for the assistNode, we try to use that with CompletionNodeDetector which fails to find anything. Also this System simple name is also available if we have method chain with parameters inside the if condition. I couldn't still find the difference between that parsing. > > Since there's no lambda in sight, I'd expect we set EOF right after the '.' > (perhaps including the ')'). > > Does the assist node remain unattached? Yes > > Or is it the IfStatement that does / doesn't appear in the parse AST - and > could be needed by CompletionEngine? Ifstatment is also not attached and assistNode also not attached. The parsed CU looks like this import java.util.ArrayList; public class Bug574823 { public Bug574823() { } public void foo() { ArrayList<String> ints; { String message; } } }
Found the root cause, Code block tested on import java.util.ArrayList; public class Bug574823 { public void foo() { ArrayList<String> ints = new ArrayList<String>(); if(ints.subList(1,1).) { System.out.println(message); } } } This is the parser debug output -- ENTER INSIDE PARSE METHOD -- - Start -------------------------------- Shift - (import) Reduce - EnterCompilationUnit EnterCompilationUnit ::= ---------------------------------------- Shift - (Identifier) Shift - (.) Reduce - Name Name ::= SimpleName ---------------------------------------- Shift - (Identifier) Shift/Reduce - (.) QualifiedName QualifiedName ::= Name DOT SimpleName ---------------------------------------- Shift - (Identifier) Shift/Reduce - (;) QualifiedName QualifiedName ::= Name DOT SimpleName ---------------------------------------- Reduce - RejectTypeAnnotations RejectTypeAnnotations ::= - SingleTypeImportDeclarationName SingleTypeImportDeclarationName ::= import Name... ---------------------------------------- Shift/Reduce - (public) SingleTypeImportDeclaration SingleTypeImportDeclaration ::=... ---------------------------------------- Reduce - ReduceImports ReduceImports ::= ---------------------------------------- Shift - (class) Reduce - Modifiersopt Modifiersopt ::= Modifiers ---------------------------------------- Shift - (Identifier) Shift/Reduce - ({) ClassHeaderName1 ClassHeaderName1 ::= Modifiersopt class Identifier ---------------------------------------- Reduce - ClassHeaderName ---------------------------------------- Reduce - ClassHeaderExtendsopt ---------------------------------------- Reduce - ClassHeaderImplementsopt ---------------------------------------- Reduce - ClassHeaderPermittedSubclassesopt - ClassHeader ClassHeader ::= ClassHeaderName ClassHeaderExtendsopt... ---------------------------------------- Shift - (public) Reduce - NestedType NestedType ::= ---------------------------------------- Shift - (void) Reduce - Modifiersopt Modifiersopt ::= Modifiers ---------------------------------------- Reduce - TypeAnnotationsopt TypeAnnotationsopt ::= ---------------------------------------- Shift/Reduce - (Identifier) PrimitiveType ---------------------------------------- Reduce - Type Type ::= PrimitiveType ---------------------------------------- Shift - (() Shift/Reduce - ()) MethodHeaderName MethodHeaderName ::= Modifiersopt Type Identifier LPAREN ---------------------------------------- Reduce - FormalParameterListopt FormalParameterListopt ::= ---------------------------------------- Shift/Reduce - ({) MethodHeaderRightParen MethodHeaderRightParen ::= RPAREN ---------------------------------------- Reduce - Dimsopt Dimsopt ::= - MethodHeaderExtendedDims MethodHeaderExtendedDims ::= Dimsopt ---------------------------------------- Reduce - MethodHeaderThrowsClauseopt - MethodHeader MethodHeader ::= MethodHeaderName FormalParameterListopt ---------------------------------------- Reduce - NestedMethod NestedMethod ::= ---------------------------------------- Shift - (}) Reduce - BlockStatementsopt BlockStatementsopt ::= ---------------------------------------- Shift/Reduce - (}) MethodBody MethodBody ::= NestedMethod LBRACE BlockStatementsopt... - MethodDeclaration MethodDeclaration ::= MethodHeader MethodBody ---------------------------------------- Reduce - ClassBodyDeclarations ---------------------------------------- Reduce - ClassBodyDeclarationsopt ClassBodyDeclarationsopt ::= NestedType... ---------------------------------------- Shift/Reduce - (Unexpected End Of File) ClassBody - ClassDeclaration ClassDeclaration ::= ClassHeader ClassBody ---------------------------------------- Reduce - InternalCompilationUnit InternalCompilationUnit ::= ImportDeclarations... - CompilationUnit CompilationUnit ::= EnterCompilationUnit... - Goal ---------------------------------------- - End ---------------------------------- -- EXIT FROM PARSE METHOD -- -- ENTER INSIDE PARSE METHOD -- - Start -------------------------------- Shift - (Identifier) Shift - (<) Reduce - Name Name ::= SimpleName ---------------------------------------- Reduce - ClassOrInterface ClassOrInterface ::= Name ---------------------------------------- Shift - (Identifier) Shift - (>) Reduce - Name Name ::= SimpleName ---------------------------------------- Reduce - ClassOrInterface ClassOrInterface ::= Name ---------------------------------------- Reduce - ReferenceType ReferenceType ::= ClassOrInterfaceType ---------------------------------------- Shift/Reduce - (Identifier) ReferenceType1 ReferenceType1 ::= ReferenceType GREATER - TypeArguments TypeArguments ::= LESS TypeArgumentList1 - GenericType GenericType ::= ClassOrInterface TypeArguments ---------------------------------------- Reduce - ReferenceType ReferenceType ::= ClassOrInterfaceType ---------------------------------------- Reduce - PushModifiers PushModifiers ::= ---------------------------------------- Shift - (=) Reduce - Dimsopt Dimsopt ::= - VariableDeclaratorId ---------------------------------------- Reduce - EnterVariable EnterVariable ::= ---------------------------------------- Shift - (new) Reduce - ForceNoDiet ForceNoDiet ::= ---------------------------------------- Shift - (Identifier) Shift - (<) Reduce - Name Name ::= SimpleName ---------------------------------------- Reduce - ClassOrInterface ClassOrInterface ::= Name ---------------------------------------- Shift - (Identifier) Shift - (>) Reduce - Name Name ::= SimpleName ---------------------------------------- Reduce - ClassOrInterface ClassOrInterface ::= Name ---------------------------------------- Reduce - ReferenceType ReferenceType ::= ClassOrInterfaceType ---------------------------------------- Shift/Reduce - (() ReferenceType1 ReferenceType1 ::= ReferenceType GREATER - TypeArguments TypeArguments ::= LESS TypeArgumentList1 - GenericType GenericType ::= ClassOrInterface TypeArguments ---------------------------------------- Reduce - ClassOrInterfaceType ---------------------------------------- Reduce - ClassType ---------------------------------------- Reduce - EnterInstanceCreationArgumentList EnterInstanceCreationArgumentList ::= ---------------------------------------- Shift - ()) Reduce - ArgumentListopt ArgumentListopt ::= ---------------------------------------- Shift - (;) Reduce - UnqualifiedClassBodyopt UnqualifiedClassBodyopt ::= - ClassInstanceCreationExpression ClassInstanceCreationExpression ::= new ClassType... ---------------------------------------- Reduce - Expression Expression ::= AssignmentExpression ---------------------------------------- Reduce - RestoreDiet RestoreDiet ::= ---------------------------------------- Reduce - ExitVariableWithInitialization ExitVariableWithInitialization ::= - VariableDeclarator ---------------------------------------- Reduce - LocalVariableDeclaration LocalVariableDeclaration ::= Type PushModifiers... ---------------------------------------- Shift/Reduce - (if) LocalVariableDeclarationStatement LocalVariableDeclarationStatement ::=... ---------------------------------------- Reduce - BlockStatements BlockStatements ::= BlockStatement ---------------------------------------- Shift - (() Shift - (Identifier) Shift - (.) Reduce - Name Name ::= SimpleName ---------------------------------------- Shift - (Identifier) Shift/Reduce - (() QualifiedName QualifiedName ::= Name DOT SimpleName ---------------------------------------- Reduce - Name ---------------------------------------- Shift - (IntegerLiteral) Shift - (,) Reduce - Expression Expression ::= AssignmentExpression - ArgumentList ---------------------------------------- Shift - (IntegerLiteral) Shift - ()) Reduce - Expression Expression ::= AssignmentExpression - ArgumentList ArgumentList ::= ArgumentList COMMA Expression ---------------------------------------- Reduce - ArgumentListopt ---------------------------------------- Shift/Reduce - (.) MethodInvocation MethodInvocation ::= Name LPAREN ArgumentListopt RPAREN ---------------------------------------- Reduce - Primary ---------------------------------------- Shift - (Identifier) Shift - ()) Reduce - FieldAccess FieldAccess ::= Primary DOT Identifier ---------------------------------------- Reduce - Expression Expression ::= AssignmentExpression ---------------------------------------- Shift - ({) Reduce - OpenBlock OpenBlock ::= ---------------------------------------- Shift - (Identifier) Shift - (Unexpected End Of File) Reduce - Name Name ::= SimpleName ---------------------------------------- Reduce - PostfixExpression PostfixExpression ::= Name ---------------------------------------- Error - At the end of the if conditions "." we return EOF from fetchNextToken, but the parser doesn't stop parsing even though the logic seems to be to stop parsing anymore. The reason is the next actFromTokenOrSynthetic actually returns a act for the SimpleName to be captured the rest as a PostfixExpression and fail. This seems to be the reason we have a unexpected expression stack which cause the orphanNode attachment not to work. So i tried to simulate a ERROR act when we have a EOF token in actFromTokenOrSynthetic which solves this problem. Is there any other way to stop parsing further when these conditions are met other than from actFromTokenOrSynthetic ?
That's strange: completion needs to stop at fake-EOF to work, but most recent changes concern avoiding fake-EOF in the first place. Therefor I wonder, which change can possibly cause the regression here. Just tried in 2020-03: it still worked, so it's a regression indeed, but caused by what change? Second: moving forward, fake-EOF should generally fade out. Could you make a quick attempt at forcing fetchNextToken() to return 'token' not EOF? Would that produce a useful AST? Proposals?
(In reply to Gayan Perera from comment #12) > At the end of the if conditions "." we return EOF from fetchNextToken, but > the parser doesn't stop parsing even though the logic seems to be to stop > parsing anymore. The reason is the next actFromTokenOrSynthetic actually > returns a act for the SimpleName to be captured the rest as a > PostfixExpression and fail. I believe the expectation is: when EOF is sent, *eventually* the automaton will detect an error and stop. The automaton should still reduce as much as it can to create remaining AST. However, it should *not* consume more tokens. The latter assumption should be ensured by assigning this.scanner.eofPosition, no?
Futher looking at the problem is in hasPendingExpression. When we are at "subList(1,1)." the expression stack only contains the two int literals. So the expression that picked up is the second literal, but there are not values in elementObjectInfoStack which cause the method to return that there are pending expressions which cause this issue.
When we don't have parameters in the second chained method such as "if(ints.iterator().)" the expressionPtr is -1, so basically we stop at that point without further processing since we set the eof in the scanner. But when we have "if(ints.subList(1,1).)", the arguments are in the expression stack and the expressionPtr points the the last argument. This cause the scanner/parser to further proceed processing. But at the second DOT which is "System." we find that top of the expression stack which contains ints.subList(1,1) becomes equal to what inside the the block delimiter which cause the parser to end at middle of parsing a statement. I think we need to find a way to improve hasPendingExpression to support method chains which contains arguments as well.
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/183239
@Stephan i tried to provide a fix to enhance the hasPendingExpression before we start parsing the statements and find the next DOT token. With the fix it seems, it also expands some completions in CompletionTest3 which were added before for if related issues. For me it seems like the current completions nodes are right. But let me know. Also let me know if you think the new tests is better to be added into CompletionTest3.
@Gayan, which test did you use to elaborate your strategy? Interestingly, I can reduce the entire method hasPendingExpression() to return this.expressionPtr > -1; and don't see any failures in RunCompletionModelTests & RunCompletionParserTests :) This could actually be a good sign: we need to send fake EOF fewer times than previously, which makes completion more robust also in situations involving lambdas. I'll check which recent change might have obsoleted the complexity in this vicinity.
(In reply to Stephan Herrmann from comment #19) > I'll check which recent change might have obsoleted the complexity in this > vicinity. Experiment: * copy CompletionTests & CompletionTests9 to new files (appending TMP to the class name) as to make them independent of git movements. These files contain all tests up-to and including the current gerrit. * checkout some commits between 2021-05 and now * make the simplification from comment 19 * run the TMP tests. By checking out various commits relevant for code assist, I found that it's the fix for bug 574215 that obsoletes the complexity in hasPendingExpression(). This makes a lot of sense when you look at this comment added in bug 574215 // additionally check if 'prefix.' should be interpreted as a variable receiver rather then part of a type reference: That's actually what hasPendingExpression() tried to avoid. Now that we can handle that case downstream, the parser can be simplified again.
Patch set #4 contains my take at it. Thanks, Gayan, for pointing to hasPendingExpression(), which was right on. I suspect you didn't test your logic in both directions: * does it hurt? * is it needed? For a moment I was tempted to even remove the remaining "this.expressionPtr <= -1". This removal changed the outcome in 14 tests, some of which even looked like improving, but then it ran too deep to be contained in this fix. I remember two patterns of changes: * in one test "fo|" would propose keyword "for", which is not proposed currently * in several tests, a METHOD_REF proposal would no longer include the trailing "()" So this last step is not included in gerrit. @Gayan, do you want to have a look at these (perhaps in a new bug)?
(In reply to Stephan Herrmann from comment #21) > Patch set #4 contains my take at it. > > Thanks, Gayan, for pointing to hasPendingExpression(), which was right on. > > I suspect you didn't test your logic in both directions: > * does it hurt? > * is it needed? > > For a moment I was tempted to even remove the remaining "this.expressionPtr > <= -1". This removal changed the outcome in 14 tests, some of which even > looked like improving, but then it ran too deep to be contained in this fix. > I remember two patterns of changes: > * in one test "fo|" would propose keyword "for", which is not proposed > currently > * in several tests, a METHOD_REF proposal would no longer include the > trailing "()" > > So this last step is not included in gerrit. @Gayan, do you want to have a > look at these (perhaps in a new bug)? @Stephan, the first attempt i did was mainly focusing on the current issue. which led me with what you observed plus some issues with expected CU which missed the if statements. The looking further i found that existing too early caused that, so i though it would be better to complete the if block parsing without exiting in the middle which caused this issue. I also tested against the orginal fix Bug573632 which showed that there is no impact. But i see now that handling the control statement blocks was not the intention of the method. As always i learn new things from you @Stephan in this parser area.
(In reply to Stephan Herrmann from comment #21) > For a moment I was tempted to even remove the remaining "this.expressionPtr > <= -1". This removal changed the outcome in 14 tests, some of which even > looked like improving, but then it ran too deep to be contained in this fix. > I remember two patterns of changes: > * in one test "fo|" would propose keyword "for", which is not proposed > currently > * in several tests, a METHOD_REF proposal would no longer include the > trailing "()" > > So this last step is not included in gerrit. @Gayan, do you want to have a > look at these (perhaps in a new bug)? Yes lets handle them in a new bug.
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/183239 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=08fc1337f1f9aaff0490648702821e7090ae70cb
(In reply to Eclipse Genie from comment #24) > Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/183239 was > merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=08fc1337f1f9aaff0490648702821e7090ae70cb Released for 4.21 M2
(In reply to Gayan Perera from comment #23) > (In reply to Stephan Herrmann from comment #21) > > For a moment I was tempted to even remove the remaining "this.expressionPtr > > <= -1". This removal changed the outcome in 14 tests, some of which even > > looked like improving, but then it ran too deep to be contained in this fix. > > I remember two patterns of changes: > > * in one test "fo|" would propose keyword "for", which is not proposed > > currently > > * in several tests, a METHOD_REF proposal would no longer include the > > trailing "()" > > > > So this last step is not included in gerrit. @Gayan, do you want to have a > > look at these (perhaps in a new bug)? > > Yes lets handle them in a new bug. -> bug 574978 and bug 574979 (which could be the same issue after all).
*** Bug 574902 has been marked as a duplicate of this bug. ***
Verified with I20210723-1800. This also fixes bug 574902.
