Bug 362628

Comment 1 Brian Svihovec 2011-11-02 16:06:46 EDT

I ran a test in Java to see what happens if I define a class named String and then tried to declare a field of type String in another package within the same project.

JP1
  - packA 
      String.java
  - packB
      TestClass.java
          private String field1;

JDT does not provide a dialog when organize imports is invoked in TestClass, and String automatically defaults to the one in the Java Runtime, without adding any import declarations.  

If we use JDT as our guide, then we should not be prompting the user for which part to use, and was should also not be adding an import declaration for FormField in the MOF model.

Comment 2 Xiao Bin Chen 2011-11-02 21:23:18 EDT

Hi Rocky,
Organize import will import those type which have error(Because those type are not import into current egl part). But system part can be reached by every customer part, so organize import will ignore system part. And If we check every type in customer part to check if each part is customer's want, it may cause some performance issue(We need to search it every time for every imported type). So I think this defect won't be fixed. It works as design.

Xiaobin

Comment 3 fahua jin 2011-11-02 21:35:09 EDT

Agree, close the defect.

Comment 4 Brian Svihovec 2011-11-02 22:08:46 EDT

In comment 2 it states that organize imports will ignore system parts, but in comment one it states that organize imports will add imports for system parts.  

Is content assist adding imports for the FormField in the system package, or is it not adding any imports in this situation?

Comment 5 Xiao Bin Chen 2011-11-03 08:13:58 EDT

(In reply to comment #4)
> In comment 2 it states that organize imports will ignore system parts, but in
> comment one it states that organize imports will add imports for system parts.  
> 
> Is content assist adding imports for the FormField in the system package, or is
> it not adding any imports in this situation?

I think I have mislead you, for "ignore" system parts.
    The "ignore" means, if you used a part, which defined in customer's projects. But there is a system part with the same name. And you did not use "import client.xxx.partname" in the head of the main handler. In this case, the compiler will regard the part as the system's part, and so the TypeBinding will be valid, so it will ignore it.

For the content assist side. It's seems like we did not put the formfield into system package, So content assist does not know anything about the org.eclipse.edt.mof.egl.FormField; I have retrieved the org.eclipse.edt.compiler/eglsource package, and I did not see the formfield there. 

PAUL, Does EDT put all of system part into edtCompiler.eglar?

And I remember we have a workitem to decide which system part could not be imported, will we support this in 0.7?

Tony, added you to this discussion, seems like you owned which system part could not be imported.

Comment 6 Brian Svihovec 2011-11-03 09:00:44 EDT

I am reopening this defect since it is still under discussion.  

In my opinion, we should not be adding imports for 'system parts' when organize imports is invoked.  In the case where a user defined part collides with a 'system part' (i.e. FormField), organize imports should not prompt the user, and should not add any imports.  This means that the user will need to either add their own import statement or fully qualify the part name.

I believe I have seen organize imports add import statements for system parts in various situations, including FormField, so I would say that this defect should be used to track the general solution of not adding imports for system parts.  It is possible that we do not have the necessary information in the model for this in .7, in which case we can defer this until 1.0.  We should not hard code anything in the tools regarding the definition of a system part, and should let the definition of the compiler/generator indicate what namespaces are considered system parts.

Paul, should we consider renaming FormField in the core model to avoid this collision with the MVC framework?  I realize that this could happen with any other name as well, but maybe there is a better name for the core model part in this particular case.

Comment 7 Paul Harmon 2011-11-03 10:26:06 EDT

I have added code to SystemEnvironment so that the mof model parts (part in the the package org.eclipse.edt.mof.*) are no longer added to the map of parts that can be used without package qualification. This will eliminate the problem of organize imports adding the import for the mof FormField.

This should suffice for 0.7. In the future we can determine the exact list of system packages that are implicitly imported.

I am leaving this open, in case we want to make the change that Brian suggests (do not add import for system parts).

Comment 8 Xiao Bin Chen 2011-11-07 01:33:05 EST

Change this "defect" to enhancement and change the target milestone to future.  To track the enhancement of  the exact list of system packages that are implicitly imported.

Comment 9 Xiao Bin Chen 2012-02-20 03:39:10 EST

Change the title to describe the problem clearly.

Comment 10 Xiao Bin Chen 2012-02-22 04:39:22 EST

Created attachment 211385 [details]
Patch for this enhancement

Two files was change for this enhancement.
OrganizeImportsOperation.java
OrganizeImportsVisitor.java


In OrganizeImportsVisitor, update the method to judge one method is system part.

In OrganizeImport file, comment one statement which try to add system part for unresolved type list. After the patch, Organize import will not try to import system part if one type was not resolved.

Comment 11 Paul Harmon 2012-02-22 08:01:32 EST

Changes seem reasonable. If we ever need to find out if a part is a system part when in SDK mode, we will need to come up with a different mechanism, since there is only 1 IR environment in SDK. One way to do this would be to add a method in SystemEnvironment:

public boolean isSystemPart(String[] pkg, String partName) {
...
}

Comment 12 Brian Svihovec 2012-02-22 10:41:31 EST

If we are no longer using code, please remove it from the .java file and do not just comment it out.

Comment 13 Xiao Bin Chen 2012-02-23 04:40:37 EST

Paul,
Since we already have a function in IRUtils , so I did not add isSystemPart in SystemEnviroment.

I have delete the code line which commented before.

Thanks.

Comment 14 fahua jin 2012-02-28 21:45:19 EST

Verified in 0.8.0.v201202272101.