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

[BUG] problem with setter when extending a class with the same simple name #2268

Closed
mateidavid opened this issue Oct 20, 2019 · 2 comments
Closed

Comments

@mateidavid
Copy link

Describe the bug

Original report on the Google group:
https://groups.google.com/d/msg/project-lombok/YZ8CRFX8HFA/ZSZIa4c2BgAJ

When the Lombok annotation processor generates a chained setter for an inner class (e.g. A.Inner), it attempts to use as return type only the simple name of the inner class (Inner), as opposed to the partially qualified name that includes the enclosing class (A.Inner). In some contexts the simple name of the inner class might not be unique, leading to compilation error.

To Reproduce

interface A {
   int a();

   @FieldDefaults(level = AccessLevel.PRIVATE)
   @Getter
   @Setter
   @Accessors(fluent = true)
   class Mutable implements A {
      int a;
   }
}

interface B extends A {
   int b();

   @FieldDefaults(level = AccessLevel.PRIVATE)
   @Getter
   @Setter
   @Accessors(fluent = true)
   class Mutable extends A.Mutable implements B {
      int b;
   }
}

On build, this produces an error that in B.Mutable, the reference to Mutable is ambiguous. Most likely, that's because Lombok produces:

public Mutable b(int b) {
   this.b = b;
   return this;
}

... and in that context Mutable is ambiguous between A.Mutable and B.Mutable.

Expected behavior

Have the setter return type use the partially qualified name that includes the enclosing class, elimintaing the ambiguity:

public B.Mutable b(int b) {
   this.b = b;
   return this;
}

Version info (please complete the following information):

  • Lombok 1.18.8
  • JDK 1.8.0_221
  • IntelliJ 2019.1.1

Additional context

  • Note that IntelliJ does not show an error, suggesting the Lombok IntelliJ plugin somehow does the right thing.
  • The workaround is to de-lombok the setters inside B.Mutable.
@ghunteranderson
Copy link

This is so interesting! I spent some time digging into this and wanted to share what I found. The tldr is that everything confirms what @mateidavid suggested with one addition. All outer classes need to be specified, not just the most immediate.

Original Issue

I debugged the @Setter handler and confirmed your assumption about the generated code is correct. It just returns type Mutable. I've been playing around with your example to get the minimal code that still creates the issue. Here's how far I got.

interface A {
    class Foo {}
}

interface B {
    @Setter
    @Accessors(fluent = true)
    class Foo implements A, B {
    	private int n;
    }
}

which generates

interface A {
    class Foo {}
}

interface B {
    class Foo implements A, B {
        private int n;
		
        public Foo n(int n) {
            this.n = n;
            return this;
        }
    }	
}

The compiler error wasn't obvious to me at first because I expected B.Foo would take precedence since the method was in that class. Instead, the types brought into scope by implementing the interfaces take precedence and conflict with each other because both Foo's are brought into scope at the same level of precedence. This does indeed go away if B.Foo#n(int) specifies return type B.Foo instead of Foo

Other Symptoms

I also found that by removing B from B.Foo's list of interfaces it implements, we get a new error caused by the same bug. In this variation, we expected to return B.Foo but Lombok specified Foo which was resolved to A.Foo. The actual compiler error is below and the fix mentioned above fixes this as well.

LombokTest.java:9: error: incompatible types: B.Foo cannot be converted to A.Foo
        @Setter
        ^
1 error

Deeper Class Nesting

One more, just because I believe it sheds a little more light on the fix.

interface A {
    class Bar {
        class Foo {}		
    }
}

interface B {
    class Bar {
        class Foo implements A, B {
            Bar.Foo b() {
                return this;
            }
        }
    }
}

In the sample above, we apply the fix of specifying the outer type in the return type expression with the intent to disambiguate but it fails to do so. Bar.Foo becomes ambiguous just like in the original issue.

It seems the return type needs to be qualified by all outer classes in the nesting not just the nearest outer class.

@ghunteranderson
Copy link

I suspected @Builder had the same bug, and it does. Check out this weird code below.

interface A {
    class Foo {
        public Foo(int i) {
            System.out.println("This shouldn't be happening");
    	}
    }
}

interface B {
    @Builder
    class Foo implements A{
        private int variable;
    }
}

public class LombokTest{
    public static void main(String[] args) {
        A.Foo obj = B.Foo.builder().variable(2).build();
    }
}

A few things I noticed attempting this

  1. If we add interface B to B.Foo, then the return type is ambiguous and it will not compile, same as the original issue filed
  2. The builder tries to invoke new Foo(int) but in the context it resolves to the wrong constructor, new A.Foo(int), which may not exist.
  3. If the constructor does exists (like above) it will successfully call it at runtime and construct an instance of A.Foo (cool but not good).
  4. Because the .build() method returns Foo, it will return (both by signature and at runtime) an instance of A.Foo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants