What version of OpenRewrite are you using? I am using <ul dir=

Findings: Compilation errors being swallowed for classes provi

Here's a simpler test: <div class="highlight highlight-source-java notranslate pos

`ChangeType` not working with types provided to the parser using `dependsOn`. about rewrite HOT 6 OPEN

ammachado commented on June 20, 2024 1

`ChangeType` not working with types provided to the parser using `dependsOn`.

from rewrite.

Comments (6)

ammachado commented on June 20, 2024 1

Findings:

Compilation errors being swallowed for classes provided via dependsOn, if a class is not declared on the default package:

1255073806633000:3: error: class MySchemaType is public, should be declared in a file named MySchemaType.java
public class MySchemaType {}
       ^

There's a fix for this issue: #4040

Identified a possible small optimization on ChangeTypeVisitor - There's a fix for this issue: #4039
Package names used the failing test does not follow the the conventions, although their values are valid. For example, ChangeType uses JavaType.ShallowClass.build(oldFullyQualifiedTypeName) to create type references. The build method identifies names based on conventions; it considers that if an uppercase letter comes after a dot, the next identifier is a class name.

from rewrite.

ammachado commented on June 20, 2024 1

Updated the last comment with the fixes for the first 2 findings.

from rewrite.

timtebeek commented on June 20, 2024

That's a strange case indeed; thanks for the detailed runnable example. I wouldn't quite know why that's missed here. I'm guessing you ran into this when doing something similar in an actual recipe beyond this test case?

Thanks for the offer to look into this as well!

from rewrite.

timtebeek commented on June 20, 2024

Thanks for the fixes so far! I've given the test that you provided above another go and I can replicate it using only a capital letter in the package name already 🤔

@Test
void renamePackageWithDependsOn() {
    @Language("java") final String schemaClass1 = """
      package foo.Capital;
      public class MySchemaType {}
      """;
    @Language("java") final String schemaClass2 = """
      package bar.Capital;
      public class MySchemaType {}
      """;

    rewriteRun(
      spec -> spec
        .parser(JavaParser.fromJavaVersion().dependsOn(schemaClass1, schemaClass2))
        .recipe(new ChangeType(
          "foo.Capital.MySchemaType",
          "bar.Capital.MySchemaType",
          true
        )),
      //language=java
      java(
        """
          package my.example.app;
          
          import foo.Capital.MySchemaType;
          
          class MyApp {
              void doWork(MySchemaType mySchema) {}
          }
          """,
        """
          package my.example.app;
          
          import bar.Capital.MySchemaType;
          
          class MyApp {
              void doWork(MySchemaType mySchema) {}
          }
          """
      )
    );
}

Which again fails with

diff --git a/my/example/app/MyApp.java b/my/example/app/MyApp.java
index 7fe83d7..9ff90dd 100644
--- a/my/example/app/MyApp.java
+++ b/my/example/app/MyApp.java
@@ -1,7 +1,5 @@ 
 package my.example.app;
 
-import bar.Capital.MySchemaType;
-
 class MyApp {
     void doWork(MySchemaType mySchema) {}
 }

from rewrite.

timtebeek commented on June 20, 2024

Look like we misidentify an owning class where there is none, just because the package contains an uppercase letter. :/

from rewrite.

ammachado commented on June 20, 2024

Here's a simpler test:

@Test
void changeTypeWithCapitalLettersInPackageName() {
    @Language("java") final String schemaClass = """
      package bar.Capital;
      public class MySchemaType {}
      """;

    rewriteRun(
      java(schemaClass, spec -> spec.afterRecipe(cu -> {
            JavaType.FullyQualified typeFromParser = cu.getClasses().get(0).getType();

            // Called on ChangeClassDefinition constructor
            JavaType.ShallowClass shallowClass = JavaType.ShallowClass
              .build(typeFromParser.getFullyQualifiedName());

            assertThat(shallowClass)
              .hasFieldOrPropertyWithValue("fullyQualifiedName", typeFromParser.getFullyQualifiedName())
              .hasFieldOrPropertyWithValue("owningClass", typeFromParser.getOwningClass());
        }
      ))
    );
}

And a variant for classes starting with lowercase letters, also failing:

@Test
void changeTypeWithClassNamesStartingWithLowercaseLetters() {
    @Language("java") final String schemaClass = """
      package bar.capital;
      public class mySchemaType {}
      """;

    rewriteRun(
      java(schemaClass, spec -> spec.afterRecipe(cu -> {
            JavaType.FullyQualified typeFromParser = cu.getClasses().get(0).getType();

            // Called on ChangeClassDefinition constructor
            JavaType.ShallowClass shallowClass = JavaType.ShallowClass
              .build(typeFromParser.getFullyQualifiedName());

            assertThat(shallowClass)
              .hasFieldOrPropertyWithValue("fullyQualifiedName", typeFromParser.getFullyQualifiedName())
              .hasFieldOrPropertyWithValue("owningClass", typeFromParser.getOwningClass());
        }
      ))
    );
}

from rewrite.

`ChangeType` not working with types provided to the parser using `dependsOn`. about rewrite HOT 6 OPEN

Comments (6)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent