rewrite-migrate-java icon indicating copy to clipboard operation
rewrite-migrate-java copied to clipboard

`UseVarForObject` creates broken code when replacing type variable

Open uhafner opened this issue 1 year ago • 4 comments

What version of OpenRewrite are you using?

I am using

  • <rewrite-maven-plugin.version>5.39.2</rewrite-maven-plugin.version>
  • <rewrite-testing-frameworks.version>2.17.1</rewrite-testing-frameworks.version>
  • <rewrite-static-analysis.version>1.15.0</rewrite-static-analysis.version>
  • <rewrite-migrate-java.version>2.23.0</rewrite-migrate-java.version>
  • <rewrite-recommendations.version>1.8.4</rewrite-recommendations.version>

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.

uhafner avatar Sep 12 '24 13:09 uhafner

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 avatar Sep 12 '24 14:09 MBoegers

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 avatar Sep 12 '24 16:09 MBoegers

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

MBoegers avatar Sep 12 '24 17:09 MBoegers

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).

uhafner avatar Sep 12 '24 18:09 uhafner

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.

knutwannheden avatar Oct 31 '24 17:10 knutwannheden

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...

uhafner avatar Nov 01 '24 15:11 uhafner

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: 😊

knutwannheden avatar Nov 01 '24 16:11 knutwannheden