Community
Participate
Working Groups
In the following example no static methods on Thread are proposed: public class ContentAssist { protected void test() { if (true) { Thread.| someMethod(); } } void someMethod() { } } Tested in Version: 2021-09 (4.21) Build id: I20210811-1800 works in 4.19
The workaround is to end the Thread. with a semicolon. But i will check if this is a quick fix which will not have change alot so it can be included in M3.
This caused by the change we did for Bug 574823, the fetchNextToken scans beyond the cursor position because we have removed the hasPendingExpression check which matched with the if conditional expression which was pending. I will see if i can make it work again, while keeping the the Bug 574823 intact.
(In reply to Gayan Perera from comment #2) > This caused by the change we did for Bug 574823, the fetchNextToken scans > beyond the cursor position because we have removed the hasPendingExpression > check which matched with the if conditional expression which was pending. > > I will see if i can make it work again, while keeping the the Bug 574823 > intact. Thanks. My guess/hope is we can leave parsing as it is and only need to find the location where CompletionEngine needs to be smarter in calling findMethods with the right input. Can we? Does it come out with <CompletionOnName:Thread>? Can this be resolved? It would be good to have a fix in M3, so if you get stuck I could take a look on Sunday, just assign it to me if you want me to.
(In reply to Stephan Herrmann from comment #3) > (In reply to Gayan Perera from comment #2) > > This caused by the change we did for Bug 574823, the fetchNextToken scans > > beyond the cursor position because we have removed the hasPendingExpression > > check which matched with the if conditional expression which was pending. > > > > I will see if i can make it work again, while keeping the the Bug 574823 > > intact. > > Thanks. > > My guess/hope is we can leave parsing as it is and only need to find the > location where CompletionEngine needs to be smarter in calling findMethods > with the right input. Can we? > > Does it come out with <CompletionOnName:Thread>? Can this be resolved? No it comes as CompletionOnQualifiedTypeReference, the method name is consumed as a variable definition. > > It would be good to have a fix in M3, so if you get stuck I could take a > look on Sunday, just assign it to me if you want me to. Reading the old issues, i feel it not a good idea to play with the fetchNextToken, but rather end the parsing by setting the scanner eofPosition in consumeToken after we have consumed the empty identifier token. I will push my patch after testing more with a Parser unit test tomorrow. if it needs a change or improvement you can change it Stephan.
(In reply to Gayan Perera from comment #4) > (In reply to Stephan Herrmann from comment #3) > > (In reply to Gayan Perera from comment #2) > > > This caused by the change we did for Bug 574823, the fetchNextToken scans > > > beyond the cursor position because we have removed the hasPendingExpression > > > check which matched with the if conditional expression which was pending. > > > > > > I will see if i can make it work again, while keeping the the Bug 574823 > > > intact. > > > > Thanks. > > > > My guess/hope is we can leave parsing as it is and only need to find the > > location where CompletionEngine needs to be smarter in calling findMethods > > with the right input. Can we? > > > > Does it come out with <CompletionOnName:Thread>? Can this be resolved? > No it comes as CompletionOnQualifiedTypeReference, the method name is > consumed as a variable definition. I see, I forgot the empty token which causes the interpretation as type, not name. So in CompletionEngine.completionOnQualifiedTypeReference() you'll already find recent changes, look for // additionally check if 'prefix.' should be interpreted as a variable receiver rather then part of a type reference: I'd prefer another change right their, rather then going back to faking eof, because the latter will look us out from building more context, which would be needed, e.g., if the code is nested in a lambda. Extracting the tokens from a QualifiedTypeReference and performing different kinds of lookups sounds fairly straight-forward to me.
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184001
Yes @Stephan, i think the approach i took fails few other scenarios and I’m not sure if i have any other conditions to narrow this down only for this case. So i will look into your approach as well. I think this strategy might be good to go into wiki as well.
(In reply to Gayan Perera from comment #7) > Yes @Stephan, i think the approach i took fails few other scenarios and I’m > not sure if i have any other conditions to narrow this down only for this > case. So i will look into your approach as well. > > I think this strategy might be good to go into wiki as well. I looked at the approach you suggested, But i have this following challenge which question me whether its the right approach For example in this situation we get : assistNode -> <CompleteOnType:Thread.> (CompletionOnQualifiedTypeReference) assistNodeParent -> <CompleteOnType:Thread.> someMethod; (LocalDeclaration) But the new code you mention will fail since the resolved the prefix binding are not valid as Variable. But we could try to do a resolution to see if its a valid Type. But then based on that if we try to resolve fields, member types and methods, then a actual variable declaration like Thread.| state; might also suggest all the static methods and fields instead of only member types. Do you see a better way to handle this still from this new logic ?
@Stephan i did some further changes in the CompletionParser to populate the assistNode as CompleteOnName and scan the rest of the code again after "Thread.". But it leads to some more problem i have to deal with such as Most of the time surrounding statements are not part of the final AST. So then i did some more changes to get the if block and related statements into the final AST. But those changes seems fail on some existing tests. So if you are going to work on this tomorrow please let me know and continue. Otherwise i will look at the code tomorrow.
Gayan, I'm starting to look into this right now. (In reply to Gayan Perera from comment #8) > But the new code you mention will fail since the resolved the prefix binding > are not valid as Variable. > > But we could try to do a resolution to see if its a valid Type. But then > based on that if we try to resolve fields, member types and methods, then a > actual variable declaration like > > Thread.| state; > > might also suggest all the static methods and fields instead of only member > types. Are you saying, in that situation we should *only* propose completions that make this a legal LocalDeclaration?
(In reply to Stephan Herrmann from comment #10) > Gayan, I'm starting to look into this right now. > > (In reply to Gayan Perera from comment #8) > > But the new code you mention will fail since the resolved the prefix binding > > are not valid as Variable. > > > > But we could try to do a resolution to see if its a valid Type. But then > > based on that if we try to resolve fields, member types and methods, then a > > actual variable declaration like > > > > Thread.| state; > > > > might also suggest all the static methods and fields instead of only member > > types. > > Are you saying, in that situation we should *only* propose completions that > make this a legal LocalDeclaration? Yes @Staphan, in a situation like Thread.| state; where it's pretty clear that its a variable declaration, we should only provide Type Completions right ? other wise one could think that the completion engine is not smart enough to distinguish it.
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184022
(In reply to Eclipse Genie from comment #12) > New Gerrit change created: > https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184022 Never mind for now (silly me forgot to delete the pseudo char '|', which incidentally makes my test pass ;p ).
(In reply to Gayan Perera from comment #11) > Yes @Staphan, in a situation like > > Thread.| state; > > where it's pretty clear that its a variable declaration, we should only > provide Type Completions right ? other wise one could think that the > completion engine is not smart enough to distinguish it. I see, by parsing this as <CompleteOnType:Thread.> someMethod; we forget that "someMethod" originally was "someMethod()", so completion indeed is not smart enough ... That indeed looks like a parser problem. I'll check if this can be fixed by avoiding fake eof in this situation ...
(In reply to Stephan Herrmann from comment #14) > [...] I'll check if this can be fixed by avoiding fake eof in this situation ... No, eof isn't set during parsing, but after parsing from: CompletionScanner(Scanner).resetTo(int, int, boolean, Scanner$ScanContext) line: 3280 CompletionScanner(Scanner).resetTo(int, int, boolean) line: 3259 CompletionScanner(Scanner).resetTo(int, int) line: 3256 CompletionParser.isInsideBody(Statement, AbstractMethodDeclaration) line: 6203 CompletionParser.endParse(int) line: 6161
What i tried was when consumingEnterVariable i checked if we have a proceeding Name.| then i push the Name. into CompletionOnQualifiedNameReference and start the scanner after that again. It actually solves the problem. But i think i have to improve the entering criteria for this logic so that it will not cause issues on other type of name completions. Mainly what i was missing was how to capture if we are on pending completion on a name reference like Name.| or Name.foo| May be this might give you some input for this.
The recovered AST looks weird indeed, so why do we get the LocalDeclaration with a CompletionOnQualifiedTypeReference in the first place? It's the empty completion identifier which turns the otherwise legal input into Thread.<emptyId> someMethod(); When the parser sees the prefix "Thread.<emptyId> someMethod" it can unambiguously conclude that this is a variable declaration, hence "Thread.<emptyId>" must be a type. From a "stupid" parser perspective this isn't all so weird. What's more, late in the release cycle (near the close of M3) I'm afraid of tricky changes in the parser. Just remember that it was a parser change, the started the recent flood of regressions :) Coward that I am, I opted for a minimal change in the parser: * just store one bit of information for later use: what is the next token we see after the assumed LocalDeclaration. Then we can do all the necessary differentiation in CompletionEngine: recognize the specific situation of this bug by testing the next token against "(". If the exact situation has been detected simply propose more things that can be referenced via the receiverType (Thread). See https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184022 (#2) Is this crude? BTW, the change in checkAndCreateModuleQualifiedAssistTypeReference() serves the purpose of doing everything *not* relating to modules directly in createQualifiedAssistTypeReference().
(In reply to Stephan Herrmann from comment #17) > What's more, late in the release cycle (near the close of M3) I'm afraid of > tricky changes in the parser. Just remember that it was a parser change, the > started the recent flood of regressions :) Yes i think its a good point. May be we should try to improve the parser in next release. > > Coward that I am, I opted for a minimal change in the parser: I think its not Coward, but rather been wise :) > * just store one bit of information for later use: > what is the next token we see after the assumed LocalDeclaration. > > Then we can do all the necessary differentiation in CompletionEngine: > recognize the specific situation of this bug by testing the next token > against "(". If the exact situation has been detected simply propose more > things that can be referenced via the receiverType (Thread). > > See https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184022 (#2) > > Is this crude? Hmmm this will only fix this particular scenario right ? How about Thread.| var1 = 10; or var++ etc ? I pushed a new change in my gerrit, i was more into fixing the parser, but i need feedback on the change i did. But may be the current patch of your can be sent to M3 and we can look into fixing the parser if my parser fix can be improved more into next release. Then we can remove the workaround we did for M3 in next release. WDYT ?
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184022 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=41f332495b3b0cf18006cfc73b8ee34a2a5a27ef
Gayan, you're right about lack of generality in my patch. For that reason I filed bug 575422. For a second I was tempted to try to finalize that approach right now, but then the "coward" came back :) So here: (In reply to Eclipse Genie from comment #19) > Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184022 was > merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=41f332495b3b0cf18006cfc73b8ee34a2a5a27ef I released my partial fix that lets CompletionEngine do the heavy lifting. => released for 4.21 M3 If we quickly identify a few more tokens that should be treated like '(' that might be fine to add for 4.21. Going forward I scheduled bug 575422 for further experiments early in 4.22. See that it already contains some more hints.
(In reply to Stephan Herrmann from comment #20) > Gayan, you're right about lack of generality in my patch. For that reason I > filed bug 575422. For a second I was tempted to try to finalize that > approach right now, but then the "coward" came back :) Yes lets fix this properly in that bug. May be we can remove the following CompletionEngine change with that to keep it clean. > So here: > > (In reply to Eclipse Genie from comment #19) > > Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184022 was > > merged to [master]. > > Commit: > > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > > ?id=41f332495b3b0cf18006cfc73b8ee34a2a5a27ef > > I released my partial fix that lets CompletionEngine do the heavy lifting. > > => released for 4.21 M3 > > If we quickly identify a few more tokens that should be treated like '(' > that might be fine to add for 4.21. > I tested following snippets protected void test() { if (true) { int i; Thread. i++; } } protected void test() { if (true) { Thread. someMethod<String>(); } } <T> void someMethod(T t) { } protected void test() { if (true) { int i; Thread. i = 10; // all type of assignments fails } } But i think we can focus on these with the new bug since we have a workaround as well for this as simple as adding ; and then complete on Thread.
Verified with Eclipse 4.21 M3 Version: 2021-09 (4.21) Build id: I20210818-1800 [For the original case - if new issues found as in previous comment 21 , please file new bugs]
For posterity: this fix was superseded by part of the fix in bug 575631 (4.22 M2).
