Bug 361327 - Static import resolution does not record all static elements being imported
Summary: Static import resolution does not record all static elements being imported
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.8   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.8 M4   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-19 01:18 EDT by Ayushman Jain CLA
Modified: 2011-12-05 04:43 EST (History)
2 users (show)

See Also:
srikanth_sankaran: review+


Attachments
proposed fix v1.0 (15.20 KB, patch)
2011-10-19 05:48 EDT, Ayushman Jain CLA
no flags Details | Diff
proposed fix v1.0 + regression tests (20.99 KB, patch)
2011-11-04 05:21 EDT, Ayushman Jain CLA
no flags Details | Diff
proposed fix v1.1+ regression tests (21.12 KB, patch)
2011-11-14 02:06 EST, Ayushman Jain CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ayushman Jain CLA 2011-10-19 01:18:12 EDT
HEAD

The static import resolution tries to find a static field, static method or static member type, in that order. As soon as something is found, it ignores the other possible static elements that are also coming from the same import.

package p2;

import static p1.Bar.B;
import static p3.Foo.B;   // javac complains, eclipse doesn't

public class OuterTest {

        public static void main(String [] args)
        {
            new OuterTest().beginTest();
         }
        public void beginTest(){
            System.out.print("1 + 1 =  ");
            if(alwaysTrue()){
                System.out.println("2");
            }
            else{
                System.out.println("3");
            }
        }
        public boolean alwaysTrue(){ // Returns FALSE in Eclipse
            String myB   =        B.class.getCanonicalName();
            String realB = p1.Bar.B.class.getCanonicalName();
            return myB.equals(realB);
        }
}

------------------------------------
package p1;
public class Bar {
    public static class  B{}
    final public static String B = new String("random"); 
}
------------------------------------
package p3;

public class Foo {
     public static class  B {}
}

In the above, it finds the field B from the import p1.Bar.B, and so does not find the type B. So when p3.Foo.B also imports B, no conflict is reported.

The above compiles with ECJ but fails with javac.
Comment 1 Ayushman Jain CLA 2011-10-19 01:19:49 EDT
I have a fairly cooked fix for this, but targetting 3.8M4 anyway, since we're towards the end of M3.
Comment 2 Ayushman Jain CLA 2011-10-19 05:48:25 EDT
Created attachment 205486 [details]
proposed fix v1.0

This patch tries to assuage the current static imports problem, by recording all possible static elements being imported by a static import.

It changes the current way of finding a single import as follows:
1) Find a single import in the usual way.
2) if we're looking at static import statement, there may be more static elements. 
  - If a field was found in (1), a method and a type may still be left out. 
  - If a method was found in (1), a type may still be left out.
  - If a type was found in (1), nothing more needs to be found.
3) For each of the above, the proper process to find duplicate/conflicting imports needs to be followed. As soon as an import is complained against, we bail out and move on to the next import.
4) Created two new fields - tempImports to replace the resolvedImports local variable (so that changing this across the new methods reflects reflects everywhere), and importPtr to replace the index local variable.
5) Created a new method recordImportBinding(..) to grow the imports array on demand and add a found import binding (Takes care of one to many mapping for each import).
6) Extracted the code in faultInImports() which checked for duplicates into a new method checkAndRecordImportBinding(..)
Comment 3 Ayushman Jain CLA 2011-11-04 05:21:11 EDT
Created attachment 206454 [details]
proposed fix v1.0 + regression tests

Added tests to the above patch.

Note that test85 and versions test the last 2 sentences in JLS 7.5.3, while test086 checks the obscuring rules in the same section.
Comment 4 Ayushman Jain CLA 2011-11-04 14:46:16 EDT
(In reply to comment #3)
> Created attachment 206454 [details] [diff]
> proposed fix v1.0 + regression tests

Note to self: All tests pass.
Comment 5 Srikanth Sankaran CLA 2011-11-10 04:36:21 EST
Ayush, patch looks good. Sorry for the delay. Here are a couple of comments:

 - I was surprised to see that checkMoreStaticBindings was not recursing
on itself but was duplicating the top half into bottom.

 - I think the code will be cleaner with checkMoreStaticBindings being a
void function. I didn't see the current return value being used in a
meaningful fashion. (control flow is the same irrespective of whether
-1 is returned or 0) - Did I miss something ?
Comment 6 Ayushman Jain CLA 2011-11-14 02:06:53 EST
Created attachment 206918 [details]
proposed fix v1.1+ regression tests

This patch takes care of above review comments.
Comment 7 Ayushman Jain CLA 2011-11-14 02:07:57 EST
Released in HEAD via commit 4889f3babd91783f30bca6f07ba58254cecf87d1
Comment 8 Jay Arthanareeswaran CLA 2011-12-05 04:43:56 EST
Verified for 3.8M4 with build I20111202-0800.