Bug 417961 - [organize imports] Import proposals for variable declaration type should consider expression type
Summary: [organize imports] Import proposals for variable declaration type should cons...
Status: RESOLVED INVALID
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.4   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: JDT-UI-Inbox CLA
QA Contact:
URL:
Whiteboard: fix candidate
Keywords: usability
Depends on: 419618 419619
Blocks:
  Show dependency tree
 
Reported: 2013-09-24 15:52 EDT by Robin Stocker CLA
Modified: 2013-10-17 03:56 EDT (History)
11 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Stocker CLA 2013-09-24 15:52:00 EDT
Given this code, where ArrayList is imported and List is not yet imported:

  List<String> l = new ArrayList<String>();

Invoking quick fix on List shows the following proposals (in that order):

* Import 'List' (java.awt)
* Import 'List' (java.util)

There are two issues with the first entry:

1. java.awt.List is not generic, therefore invalid
2. java.awt.List is not a super-type of java.util.ArrayList, therefore invalid

So the only choice that does not produce an error is java.util.List.

Ideally, when having the "Organize Imports" save action configured, it should be imported automatically, not requiring further user input.

In case the quick fix is invoked, invalid choices should either be not listed or be listed after the valid choices.

See also ide-dev mailing list thread:

https://dev.eclipse.org/mhonarc/lists/ide-dev/msg00072.html
Comment 1 Noopur Gupta CLA 2013-09-25 06:02:33 EDT
*** Bug 395500 has been marked as a duplicate of this bug. ***
Comment 2 Marcel Bruch CLA 2013-10-13 03:28:42 EDT
a few ideas how to make this more flexible:


The proposal sorter is hard-coded in JavaCorrectionProcessor. Since it works on IJavaCompletionProposals it *may* reuse the configured sorter for content assist. This does not fix the issue but opens that mechanism for new extensions. But it would probably be better to have a new base class for that.

With a new base class, the sorter should get access to the AssistContext to compute the facts it needs to sort proposals properly.

Third (here the opinions way diverge), the sorter may get access to the complete list of proposals to filter the unwanted / unlikely proposals. Even if unmodifiable, a list of available assists would be helpful.

AFAICT, all changes are local to org.eclipse.jdt.internal.ui.text.correction.JavaCorrectionProcessor.computeQuickAssistProposals(IQuickAssistInvocationContext)

To me this change makes sense. Would JDT accept this solution as contribution for 4.4 and 4.3.2?
Comment 3 Lars Vogel CLA 2013-10-14 03:53:24 EDT
Marcel, why not create a Gerrit change request? I think with code it's easier for JDT to comment on the proposal.
Comment 4 Marcel Bruch CLA 2013-10-14 03:56:45 EDT
Because (i) it needs some time to implement it, (ii) there are different styles how to iplement it, (iii) some features are JDT might reject/dislike, and (iv)  JDT people (like Dani I guess) know quickly if this is the way they would like it to be and accept.
Comment 5 Lars Vogel CLA 2013-10-14 04:26:01 EDT
I have good experience with patches via Gerrit. The team can comment on styling issues.
Comment 6 Dani Megert CLA 2013-10-15 07:52:12 EDT
Comment 0 describes two different issues:
- Organize Imports doesn't detect and filter unappropriated types (generic vs. 
  non-generic and assignment mismatches)
- Quick Fix does not rate perfect matches higher
and there is a third case which is interesting: content assist - it has the same sorting issue as the Quick Fix. If we improve/fix content assist in JDT Core to set better relevances, it will also fix the Quick Fix, because those proposals are also computed via content assist.

No new extensions or APIs are needed to achieve that.

I suggest to make two bugs out of this one.
Comment 7 Robin Stocker CLA 2013-10-16 14:03:47 EDT
(In reply to Dani Megert from comment #6)
> I suggest to make two bugs out of this one.

Done, I created two bugs and made them block this one (but this could also be closed).

> Comment 0 describes two different issues:
> - Organize Imports doesn't detect and filter unappropriated types (generic
> vs. 
>   non-generic and assignment mismatches)

See bug 419618.

> - Quick Fix does not rate perfect matches higher
> and there is a third case which is interesting: content assist - it has the
> same sorting issue as the Quick Fix. If we improve/fix content assist in JDT
> Core to set better relevances, it will also fix the Quick Fix, because those
> proposals are also computed via content assist.

See bug 419619.

> No new extensions or APIs are needed to achieve that.

Good to hear!
Comment 8 Marcel Bruch CLA 2013-10-17 03:15:26 EDT
(In reply to Dani Megert from comment #6)
> If we improve/fix content assist in JDT
> Core to set better relevances, it will also fix the Quick Fix, because those
> proposals are also computed via content assist.

It looks like there is no code yet that provides access to RHS of the assignment in CompletionEngine. The referenceContext does not provide that information (only field declarations but not it's initializations). Any idea how this should be solved?

Should the type resolution consider the import statements (which are available) and do a "is-proposed-type-a-super-type-of-any-imported-type" thingy?
Comment 9 Dani Megert CLA 2013-10-17 03:55:45 EDT
(In reply to Marcel Bruch from comment #8)
> (In reply to Dani Megert from comment #6)
> It looks like there is no code yet that provides access to RHS of the
> assignment in CompletionEngine. The referenceContext does not provide that
> information (only field declarations but not it's initializations). Any idea
> how this should be solved?
> 
> Should the type resolution consider the import statements (which are
> available) and do a "is-proposed-type-a-super-type-of-any-imported-type"
> thingy?

I've copied this over to bug 419619.
Comment 10 Dani Megert CLA 2013-10-17 03:56:43 EDT
Closed as INVALID as we have two independent bugs to track the issues.