openrewrite / rewrite-migrate-java

OpenRewrite recipes for migrating to newer versions of Java.
Apache License 2.0
105 stars 73 forks source link

`UseVarForObject` creates broken code when replacing type variable #550

Closed uhafner closed 1 week ago

uhafner commented 1 month ago

What version of OpenRewrite are you using?

I am using

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project. I did run the command mvn -X clean rewrite:run.

See https://github.com/uhafner/codingstyle for details. The OpenRewrite configuration is visible in the pom.xml.

What is the smallest, simplest way to reproduce the problem?

package edu.hm.hafner.util;

import java.io.Serializable;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

public abstract class SerializableTest<T extends Serializable> extends ResourceTest {
    protected abstract T createSerializable();

    @Test
    @DisplayName("should be serializable: instance -> byte array -> instance")
    void shouldBeSerializable() {
        T serializableInstance = createSerializable();
    }
}

What did you expect to see?

Type variable should not be replaced by var.

package edu.hm.hafner.util;

import java.io.Serializable;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

public abstract class SerializableTest<T extends Serializable> extends ResourceTest {
    protected abstract T createSerializable();

    @Test
    @DisplayName("should be serializable: instance -> byte array -> instance")
    void shouldBeSerializable() {
        T serializableInstance = createSerializable();
    }
}

What did you see instead?

package edu.hm.hafner.util;

import java.io.Serializable;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

public abstract class SerializableTest<T extends Serializable> extends ResourceTest {
    protected abstract T createSerializable();

    @Test
    @DisplayName("should be serializable: instance -> byte array -> instance")
    void shouldBeSerializable() {
        var serializableInstance = __P__.<error>;
    }
}

What is the full stack trace of any errors you encountered?

No stack trace produced.

Are you interested in [contributing a fix to OpenRewrite]

Not right now.

MBoegers commented 1 month ago

In this case the derivation of the type seems broken. Such change is indeed not correct. The expectation seems right, I'll check what's missing. Thanks for reporting πŸ™

MBoegers commented 1 month ago

We can reproduce this with the test below. The issue is caused be the type boundary (extends Serializable) at the generic parameter. Further exploration is needed here.

@Test
    @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/550")
    void genericType() {
        rewriteRun(
          java(
            """
              import java.io.Serializable;

              abstract class Outter<T extends Serializable> {
                 abstract T doIt();
                 void trigger() {
                    T x = doIt();
                 }
              }
              """, """
              import java.io.Serializable;

              abstract class Outter<T extends Serializable> {
                 abstract T doIt();
                 void trigger() {
                    var x = doIt();
                 }
              }
              """
          )
        );
    }
MBoegers commented 1 month ago

One additional question @uhafner, why are you expecting T instead of var? var is perfectly fine here, from the JLS view.

uhafner commented 1 month ago

One additional question @uhafner, why are you expecting T instead of var? var is perfectly fine here, from the JLS view.

You are right, it works with var as well (I never used var for type variables before).

knutwannheden commented 1 week ago

Hi @uhafner! Long time no see !πŸ˜„ I think the linked PR should fix your issue. Once merged there should soon be snapshot releases including the fix.

uhafner commented 1 week ago

Hi @uhafner! Long time no see !πŸ˜„ I think the linked PR should fix your issue. Once merged there should soon be snapshot releases including the fix.

πŸ‘‹ Hi @knutwannheden. You are right, it's been a few years since we met in person πŸ˜„ I'm now back in Munich to teach students software engineering with open source tools here at the university of applied sciences... Good to see that you are still working in the area of open source! I also try to spent my rare spare time in my open source Jenkins projects...

knutwannheden commented 1 week ago

I've gone full circle and develop parsers now again as I did way back when. Surprisingly enough the full-time SQL I practiced back then still comes in handy every now and then: 😊