Bug 469324 - [Content assist] auto-completion adds wrong type arguments
Summary: [Content assist] auto-completion adds wrong type arguments
Status: ASSIGNED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 4.5   Edit
Hardware: PC Linux
: P3 normal with 4 votes (vote)
Target Milestone: ---   Edit
Assignee: Julian Honnen CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords: helpwanted
: 433122 507813 542455 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-06-03 15:27 EDT by Aurelien Manteaux CLA
Modified: 2023-01-31 13:38 EST (History)
11 users (show)

See Also:


Attachments
more token locations in CompletionContext (2.10 KB, patch)
2020-06-18 03:19 EDT, Julian Honnen CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aurelien Manteaux CLA 2015-06-03 15:27:47 EDT
The bug only append when a Type (class) is not imported. Once the statement "import package.Type;" appear, it works almost as expected.

Here is some examples that produce incorrect auto-completed code :
- Stream.cl| => Stream<T>.class (the pipe means clrl+space)
- ImmutableList.of| => ImmutableList<E>.of()
- Strea| => Stream<T> : the only case where it could be good would be in the variable declaration process ; however since Eclipse cannot know the type parameter, it will write Stream<T>, so one has to remove T and replace it with the right type. This case also append when the Type is imported.

Here is the only example that produce right auto-completed code :
- new ArrayL| => new ArrayList<>()

Type arguments should only be appended when the new keyword precedes the type :
- new Someth| => append arguments : new Something<>()
- Someth| => don't append arguments : Something
- Something.me| => still don't append arguments : Something.method()
Comment 1 Noopur Gupta CLA 2015-06-04 02:21:29 EDT
See other duplicates of bug 301990 also which have similar use cases.

