Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Sealed classes] Incorrect Reconciler error: The type A that implements a sealed interface I should be a permitted subtype of I #1998

Closed
srikanth-sankaran opened this issue Feb 9, 2024 · 8 comments · Fixed by #2601
Assignees
Milestone

Comments

@srikanth-sankaran
Copy link
Contributor

Snippet:

sealed interface I {}
final class A implements I {}

record R<T extends I>(T x, T  y) {}

public class X {
    public static int foo(R r) {
       return  switch (r) {
            case R(A a1, A a2) -> 0;
        };
    }

    @SuppressWarnings("unchecked")
       public static void main(String argv[]) {
       System.out.println(X.foo(new R(new A(), new A())));
    }
}
 

On a save, problem marker vanishes. Edit the file, reappears.

@srikanth-sankaran srikanth-sankaran self-assigned this Feb 9, 2024
@lukeu
Copy link

lukeu commented Mar 14, 2024

I am also observing something "weird and seemingly stateful" happening with sealed interfaces/classes.

If I add sealed and permits onto an interface that has 2 derived classes, both those classes give false compile errors. (Separate files, same package, all in the 'unnamed' module.)

I upgraded Eclipse from 4.30 -> 4.31 but that didn't seem to help.

Interestingly, if I clean + build the one project containing this modification:

  • All errors go away in that project
  • Errors about sealed-interfaces suddenly appear in other projects that depend on the modified one!
  • So then I must clean + build those also.
  • It seems to stay error-free unless I touch anything in a sealed-type hierarchy again.

This leads to needing a frequent "clean + rebuild all" workaround, which won't be scalable for my workspace. So I think this blocks me trying to adopt sealed any further (and I will hope that no other devs on the team add it in the meantime).

I would be happy to aid with testing any fixes on this big-ish workspace. (I can't promise how responsive I'll be, but I'll try.)

@srikanth-sankaran
Copy link
Contributor Author

I am also observing something "weird and seemingly stateful" happening with sealed interfaces/classes.

If I add sealed and permits onto an interface that has 2 derived classes, both those classes give false compile errors. (Separate files, same package, all in the 'unnamed' module.)

I upgraded Eclipse from 4.30 -> 4.31 but that didn't seem to help.

Interestingly, if I clean + build the one project containing this modification:

  • All errors go away in that project
  • Errors about sealed-interfaces suddenly appear in other projects that depend on the modified one!
  • So then I must clean + build those also.
  • It seems to stay error-free unless I touch anything in a sealed-type hierarchy again.

This leads to needing a frequent "clean + rebuild all" workaround, which won't be scalable for my workspace. So I think this blocks me trying to adopt sealed any further (and I will hope that no other devs on the team add it in the meantime).

I would be happy to aid with testing any fixes on this big-ish workspace. (I can't promise how responsive I'll be, but I'll try.)

I think what you are hitting is a problem in incremental builds/separate compilation. Thanks for the offer to test - I will propose a patch soon in the context of https://bugs.eclipse.org/bugs/show_bug.cgi?id=577872 - it would be great if you can test and report if that addresses your problem

@lukeu
Copy link

lukeu commented Jun 30, 2024

I will propose a patch soon in the context of https://bugs.eclipse.org/bugs/show_bug.cgi?id=577872 - it would be great if you can test and report if that addresses your problem

tl:dr; I tested - looks good!

What I did to test was run my usual Eclipse SDK 2024-06 (4.32) and upgraded just the JDT tools using this update site:

https://download.eclipse.org/eclipse/updates/4.33-I-builds/I20240628-1800/

I no longer recollect which classes I was trying to convert to sealed-interfaces + records when I last hit this. However, I found some files known to be problematic (if I touched them at all in the editor, errors appear and the code cannot execute). These issues all look to be resolved in the new JDT, from what I can tell.

(Now a small side-issue for me is that this JDT update did not get registered in the "installation history" so I cannot seem to revert it. Oh well, hopefully it doesn't cause trouble when upgrading to 4.33. Unfortunately from past experience, I think this does...)

@srikanth-sankaran
Copy link
Contributor Author

Thanks @lukeu very much!

For the install issue I'll request @iloveeclipse to comment - it is out of my area.

@lukeu
Copy link

lukeu commented Jul 3, 2024

Ooh, I have found one regression with the nightly version of the JDT compiler, which seems limited to "sealed interface with nested records". There is a compiler error reported in the 2nd line of the main function - saying "Cannot cast from Object to Either<String, Integer>"

    public sealed interface Either<L,R> {
    
        L getLeft();
        R getRight();
    
        record Left<L, R>(L error) implements Either<L, R> {
            @Override
            public L getLeft() {
                return error;
            }
            @Override
            public R getRight() {
                throw new NoSuchElementException();
            }
        }
    
        record Right<L, R>(R value) implements Either<L, R> {
            @Override
            public L getLeft() {
                throw new NoSuchElementException();
            }
            @Override
            public R getRight() {
                return value;
            }
        }
    
        public static void main(String[] args) {
            Object o = new Left<String, Integer>("boo");
            var either = (Either<String, Integer>) o;
            System.out.println(either.getLeft());
        }
    }

Running this with java Either.java in JDK-17 or JDK-21 issues only a unchecked-cast warning, not an error.

(I was updating a Scala-inspired Either class, if interested in the background)

@srikanth-sankaran
Copy link
Contributor Author

Can you open a fresh ticket please ?? When you say a regression, did it work earlier with some version of eclipse ? If so could you cite the version please. I'll take a look soon. I am working on cleaning up the entire sealed types implementation in ECJ and defect reports are very welcome and timely now.

@lukeu
Copy link

lukeu commented Jul 3, 2024

Can you open a fresh ticket please ??

Sure, see: #2667

@lukeu
Copy link

lukeu commented Jul 5, 2024

Just to follow up & for posterity, I've solved this:

(Now a small side-issue for me is that this JDT update did not get registered in the "installation history" so I cannot seem to revert it)

gory details: I had searched "JDT" in the "Install" dialog, but it matched something different, "Eclipse compiler for Java" at a top level (outside Eclipse Project SDK). This indeed caused conflicts updating - however it was in the revert dialog all along. With the new name worked out, I could unwind that and upgrade everything to 4.33-M1.

robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this issue Jul 18, 2024
…ts a sealed interface I should be a permitted subtype of I (eclipse-jdt#2601)

* Fixes eclipse-jdt#1998

* Disable failing tests from thus far unhooked junit (See
eclipse-jdt#2602)
gayanper pushed a commit to gayanper/eclipse.jdt.core that referenced this issue Sep 7, 2024
…ts a sealed interface I should be a permitted subtype of I (eclipse-jdt#2601)

* Fixes eclipse-jdt#1998

* Disable failing tests from thus far unhooked junit (See
eclipse-jdt#2602)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants