openrewrite / rewrite-kotlin

Work-in-progress implementation of Kotlin language support for OpenRewrite.
Apache License 2.0
43 stars 12 forks source link

Get rid of `TypeReferencePrefix` marker from `J.VariableDeclarations` #505

Closed kunli2 closed 9 months ago

kunli2 commented 9 months ago

This part of https://github.com/openrewrite/rewrite-kotlin/issues/477 To get rid of TypeReferencePrefix marker from J.VariableDeclarations.

Here are 3 TypeReferencePrefix use cases

1. Variable declaration statement in a block

class Test {
    var t /*C0*/ : /*C1*/ Int = 1 /*C2*/  ; /*C3*/
    fun method() {}
}

For this, this PR changes the spaces locations from

// "C0" -> Marker of `TypeReferencePrefix`
// "C1" -> prefix of typeExpression
// "C2" -> right padding of variable

to

// "C0" -> right padding of variable
// "C2" -> right padding of variable of the statement

2. Variable declaration as Parameters

fun method ( input /*C0*/ : Any = 1 /*C2*/ , x : Int ) {
    return "42"
}

Change from

// "C0" : marker of `TypeReferencePrefix`
// "C2" : right padding of the statement

to

// "C0" : right padding of variable
// "C2" : right padding of the statement

3. Method return type

fun method () /*C3*/ :  Int {
    return 42
}

This is not covered by this PR.

kunli2 commented 9 months ago

I think it may be simpler to solve TypeTree for the returnType of J.MethodDeclaration and apply that change to each location of a TypeReference, since each will be a TypeTree.

I don't think so, it's not about the simplify, and related to J.MethodDeclaration, but we don't need this marker for J.VariableDeclaration itself, it is unnecessary to have TypeReferencePrefix for J.VariableDeclaration, and for J.MethodDeclaration, the approach will be applied on J.MethodDeclation and different.

That will allow auto-formatting to visit a single location and apply the changes uniformly across the entire LST for the space before colons, as well as make it easier for users to make updates with recipes. Do you not agree?

I don't agree, I have updated the spaceVisitor in auto-formatting in this PR (for J.VariableDeclaration).

traceyyoshima commented 9 months ago

I don't think so, it's not about the simplify, and related to J.MethodDeclaration, but we don't need this marker for J.VariableDeclaration itself, it is unnecessary to have TypeReferencePrefix for J.VariableDeclaration, and for J.MethodDeclaration, the approach will be applied on J.MethodDeclation and different.

Okay, I'll trust your opinion and step away from the review. I think it's a mistake to retrofit the J trees in a custom way to fit Kotlin-specific whitespace.

traceyyoshima commented 9 months ago

Here are 3 TypeReferencePrefix use cases

I think there's been a misunderstanding, the reason I mentioned return types on method declarations and that we'll need someone to check the LST for other cases is that there are other positions for TypeTree where a : may exist. There is at least one additional case, and I haven't checked on any others.