Bug 520874 - [compiler] difference in behavior in single static import and on-demand
Summary: [compiler] difference in behavior in single static import and on-demand
Status: RESOLVED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.7   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: BETA J9   Edit
Assignee: Jay Arthanareeswaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 525291 529777
  Show dependency tree
 
Reported: 2017-08-11 03:16 EDT by Manoj N Palat CLA
Modified: 2018-01-15 12:37 EST (History)
4 users (show)

See Also:


Attachments
code to reproduce the issue (260 bytes, text/plain)
2017-08-11 03:19 EDT, Manoj N Palat CLA
no flags Details
testcase 2 (341 bytes, text/plain)
2017-08-21 06:51 EDT, Manoj N Palat CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Manoj N Palat CLA 2017-08-11 03:16:33 EDT
Two different behaviours when an static private class is used.
See the attached code and uncomment the first one - error at Z
uncomment the second one, no
Comment 1 Manoj N Palat CLA 2017-08-11 03:19:28 EDT
Created attachment 269797 [details]
code to reproduce the issue

[submitted too soon]  no error reported if the on-demand import is on.
Javac reports error though.
[need to investigate which behaviour is correct] bug 370814 may have a related fix.
Comment 2 Manoj N Palat CLA 2017-08-21 06:51:50 EDT
Created attachment 269913 [details]
testcase 2


another test case with similar behaviour
Comment 3 Jay Arthanareeswaran CLA 2017-09-06 03:54:46 EDT
I see three different issues with respect to static imports:

1. With a static import, private member types are allowed in the enclosing type declaration, for e.g. in the type parameter. Sample:

package a;
import static a.A1.Outer.*;
public class B1 {}
class A1 {
	static class Outer extends Inner { // should be reported
		private static interface Inner {}
	}
	static class AnotherOuter<T extends AnotherInner> { // should be reported
		private static class AnotherInner {}
	}
}

2. Compiler rejects if a static import and a non static import import collide, even when they point to the same type. We should complain only when they are not resolved to the same type.

Sample: Have these in two different CUs:

package a;
import static a.A.B.Inner;
import a.Bar.Inner;
public class X {}
class A {
    static class B extends Bar {}
}

---------

package a;
public class Bar {
	public static class Inner {}
}

3. In complex cyclic hierarchy involving nested types and multiple CUs, we don't report error. Things work fine in reconciler (mostly because the other CUs are considered to be binary type and that execution takes a different route. But when all of them compiled at once, the cycle is not caught. At times it also depends on the order of the CUs being fed to the compiler.

I have a fix for first two and working on the third.
Comment 4 Eclipse Genie CLA 2017-09-06 09:56:11 EDT
New Gerrit change created: https://git.eclipse.org/r/104460
Comment 5 Jay Arthanareeswaran CLA 2017-09-06 10:03:10 EDT
(In reply to Eclipse Genie from comment #4)
> New Gerrit change created: https://git.eclipse.org/r/104460

This provides fix for first two items in comment #3. 

This also undoes the change made in Scope.java for bug 235658. The older fix returned resolved bindings for the private nested types. But this one changes it to ProblemReferenceBinding with problem reason NotVisible, which is just fine as far as code selection is concerned.
Comment 7 Eclipse Genie CLA 2017-09-07 02:12:34 EDT
New Gerrit change created: https://git.eclipse.org/r/104621
Comment 8 Jay Arthanareeswaran CLA 2017-09-08 00:07:59 EDT
(In reply to Eclipse Genie from comment #7)
> New Gerrit change created: https://git.eclipse.org/r/104621

A word on the patch:

When looking for cycles in hierarchies, we simply look for member type's enclosing types and see if they are involved in the hierarchy. But while doing so, we compare the enclosing type only if it same as the immediate sub type. This filters out all but one level of inheritance. So, I am removing that check. This also takes are of cycles when files are fed in different order.

In certain cases (GenericTypeTest.test1021b), we 'now' report an additional error, which would be reported if the cycle wasn't present. So, I will leave it there.
Comment 10 Jay Arthanareeswaran CLA 2017-09-08 21:59:08 EDT
All reported cases are addressed. Closing the report.
Comment 11 Stephan Herrmann CLA 2017-10-19 07:52:07 EDT
@Jay, this change breaks the following case:

//---
package p;
import p.B;
public class A extends B {
	public static abstract class C {}
}
//---
package p;
import p.A.C;
public class B extends C {}
//---

This looks similar to some of your cases, so the change must be intentional, but I don't understand the reason (and I have this exact pattern working in some of my code for a long time, so I have reasons to believe it is meaningful :) ).

I can see that ecj now agrees with javac in this regard, but I don't understand, why this should be an error. The static nested C does not *depend* on its enclosing class, does it? Are we missing some !isStatic() check?
Comment 12 Stephan Herrmann CLA 2017-10-31 07:40:14 EDT
(In reply to Stephan Herrmann from comment #11)
> @Jay, this change breaks the following case:

moved to its own bug 526681
Comment 13 Andrey Loskutov CLA 2018-01-15 12:37:17 EST
(In reply to Eclipse Genie from comment #6)
> Gerrit change https://git.eclipse.org/r/104460 was merged to [BETA_JAVA9].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=39f06c360c3afa72b1c211d620c70a07e6fca943

This commit caused regression, see bug 529777