Update Kotlin DSL rewriting functionality to use KotlinEditor #1299

wsutina · 2024-11-01T07:03:50Z

📝 Description

Updating the RewritingTask to use kotlin-editor, which contains an updated grammar for more complex Kotlin Gradle build scripts. The grammar used for parsing Groovy build scripts remains unchanged.

Note: this depends on cashapp/kotlin-editor#17

gradle/libs.versions.toml

autonomousapps

This is looking really good! Thanks! <3

src/main/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriter.kt

src/test/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriterTest.kt

autonomousapps

Looks great! Thank you <3 Left a few comments.

autonomousapps · 2024-11-07T17:21:49Z

src/main/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriter.kt

+  }
+
+  override fun exitNamedBlock(ctx: NamedBlockContext) {
+    dependencyExtractor.onExitBlock()


I think the (implied) contract of this function is that it should be at the bottom of the exitNamedBlock function.

Done in d36ba8a#diff-b2a94f740ad8ff6516bf944965d212e085314087d0edc9cc852710ad1ef35c21R94-R95

autonomousapps · 2024-11-07T17:25:27Z

src/main/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriter.kt

+    advice.filterToOrderedSet { it.isAnyAdd() }.ifNotEmpty { addAdvice ->
+      rewriter.insertBefore(
+        ctx.stop,
+        addAdvice.joinToString(prefix = "\ndependencies {\n", postfix = "\n}\n", separator = "\n") { a ->
+          printer.toDeclaration(a)
+        }
+      )
+    }


This block looks basically identical to the block above, with the exception that this one uses filterToOrderedSet and the above one is just filterToSet. I think We should extract this to a function and use the ordered set function.

Done in https://github.com/autonomousapps/dependency-analysis-gradle-plugin/pull/1299/commits

autonomousapps · 2024-11-07T17:32:20Z

src/main/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriter.kt

+        } else if (a.isAnyChange()) {
+          val configuration = context.primaryExpression()
+          rewriter.replace(configuration.start, configuration.stop, a.toConfiguration)
+        }


I think this is being too clever. The DependencyDeclaration data class has all the information we need, and we shouldn't try to re-parse the context object to get the configuration context out of it. We should just rewrite the whole line using printer.toDeclaration(a). That ensures there's just one method responsible for printing declarations.

I think the above ☝️ is true, but I will note one caveat that came to mind as I wrote it. There are cases where there are really complex dependency declarations, and what I said assumes that DAGP is correctly modeling a declaration in its totality. Your approach is more conservative by just rewriting the configuration name.

All that said, I prefer keeping the code simple and using printer.toDeclaration(a) to rewrite the whole line. fixDependencies is an optional task and is allowed to make assumptions that buildHealth can't.

. We should just rewrite the whole line using printer.toDeclaration(a). That ensures there's just one method responsible for printing declarations.

Good point. I will update this

👍 I'm happy to update this to printer.toDeclaration(a) for simplicity. I was just following the existing pattern e.g.

dependency-analysis-gradle-plugin/src/main/kotlin/com/autonomousapps/internal/parse/GroovyBuildScriptDependenciesRewriter.kt

Lines 149 to 150 in d36ba8a

val c = ctxDependency.configuration

rewriter.replace(c.start, c.stop, a.toConfiguration)

Note on the test set up

After making the change, the test "update dependencies with dependencyMap" test failed e.g. compileOnly(project(:marvin)) (without "") instead of compileOnly(project(":marvin"))

It's because the AdvicePrinter doesn't print the quote if dependencyMap is presented for the specific advice.

dependency-analysis-gradle-plugin/src/main/kotlin/com/autonomousapps/internal/advice/AdvicePrinter.kt

Lines 58 to 66 in 7c7c915

return if (!mapped.isNullOrBlank()) {

// If the user is mapping, it's bring-your-own-quotes

mapped

} else {

// If there's no map, include quotes

when (dslKind) {

DslKind.KOTLIN -> "\"$gav\""

DslKind.GROOVY -> "'$gav'"

}

To get the test passing, I need to avoid providing dependencyMap for the dependency we want to remain the same. See change

Question

Can we assume that the given dependencyMap from users should always include quotes?

Can we assume that the given dependencyMap from users should always include quotes?

Yes. Good catch and fix!

autonomousapps

LGTM!

autonomousapps · 2024-11-11T19:35:48Z

src/main/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriter.kt

+      if (it.statement.leafRule() !is PostfixUnaryExpressionContext) return@forEach
+
+      val context = it.statement.leafRule() as PostfixUnaryExpressionContext


This could be simplified a bit into a one-liner:

val context = it.statement.leafRule() as? PostfixUnaryExpressionContext ?: return@forEach

autonomousapps · 2024-11-11T19:39:38Z

src/main/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriter.kt

+        } else if (a.isAnyChange()) {
+          val configuration = context.primaryExpression()
+          rewriter.replace(configuration.start, configuration.stop, a.toConfiguration)
+        }


Can we assume that the given dependencyMap from users should always include quotes?

Yes. Good catch and fix!

autonomousapps · 2024-11-11T19:49:40Z

The :buildHealth failure is interesting, and I'll have to think about how to resolve it. The existing antlr support for Groovy deliberately shades antlr to work around common problems with people bringing in different versions of that tool and breaking this plugin. However, now we have KotlinEditor also bringing in antlr. ~~Maybe the most straightforward answer is to shade KotlinEditor as well. Could maybe do it with the extant shadowed/antlr project.~~ 🤔 No, just checked again and that doesn't make sense. I could add a new module just for a shaded version of KotlinEditor. Or I could make the more substantial effort to add Shadow directly to the main module (but this doesn't excite me as a great idea). Or we could even add a shaded version of antlr directly to KotlinEditor -- but I think that's really a user concern and not the responsibility of KotlinEditor.

wsutina changed the title ~~🚧 Refactor fixDependencies task to use grammar from KotlinEditor for Kotlin Gradle DSL~~ 🚧 Updat Kotlin DSL rewriting functionality to use KotlinEditor Nov 1, 2024

wsutina changed the title ~~🚧 Updat Kotlin DSL rewriting functionality to use KotlinEditor~~ 🚧 Update Kotlin DSL rewriting functionality to use KotlinEditor Nov 1, 2024

autonomousapps added the enhancement New feature or request label Nov 1, 2024

autonomousapps self-requested a review November 1, 2024 17:42

autonomousapps added this to the next milestone Nov 1, 2024

autonomousapps reviewed Nov 1, 2024

View reviewed changes

gradle/libs.versions.toml Outdated Show resolved Hide resolved

autonomousapps reviewed Nov 1, 2024

View reviewed changes

wsutina force-pushed the swipawiwat/kotlin-rewriter branch 2 times, most recently from 529e11a to e71d021 Compare November 6, 2024 07:33

wsutina commented Nov 6, 2024

View reviewed changes

src/test/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriterTest.kt Show resolved Hide resolved

wsutina force-pushed the swipawiwat/kotlin-rewriter branch 3 times, most recently from 9327c2b to 8778ec0 Compare November 7, 2024 04:57

wsutina changed the title ~~🚧 Update Kotlin DSL rewriting functionality to use KotlinEditor~~ Update Kotlin DSL rewriting functionality to use KotlinEditor Nov 7, 2024

wsutina force-pushed the swipawiwat/kotlin-rewriter branch from 8778ec0 to 510c512 Compare November 7, 2024 05:11

wsutina marked this pull request as ready for review November 7, 2024 05:14

wsutina force-pushed the swipawiwat/kotlin-rewriter branch from 510c512 to b17b6f3 Compare November 7, 2024 05:18

wsutina commented Nov 7, 2024

View reviewed changes

src/test/kotlin/com/autonomousapps/internal/parse/KotlinBuildScriptDependenciesRewriterTest.kt Show resolved Hide resolved

autonomousapps requested changes Nov 7, 2024

View reviewed changes

wsutina force-pushed the swipawiwat/kotlin-rewriter branch 2 times, most recently from c358cd6 to cf85b18 Compare November 10, 2024 23:31

autonomousapps approved these changes Nov 11, 2024

View reviewed changes

wsutina and others added 7 commits November 11, 2024 15:04

feat: use KotlinEditor to rewrite Gradle Kotlin DSL scripts.

ef17744

build: create new kotlin-editor-relocated project.

b8620e0

Prepare for kotlin-editor-relocated release 0.10.0-SNAPSHOT.

e941c0d

refactor: update source to use newly relocated packages.

07e7b18

Prepare for kotlin-editor-relocated release 0.10.0.

9454d29

Prepare next development version.

5fc438f

chore: use kotlin-editor-relocated release.

f1f9c94

autonomousapps force-pushed the swipawiwat/kotlin-rewriter branch from 2cc86a8 to f1f9c94 Compare November 11, 2024 23:05

autonomousapps merged commit f1f9c94 into autonomousapps:main Nov 11, 2024

	val c = ctxDependency.configuration
	rewriter.replace(c.start, c.stop, a.toConfiguration)

	return if (!mapped.isNullOrBlank()) {
	// If the user is mapping, it's bring-your-own-quotes
	mapped
	} else {
	// If there's no map, include quotes
	when (dslKind) {
	DslKind.KOTLIN -> "\"$gav\""
	DslKind.GROOVY -> "'$gav'"
	}

		if (it.statement.leafRule() !is PostfixUnaryExpressionContext) return@forEach

		val context = it.statement.leafRule() as PostfixUnaryExpressionContext

Uh oh!

Update Kotlin DSL rewriting functionality to use KotlinEditor #1299

Update Kotlin DSL rewriting functionality to use KotlinEditor #1299

Uh oh!

Conversation

wsutina commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description

Uh oh!

Uh oh!

autonomousapps left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

autonomousapps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Note on the test set up

Question

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autonomousapps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autonomousapps commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wsutina commented Nov 1, 2024 •

edited

Loading

autonomousapps commented Nov 11, 2024 •

edited

Loading