*** This bug has been marked as a duplicate of bug 301990 ***
Comment 2 Aurelien Manteaux CLA 2015-06-04 03:49:09 EDT
(In reply to Noopur Gupta from comment #1)
> See other duplicates of bug 301990 also which have similar use cases.
> 
> *** This bug has been marked as a duplicate of bug 301990 ***

It looks like you have made a mistake, the bug 301990 is marked resolved since Eclipse 4.3 M7, however, the bug I reported is still present in Eclipse 4.5.0RC2.
Comment 3 Dani Megert CLA 2015-06-04 05:16:56 EDT
Noopur, please take another look.
Comment 4 Noopur Gupta CLA 2015-06-05 06:59:13 EDT
As mentioned in bug 283950 comment #9, we can alleviate this problem a bit by the fix that is released via bug 301990.

(In reply to Aurelien Manteaux from comment #0)
> The bug only append when a Type (class) is not imported. Once the statement
> "import package.Type;" appear, it works almost as expected.
> 
> Here is some examples that produce incorrect auto-completed code :
> - Stream.cl| => Stream<T>.class (the pipe means clrl+space)
> - ImmutableList.of| => ImmutableList<E>.of()
> - Strea| => Stream<T> : the only case where it could be good would be in the
> variable declaration process ; however since Eclipse cannot know the type
> parameter, it will write Stream<T>, so one has to remove T and replace it
> with the right type. This case also append when the Type is imported.

In your examples, you can invoke content assist on the type name first and remove the type arguments (via fix for bug 301990) before appending .class or a static method/field.

Let me know if I am missing something else from your bug report in terms of being different from bug 301990 and its duplicates.
Comment 5 Aurelien Manteaux CLA 2015-06-05 08:33:46 EDT
(In reply to Noopur Gupta from comment #4)
> As mentioned in bug 283950 comment #9, we can alleviate this problem a bit
> by the fix that is released via bug 301990.
> 
> (In reply to Aurelien Manteaux from comment #0)
> > The bug only append when a Type (class) is not imported. Once the statement
> > "import package.Type;" appear, it works almost as expected.
> > 
> > Here is some examples that produce incorrect auto-completed code :
> > - Stream.cl| => Stream<T>.class (the pipe means clrl+space)
> > - ImmutableList.of| => ImmutableList<E>.of()
> > - Strea| => Stream<T> : the only case where it could be good would be in the
> > variable declaration process ; however since Eclipse cannot know the type
> > parameter, it will write Stream<T>, so one has to remove T and replace it
> > with the right type. This case also append when the Type is imported.
> 
> In your examples, you can invoke content assist on the type name first and
> remove the type arguments (via fix for bug 301990) before appending .class
> or a static method/field.
> 
> Let me know if I am missing something else from your bug report in terms of
> being different from bug 301990 and its duplicates.

Thank you Noopur for your response.

Is the patch for bug 301990 already released ?
On Eclipse 4.5.0RC2 and 4.4.2, when
1/ I type Map|,
2/ I will get Map<K|, V>, with the cursor on K.
3/ If I press on backspace, K is deleted : Map<|, V>,
4/ If I press again on backspace, < is deleted : Map|, V>
From what I understood in bug 301990, at step 3, I should have had only : Map|
Did I miss something ?

However, even if the fix would our life easier, it would not be ideal :
1/ We would have to remove the generics parameters (with only one backspace, but still one backspace)
2/ As you said, we would have to always use content assist with the type before referencing a method or a field.

Don't get me wrong, I use a lot and I like Eclipse, but in Intellij this feature seems to be implemented in an easier way which is working very fine : the generic parameters are only appended when the type is prepended by "new ". Would it possible to implement something like that in Eclipse ? Is there any concern with this feature ?
Comment 6 Noopur Gupta CLA 2015-06-05 10:48:30 EDT
(In reply to Aurelien Manteaux from comment #5)
> Is the patch for bug 301990 already released ?
> On Eclipse 4.5.0RC2 and 4.4.2, when
> 1/ I type Map|,
> 2/ I will get Map<K|, V>, with the cursor on K.
> 3/ If I press on backspace, K is deleted : Map<|, V>,
> 4/ If I press again on backspace, < is deleted : Map|, V>
> From what I understood in bug 301990, at step 3, I should have had only :
> Map|
> Did I miss something ?

See bug 301990 comment #7 for the behavior. The first backspace will delete the first type arg and the second backspace will delete the entire text up to '>'. So after step 4, u should be left with just 'Map'. I checked with 4.5 RC3 build and it works as expected.

> However, even if the fix would our life easier, it would not be ideal :
> 1/ We would have to remove the generics parameters (with only one backspace,
> but still one backspace)
> 2/ As you said, we would have to always use content assist with the type
> before referencing a method or a field.
> 
> Don't get me wrong, I use a lot and I like Eclipse, but in Intellij this
> feature seems to be implemented in an easier way which is working very fine
> : the generic parameters are only appended when the type is prepended by
> "new ". Would it possible to implement something like that in Eclipse ? Is
> there any concern with this feature ?

I cannot comment on this right now. Will have to look deeper. Based on bug 283950 comment #9, it seems complicated to decide where we need type args.

"Type arguments should only be appended when the new keyword precedes the type": This should be checked to see that it doesn't create more issues with raw types than adding unnecessary type args right now.
Comment 7 Jeremy Buege CLA 2016-02-22 22:17:23 EST
(In reply to Noopur Gupta from comment #6)
> See bug 301990 comment #7 for the behavior. The first backspace will delete
> the first type arg and the second backspace will delete the entire text up
> to '>'. So after step 4, u should be left with just 'Map'. I checked with
> 4.5 RC3 build and it works as expected.

Unfortunately, this behavior actually fails to apply for the use case originally described in bug 283950 (as Aurelien tried to point out). The backspace behavior works as described in the context of starting a new line of code, but not when invoking content assist on the right side of an assignment operator. Try the following steps in an attempt to invoke Guava's ImmutableMap.of("Key","Value"):

1. Type: Map<String, String> map = ImmutableM|
2. Invoke content assist with CTRL+Space, Enter
3. Note that this returns "ImmutableMap<String, String>|", with cursor trailing after the inserted text instead of selecting the first type argument for editing.

The mitigation behavior from bug 301990 is not applicable because Eclipse already believes it has identified the type arguments (that still don't qualify as valid syntax in this context). This situation currently requires 16 backspaces, not 2. The fix introduced in bug 301990 helped mitigate duplicates 336043 and 356282, but did not address 283950, which was improperly closed.

> "Type arguments should only be appended when the new keyword precedes the
> type": This should be checked to see that it doesn't create more issues with
> raw types than adding unnecessary type args right now.

Correct, and I actually strongly disagree with Aurelien's claim here. Type arguments should still be supplied for:
1. Start-of-line contexts for field and local method variable declarations: List<String> accounts = null;
2. Parameters defined in method/constructor declarations: private void validate(List<String> accounts) {

Retaining the behavior for #1 still breaks static void method invocations on generic classes, but this case is rare and already mitigated satisfactorily.
As for "new List<String>()", I'd argue even that should rarely require the full type definition due to Java 7 inference.

My suggestion: Remove the invalid type completion in the following three scenarios:
1. Immediately following an assignment operator
2. Parameter value in a method invocation
3. Parameter value in a constructor invocation

This would eliminate 99% of my pain with this issue.
Comment 8 Noopur Gupta CLA 2016-11-21 03:26:24 EST
*** Bug 507813 has been marked as a duplicate of this bug. ***
Comment 9 Aurelien Manteaux CLA 2016-12-14 04:07:10 EST
Jeremy sums up pretty good the problem!

Noopur, do you think it would be possible to implement his idea?
Comment 10 Corneliud Dirmeier CLA 2017-04-10 06:25:36 EDT
I noticed the same problem when filling a method argument.

Example

...stream().map(Function|) => stream().map(Function<T, R>)

Not what I excpected when I want to write Function.identity()

Because it was in a method argument I tried to disable the option "Fill method arguments and show guessed arguments" in the Content Assist Preferences.

It cames out that this option does disable both described behavior (generic completion of method arguments as well as generic completion in assignments).

Not the best way to give up the armument completion but at the moment it is a workaround.
Comment 11 Noopur Gupta CLA 2018-12-06 07:48:41 EST
*** Bug 542455 has been marked as a duplicate of this bug. ***
Comment 12 Julian Honnen CLA 2020-05-22 09:11:31 EDT
Parts of this bug related to not imported types will be addressed by bug 563478:
(In reply to Aurelien Manteaux from comment #0)
> The bug only append when a Type (class) is not imported. Once the statement
> "import package.Type;" appear, it works almost as expected.
> 
> Here is some examples that produce incorrect auto-completed code :
> - Stream.cl| => Stream<T>.class (the pipe means clrl+space)
> - ImmutableList.of| => ImmutableList<E>.of()

I'm leaving this bug open to address other places where type arguments are always invalid like in variable assignments

(In reply to Jeremy Buege from comment #7)
> 1. Type: Map<String, String> map = ImmutableM|
> 2. Invoke content assist with CTRL+Space, Enter
> 3. Note that this returns "ImmutableMap<String, String>|", with cursor
> trailing after the inserted text instead of selecting the first type
> argument for editing.
or parameter values
(In reply to Corneliud Dirmeier from comment #10)
> ...stream().map(Function|) => stream().map(Function<T, R>)

Note that there is an existing workaround for both examples: Insert the proposal with '.' instead of enter.
Comment 13 Julian Honnen CLA 2020-05-22 09:14:03 EDT
*** Bug 433122 has been marked as a duplicate of this bug. ***
Comment 14 Julian Honnen CLA 2020-06-18 03:19:48 EDT
Created attachment 283328 [details]
more token locations in CompletionContext

I don't this can be solved without additional information from JDT core. Attached is a WIP patch to add some more token location based on the parent ast node as fallback where it was previously reported as unknown.

UI can than distinguish where a generic type is allowed.

Is that an option or are there better ones?
Comment 15 Eclipse Genie CLA 2023-01-08 13:00:55 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 16 Roland Illig CLA 2023-01-31 13:38:08 EST
Still relevant.