Community
Participate
Working Groups
(In reply to Noopur Gupta from bug 522205 comment #9) > (In reply to Stephan Herrmann from bug 522205comment #8) > > > When the created service provider is an interface, we get a compile error in > > > module-info.java - bug 525918. > > > > Sounds great. > > > > To follow-up on the interface case: have you considered creating a public > > static provider method? > > Or should that be subject of a follow-up quick fix, once Core correctly > > flags the missing provider method instead of the bogus "abstract" error? > > Not sure without checking the code. NewTypeWizardPage#createTypeMembers > should probably be overridden in that case for an interface. > > Also, I am not sure what stub should be created for that. > > From JLS 9 7.7.4 > (https://docs.oracle.com/javase/specs/jls/se9/html/jls-7.html#jls-7.7.4): > If a service provider has a provider method, then its return type must i) > either be declared in the current module, or be declared in another module > and be accessible to code in the current module; and ii) be a subtype of the > service specified in the provides directive; or a compile-time error occurs. > > Also, if the provider method is created in the interface then should the > interface still be a subtype of the service? > > From JLS 9 7.7.4: > If a service provider does not have a provider method, then that service > provider must have a provider constructor and must be a subtype of the > service specified in the provides directive, or a compile-time error occurs. > > Stephan, please create a new bug report to handle this with your suggestions > on the expected result. This quick fix should mend the error of an interface used in module-info after "with" where the interface has no suitable "provider method". So the code to insert would look s.t. like public static SomeService provider() { // TODO generated method stub return null; } Where candidates for SomeService should be determined like this: - subtype hierarchy of the type mentioned after "provides" of the same directive - accept only those types that are accessible in the current module If several types fulfill these criteria, they should all be offered in linked mode. The quick fix should also be offered for service classes that lack a proper "provider constructor" (public no-arg ctor) (declared or default ctor).
New Gerrit change created: https://git.eclipse.org/r/131471
Here's an observation that goes beyond the direct code review: I made the following stress-test: provides Serializable with org.test.MyIFC1; In this situation, hovering over the error leaves the UI unresponsive for several (~10) seconds (no surprise since we have *many* subtypes of Serializable). - I was surprised that such a long wait still produced an empty list of subTypes => bug 541472 - It may happen, that the type hierarchy contains lots of types that will then be filtered out due to inaccessibility. If performance is an issue then doing this in two steps is not optimal. Do we have the means to use a module scope during type hierarchy computation? Copying Manoj to comment on this, existent or desirable? How much would passing the IJavaProject to IType.newTypeHierarchy() already change? - Seeing that proposal computation takes long time even during hover computation I wonder if it has been considered to just create one proposal, disregarding the actual return type, and postpone the expensive computation till when the proposal is actually selected, in which case possible return types can be presented in linked mode. Given that the above is not a normal case, but indeed a stress test, we could investigate these issues in a follow-up bug, to not block shipping what we have.
Is this still planned for RC1? I see a couple of questions still unanswered: - Gerrit: the case of non-public types in the current module - Bugzilla: has single proposal with linked mode been considered? (comment 2) Even if all has been considered (one way or other) we'd still need approval by a project lead. @Kalyan, have you spoken to any lead?
(In reply to Stephan Herrmann from comment #3) > Is this still planned for RC1? > > I see a couple of questions still unanswered: > - Gerrit: the case of non-public types in the current module > - Bugzilla: has single proposal with linked mode been considered? (comment 2) > > Even if all has been considered (one way or other) we'd still need approval > by a project lead. @Kalyan, have you spoken to any lead? @Stephan I had spoken to Noopur, but as it is late for 4.10 RC1 I am moving it to 4.11. @Roland Please answer the questions raised by Stephan.
(In reply to Stephan Herrmann from comment #3) > Is this still planned for RC1? > > I see a couple of questions still unanswered: > - Gerrit: the case of non-public types in the current module I've added 'Flags.isPublic(t.getFlags())' for an IType t, derived from a subtype hierarchy search. This should leave out the non-public types of the current module. > - Bugzilla: has single proposal with linked mode been considered? (comment 2) I've modified the contribution to support linked mode. The service itself is suggested as the proposed return value, and all valid subtypes are added in linked mode. This way, the actual subtype hierarchy search is done once a user selects the proposal, and not while the quick fix proposal pop-up is coming up.
(In reply to Roland Grunberg from comment #5) > (In reply to Stephan Herrmann from comment #3) > > Is this still planned for RC1? > > > > I see a couple of questions still unanswered: > > - Gerrit: the case of non-public types in the current module > > I've added 'Flags.isPublic(t.getFlags())' for an IType t, derived from a > subtype hierarchy search. This should leave out the non-public types of the > current module. Not sure we're on the same page here. Shouldn't non-public types from the *current* module be allowed? The need to be public applies only to types from *other* modules, no? > > - Bugzilla: has single proposal with linked mode been considered? (comment 2) > > I've modified the contribution to support linked mode. The service itself is > suggested as the proposed return value, and all valid subtypes are added in > linked mode. This way, the actual subtype hierarchy search is done once a > user selects the proposal, and not while the quick fix proposal pop-up is > coming up. Thanks, I like that. If later we find that this still causes an undue delay, we could then try to trigger the subtype hierarchy in the background once the proposal is first shown, but let's not go into speculative optimization just now.
(In reply to Stephan Herrmann from comment #6) > (In reply to Roland Grunberg from comment #5) > > (In reply to Stephan Herrmann from comment #3) > > > Is this still planned for RC1? > > > > > > I see a couple of questions still unanswered: > > > - Gerrit: the case of non-public types in the current module > > > > I've added 'Flags.isPublic(t.getFlags())' for an IType t, derived from a > > subtype hierarchy search. This should leave out the non-public types of the > > current module. > > Not sure we're on the same page here. Shouldn't non-public types from the > *current* module be allowed? The need to be public applies only to types > from *other* modules, no? Yes, you're right. I removed the check, but then noticed that nested non-public types shouldn't be suggested (the quick fixes do complain they wouldn't be visible) but newTypeHierarchy(JavaProject) doesn't seem to filter them out. I've added a 'type.getDeclaringtype() == null || Flags.isPublic(t.getFlags())', which should filter such types.
@Stephan , let me know if you're fine with pushing this in, or if there's some other issues to resolve with the change.
Created attachment 277492 [details] test projects (In reply to Roland Grunberg from comment #8) > @Stephan , let me know if you're fine with pushing this in, or if there's > some other issues to resolve with the change. Sorry, I wasn't sure if you are waiting for me or vice versa :) Looking again today the change looks well engineered, with the sole exception that the visibility issue still isn't perfect. The attached projects make for a stress test of this aspect, by declaring lots of implementations of the interface IService with different relationships to the type containing the provider method. As an example of a bogusly proposed type consider class ServiceNonPublic which passes your test simply by being a toplevel type, but that's not enough, since it is package-private to another package, etc. OTOH, a private type within the same compilation unit (nested or not) *is* OK. To avoid re-inventing the wheel I looked and found JavaModelUtil.isVisible() which should keep most of the complexity at bay. I'll uploaded my take on the visibility issue in a minute.
I filed bug 544225 for module-related visibility checks, so we don't have to battle them here in this ticket. Here's my proposed visibility story: https://git.eclipse.org/r/#/c/131471/25..26 If Jenkins and Roland approve I'm ready to push.
Gerrit change https://git.eclipse.org/r/131471 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=dce4e94ce2f8a1fbd150e9ea7efdb7967bf69bd9
(In reply to Eclipse Genie from comment #11) > Gerrit change https://git.eclipse.org/r/131471 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=dce4e94ce2f8a1fbd150e9ea7efdb7967bf69bd9 Let's document: the change has been released for 4.11 M3 Thanks Roland for your persistence and solid work.
Thanks, Roland and Stephan. Roland, please add a N&N entry in the news doc for M3.
New Gerrit change created: https://git.eclipse.org/r/136534
While reviewing the N&N, I tried the following example and got NPE. Using a Java 11 project, where 'D' is an interface in package 'p': ------------------------------------- import java.sql.Driver; import p.D; module com.example { requires java.sql; provides Driver with D; } ------------------------------------- - Apply the proposal on 'D'. We get: java.lang.NullPointerException at org.eclipse.jdt.internal.ui.text.correction.proposals.NewProviderMethodDeclaration.performChange(NewProviderMethodDeclaration.java:81) at org.eclipse.jdt.ui.text.java.correction.CUCorrectionProposal.apply(CUCorrectionProposal.java:167) ...
Moving to RC1 to fix comment #15 and add N&N.
New Gerrit change created: https://git.eclipse.org/r/137229
Gerrit change https://git.eclipse.org/r/136534 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=bf8bb68b7804b69fc4435ad4311547d396f53e12
Gerrit change https://git.eclipse.org/r/137229 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3801df607a69be584b26e412ab29e251abc61998
+1 for RC1.
The "Create '...' provider method" quick fix would also make sense for a class that is not a subtype of the service specified in the provides directive, wouldn't it? For example, public class MyServiceProvider /* implements nothing */ {} See Java 11 Language Specification 7.7.4 (https://docs.oracle.com/javase/specs/jls/se11/html/jls-7.html#jls-7.7.4): "If a service provider does not have a provider method, then that service provider must have a provider constructor and must be a subtype of the service specified in the provides directive, or a compile-time error occurs." In addition, a "Create provider constructor" quick fix would be nice to have, too.
New Gerrit change created: https://git.eclipse.org/r/138146
Gerrit change https://git.eclipse.org/r/138146 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=e04ef11436a8b7587adbe09a8eaeba4905fcd935
