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

Improve quality of LST: replace applicable markers with LST elements #477

Open
5 of 6 tasks
traceyyoshima opened this issue Nov 29, 2023 · 5 comments
Open
5 of 6 tasks
Assignees

Comments

@traceyyoshima
Copy link
Contributor

traceyyoshima commented Nov 29, 2023

Replace:

  • AnnotationUseSite:
    The J tree expects J.Annotation(s), a final class. In K, there is an additional suffix after the use site annotation like @file : .... This change may require an update to J.Annotation since various classes require it.
  • IsNullable:
    Nullable is a generally applicable concept across J and every implementation of J, and may be generally fixable. Deprecate IsNullable marker and use K.Unary instead #489
    Deprecate IsNullable marker and use J.NullableType instead #497
  • IsNullSafe:
    Possible solution: Add an IsNullSafe Expression pass-through to contain the applicable expression.
  • Modifier: The modifier marker was supposed to be replaced in the PSI parser with the J.Modifier#keyword, and appears to still be in use.
  • TypeReferencePrefix
    Possible solution: Add a TypeReference TypeTree tree to contain the applicable expression.

Update:

  • Elvis:
    This marker exists in both Groovy and Kotlin, and will likely be reused elsewhere. We should probably add this to rewrite-java and use the reuse the marker.
@traceyyoshima traceyyoshima added the enhancement New feature or request label Nov 29, 2023
@traceyyoshima traceyyoshima changed the title Improve LST model: Replace markers with LST trees. Improve quality of LST: Replace markers with LST trees. Nov 29, 2023
@traceyyoshima traceyyoshima changed the title Improve quality of LST: Replace markers with LST trees. Improve quality of LST: replace applicable markers with LST elements Nov 29, 2023
@timtebeek timtebeek moved this to Backlog in OpenRewrite Nov 29, 2023
@kunli2
Copy link
Contributor

kunli2 commented Nov 30, 2023

For AnnotationUseSite marker, I propose we create a new model K.Annotation to present KtAnnotation

@kunli2
Copy link
Contributor

kunli2 commented Nov 30, 2023

Elvis marker is not used anymore in Kotlin and has been changed a K.Binary, so we can deprecate it directly here.

@traceyyoshima
Copy link
Contributor Author

  • The J tree expects J.Annotation(s), a final class. In K, there is an additional suffix after the use site annotation like @file : .... This change may require an update to J.Annotation since various classes require it.

How does this work for the requirements of J.Annotation?

@kunli2
Copy link
Contributor

kunli2 commented Dec 1, 2023

I double checked IsNullSafe marker, there are total 3 usage scenarios

  1. J.FieldAccess, example: test ?. property, the space before ?. is handled by leftPadded
  2. J.MethodInvocation, example:
              val filter = null   ?. let { _ ->
                  { false
                  }
              }

the space before ?. is handled by suffix of the select (RightPadded)

  1. J.TypeCast, example val map = mapOf(Pair("one", 1)) as? Map<*, *>, the space before ?. is handled by the prefix of J.TypeCast.clazz

so I think the prefix in this marker is not necessary, we can just deprecate the prefix field and keep the marker there.
@knutwannheden @traceyyoshima what do you think?

@knutwannheden
Copy link
Contributor

Let's also try to be careful to not break LST deserialization as we do these refactorings. In moderne-ast-write we can set up regression tests, so that we know if we need to customize deserialization or not.

@traceyyoshima traceyyoshima changed the title Improve quality of LST: replace applicable markers with LST elements Priority High: Improve quality of LST: replace applicable markers with LST elements Dec 20, 2023
@traceyyoshima traceyyoshima changed the title Priority High: Improve quality of LST: replace applicable markers with LST elements Improve quality of LST: replace applicable markers with LST elements Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

4 participants