search

eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
158 stars 125 forks source link

CompilationUnit.types() implementation inconsistent with javadoc #2481

Open iloveeclipse opened 4 months ago

iloveeclipse commented 4 months ago

Regression from https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2066

Either org.eclipse.jdt.core.dom.CompilationUnit.types() implementation or javadoc or ImplicitTypeDeclaration must be fixed, as CompilationUnit.types() now can return also ImplicitTypeDeclaration elements (which is NOT subtype of AbstractTypeDeclaration) - and that may affect clients expecting only AbstractTypeDeclaration.

Originally posted by @iloveeclipse in https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2066#issuecomment-2122966449

iloveeclipse commented 4 months ago

@datho7561, @stephan-herrmann, @jarthana : would it be possible to discuss ASAP implications of ImplicitTypeDeclaration being NOT subtype of AbstractTypeDeclaration (which is the actual root cause of the bug here)?

If we release it now "as is", we would not able to change type hierarchy in next release, as it would be breaking API change.

With the current implementation, I see few SDK types that rely on org.eclipse.jdt.core.dom.CompilationUnit.types() to return List<AbstractTypeDeclaration> list, apply this patch (which is not OK, but just to see direct compilation dependencies):

diff --git a/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/CompilationUnit.java b/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/CompilationUnit.java
index 4a12381..b2f7377 100644
--- a/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/CompilationUnit.java
+++ b/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/CompilationUnit.java
@@ -1166,3 +1166,3 @@
     */
-   public List types() {
+   public List<AbstractUnnamedTypeDeclaration> types() {
        return this.types;
stephan-herrmann commented 4 months ago

One reason against making ImplicitTypeDeclaration a subclass of AbstractTypeDeclaration is the absence of a name.

For this I quoted the precedent in IPackageBinding:

    /**
     * Returns the name of the package represented by this binding. For named
     * packages, this is the fully qualified package name (using "." for
     * separators). For unnamed packages, this is an empty string.
     *
     * @return the name of the package represented by this binding, or
     *    an empty string for an unnamed package
     */
    @Override
    public String getName();

To which @datho7561 responded:

In AbstractTypeDeclaration the name is lazily initialized. The implementation of getName creates a blank AST Node for the name if it's null. setName forbids you from setting the name to null. I think some methods callers have assumed that getName will never return null. We'd need to change them in order to accommodate this, and also come up with a way to lazily initialize the name while not creating a blank AST node in the case that it's unnamed.

The example of IPackageBinding, however, shows how to avoid null: use an empty String, viz set a SimpleName with identifier "". Currently, SimpleName.setIdentifier() requires a valid java identifier, but we might get around this by creating a new EmptyName subclass of either SimpleName or directly Name, whichever works better.

Question: is the name the only property that needs to be "suppressed"?

stephan-herrmann commented 4 months ago

If we release it now "as is", we would not able to change type hierarchy in next release, as it would be breaking API change.

Don't we have provisions for marking API for preview features as provisional?

iloveeclipse commented 4 months ago

Don't we have provisions for marking API for preview features as provisional?

AFAIK only by adding this as javadoc:

 * <p>
 * <strong>EXPERIMENTAL</strong>. This class or interface has been added as part
 * of a work in progress. There is no guarantee that this API will work or that
 * it will remain the same. Please do not use this API without consulting with
 * the API development team.
 * </p>
datho7561 commented 4 months ago

I started playing with the codebase using Andrey's suggestion of updating the return type of types() to AbstractUnnamedTypeDeclaration temporarily to see the implications.

Question: is the name the only property that needs to be "suppressed"?

Although it's not a property, there is also resolveBinding which is not a part of AbstractUnnamedTypeDeclaration, since implicitly declared classes cannot be referred to by name, so it makes no sense for them to have a binding.

we might get around this by creating a new EmptyName subclass of either SimpleName or directly Name, whichever works better.

We'd also need to be careful with the source range of the node that's created. If I recall correctly, the AST validator throws an Exception if the nodes in an AST tree are out of order. We can probably work around this, but it will require some thought.

stephan-herrmann commented 4 months ago

Question: is the name the only property that needs to be "suppressed"?

Although it's not a property, there is also resolveBinding which is not a part of AbstractUnnamedTypeDeclaration, since implicitly declared classes cannot be referred to by name, so it makes no sense for them to have a binding.

If an implicit class has a method, that method has a binding, what should it answer as its declaring class? :smile:

I believe it's simpler to make it behave like an almost-normal class. See also that IPackageBinding representing an unnamed package cannot be referred to by name, but it's still a useful thing.

mpalat commented 4 months ago

@datho7561, @stephan-herrmann, @jarthana : would it be possible to discuss ASAP implications of ImplicitTypeDeclaration being NOT subtype of AbstractTypeDeclaration (which is the actual root cause of the bug here)?

If we release it now "as is", we would not able to change type hierarchy in next release, as it would be breaking API change.

@akurtakov : Jarthana is on vacation - so there may be a delay in reply.. so jumping in with my 2 cents: This is still a preview feature. Ideally we should not have had this as an API. What is generally done is to put "@noreference" to all Preview Feature Classes/APIs until it become standard. In this specific case, in Java 23, this is still a preview feature - JEP 477. So I believe we should put a PR to make this "noreference", and as per https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/ApiRemovalProcess.md, deprecate this API and ask for a rebuild of RC1. we can also thus delay the discussion on how this API should look like to a later release.

srikanth-sankaran commented 4 months ago

@datho7561, @stephan-herrmann, @jarthana : would it be possible to discuss ASAP implications of ImplicitTypeDeclaration being NOT subtype of AbstractTypeDeclaration (which is the actual root cause of the bug here)? If we release it now "as is", we would not able to change type hierarchy in next release, as it would be breaking API change.

@akurtakov : Jarthana is on vacation - so there may be a delay in reply.. so jumping in with my 2 cents: This is still a preview feature. Ideally we should not have had this as an API. What is generally done is to put "@noreference" to all Preview Feature Classes/APIs until it become standard. In this specific case, in Java 23, this is still a preview feature - JEP 477. So I believe we should put a PR to make this "noreference", and as per https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/ApiRemovalProcess.md, deprecate this API and ask for a rebuild of RC1. we can also thus delay the discussion on how this API should look like to a later release.

@mpalat could you look into this PR you are suggesting - I would like it for @jarthana to be able to comment on the larger issue. if @noreference would buy us time to make an unhurried decision, that would be good

mpalat commented 4 months ago

@iloveeclipse Could you please review the PR? thanks!

iloveeclipse commented 4 months ago

@datho7561 : main change is merged now, could you please follow up on @mpalat suggestion and add noreference annotation on the new ImplicitTypeDeclaration type?

mpalat commented 4 months ago

@datho7561, @jarthana - another quick question -> What is the expected behaviour in IDE?

When I put this code -> """ static String str = "1";

public static void main(String... args) { System.out.println(str); } """ in an X.file, it compiles. However, when I try running it using Run Configurations it complains saying that "Editor does not have a main type". Of course, running with "java --enable-preview X" runs it and print 1 the correct result. In case this point was already discussed, sorry to have missed it - please point. Thx.

jarthana commented 4 months ago

@datho7561, @jarthana - another quick question -> What is the expected behaviour in IDE?

When I put this code -> """ static String str = "1";

public static void main(String... args) { System.out.println(str); } """ in an X.file, it compiles. However, when I try running it using Run Configurations it complains saying that "Editor does not have a main type". Of course, running with "java --enable-preview X" runs it and print 1 the correct result. In case this point was already discussed, sorry to have missed it - please point. Thx.

I don't think this was discussed. I think this needs to be handled in jdt ui or debug where we create the Java launch configs. The fix imo should be to just pass the preview flag to the JRE.

datho7561 commented 4 months ago

could you please follow up on @mpalat suggestion

See https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2498

What is the expected behaviour in IDE?

@mpalat Jay is right, I haven't looked into implementing anything in JDT UI, but something would need to be added/changed there to get it to work properly.