Community
Participate
Working Groups
Given the following code in a java 9 project without module-info.java: package nonmodular; import java.sql.Connection; import java.util.Map.Entry; public class ProblemWithNested { } The following bogus error is reported: ---------- 1. ERROR in nonmodular\ProblemWithNested.java (at line 4) import java.util.Map.Entry; ^^^^^^^^^^^^^ The type java.util.Map is not visible ----------
reproduced.
This is inconsistently handled in different builder vs. reconciler settings. Till, if you have the time it would be phantastic if you could prepare this and related reports as tests in ModuleCompilationTest (batch) and ModuleBuilderTests (builder & reconciler). We are most likely searching for bugs in individual implementations of INameEnvironment.
(In reply to Stephan Herrmann from comment #2) > This is inconsistently handled in different builder vs. reconciler settings. > > Till, if you have the time it would be phantastic if you could prepare this > and related reports as tests in ModuleCompilationTest (batch) and > ModuleBuilderTests (builder & reconciler). Sorry it's getting too late today for me - I'm not even finished distilling test cases from bugs I can see in the workspace. I'll try to do that in the next evenings!
(In reply to Till Brychcy from comment #3) > (In reply to Stephan Herrmann from comment #2) > > This is inconsistently handled in different builder vs. reconciler settings. > > > > Till, if you have the time it would be phantastic if you could prepare this > > and related reports as tests in ModuleCompilationTest (batch) and > > ModuleBuilderTests (builder & reconciler). > > Sorry it's getting too late today for me .. I agree on that .. :) Don't worry, nobody is idly waiting ... I hope to find some time over the weekend to investigate this, let's see, who's first ... Anyway, bug reports and test cases either/both are highly appreciated!
New Gerrit change created: https://git.eclipse.org/r/105253
This is tricky and the proposed fix looks interesting. Still I want to spend a bit more time on it, as it kindof side-tracks my previous strategy: SPB.knownTypes is only populated when we askForType via this exact SPB. If SPB.getType0() answers null, this only indicates we don't yet have the full answer. OTOH, askForType() includes existing knowTypes in its search, if looking at an SPB. I want to check: - Will we still detect all same-named types from different (concealed) packages? - If the propagation strategy is OK, can we cut back some jumping-through-hoops in askForType()?
(In reply to Stephan Herrmann from comment #6) > This is tricky and the proposed fix looks interesting. > > Still I want to spend a bit more time on it, as it kindof side-tracks my Sure! Just take over! > previous strategy: SPB.knownTypes is only populated when we askForType via > this exact SPB. If SPB.getType0() answers null, this only indicates we don't I also thought about making a method "updateUnresolved" similar to addType, that only makes sure that any already stored URBs in the SPBs are updated (to avoid filling all that hashtables). But then I thought it would be better to hear about your opionion about the solution before adding any optimizations :-) > yet have the full answer. OTOH, askForType() includes existing knowTypes in > its search, if looking at an SPB. > > I want to check: > - Will we still detect all same-named types from different (concealed) > packages? > - If the propagation strategy is OK, can we cut back some > jumping-through-hoops in askForType()?
I found your approach very useful, just slightly extended it, see patch set #2. To validate if we have a sound story, let me try to explain: A URB whose fPackage is an SPB declares: somewhere here we need to find this type, we don't yet know which incarnation will hold it. Later, when we find the type (in a concrete PB!), we navigate to all related SBP (good: only one stop navigation - no zig-zag to other incarnations and beyond, those could have unrelated types). At each SBP we check: does it already have a corresponding URB? If so: - set the resolved type of the URB - let the SBP forget about the URB I'm not adding the resolved type to the SPB, because we haven't yet checked whether the type is without conflict within the SPB. Perhaps other incarnations contribute a same-named type? Instead, nulling out the SPB's type forces that subsequent lookup via this SPB needs to again ask all its incarnations, including the check for conflicts. Re-reading comment 7, this might be exactly what you had in mind? :) I just wouldn't call it an optimization, I think it's "more correct"... These are extreme corner cases, and I'm not sure how to create tests to demonstrate the subtle differences. That's why I rely on story telling. Does my story make sense?
(In reply to Stephan Herrmann from comment #8) > I found your approach very useful, just slightly extended it, see patch set > #2. > > To validate if we have a sound story, let me try to explain: > > A URB whose fPackage is an SPB declares: somewhere here we need to find this > type, we don't yet know which incarnation will hold it. > > Later, when we find the type (in a concrete PB!), we navigate to all related > SBP (good: only one stop navigation - no zig-zag to other incarnations and > beyond, those could have unrelated types). > At each SBP we check: does it already have a corresponding URB? If so: > - set the resolved type of the URB > - let the SBP forget about the URB > > I'm not adding the resolved type to the SPB, because we haven't yet checked > whether the type is without conflict within the SPB. Perhaps other > incarnations contribute a same-named type? Which could happen, but only in the Unnamed module. In a "real" module, only one incarnation may provide types for a package, right? > > Instead, nulling out the SPB's type forces that subsequent lookup via this > SPB needs to again ask all its incarnations, including the check for > conflicts. I thought about that, too, but the problem is: If there already is an URB, then at least one BTB will now use the just resolved type. So it would be even worse, if consecutive lookups provide a type from a different incarnation. > > Re-reading comment 7, this might be exactly what you had in mind? :) Yes. > I just wouldn't call it an optimization, I think it's "more correct"... See above comment. > > These are extreme corner cases, and I'm not sure how to create tests to > demonstrate the subtle differences. That's why I rely on story telling. Does > my story make sense? Yes.
This may well be a can of worms for which we supply only a band-aid now, and have to work on a full solution for the next release. Let me draw a basic scenario where these things would become relevant: Binary modules: M1, M2, M3, all containing a package 'p' N1 reads M1 & M2 N2 reads M2 & M3 Source module being compiled: O reads N1 & N2, but none of M1-M3 This results in two split packages: p' from M1, M2 p'' from M2, M3 package M2.p has both p' and p'' in its wrappingSplitPackageBindings We can now populate this space in a legal or illegal way: Legal: M1 exports p, declares class p.C M2 exports p.q, declares class p.q.X M3 exports p, declares class p.C No module reads M1 & M3, hence there's no conflict re unique visibility. Illegal M1 exports p, declares class p.C M2 exports p, declares class p.C M3 exports p.q, declares class p.q.X p is exported to N1 from both M1 & M2 -> violation of unique visibility. Note that N1, N2 may contain exported types with signatures that mention p.C, exported to O. Now O needs to be able to 'understand' the 'invisible' type p.C, even distinguish both variants. Some things we need to avoid: In the legal case, the binding for M1/p.C must not leak into p''. In the illegal case, the URB(p.C in p'') must not prevent asking for M1/p.C. The URB is conflict-free. If we declare this also as a knownType in p', we will not discover the conflict - not via the type, i.e. TODO: check if detection of package conflicts including hasCompilationUnit()-checks is sufficient to detect this error. In these regards I feel patch set #2 to be closer to the target, close but perhaps no cigar. I couldn't quite come up with an example for your concerns in comment 9. We surely need more BTB's in N1 or N2 to create such example. What else? What wrong outcome could patch set #2 create? Proposal: release patch set #2 now, file a follow-up bug for 4.8 to discuss and fix any remaining issues. There are other bugs where the remaining time between now and the release can probably create more value for users. WDYT?
(In reply to Stephan Herrmann from comment #10) > Proposal: release patch set #2 now, file a follow-up bug for 4.8 to discuss > and fix any remaining issues. There are other bugs where the remaining time > between now and the release can probably create more value for users. WDYT? Good for me!
Gerrit change https://git.eclipse.org/r/105253 was merged to [BETA_JAVA9]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=ee1456d22bc6413cf0655e6ab4304dc6965f4445
(In reply to Eclipse Genie from comment #12) > Gerrit change https://git.eclipse.org/r/105253 was merged to [BETA_JAVA9]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=ee1456d22bc6413cf0655e6ab4304dc6965f4445 > Done for now, follow-up is bug 522403 Thanks!
