Skip to content

Implement Resolver.getModuleName API#1649

Closed
ZacSweers wants to merge 13 commits into
google:mainfrom
ZacSweers:z/moduleName
Closed

Implement Resolver.getModuleName API#1649
ZacSweers wants to merge 13 commits into
google:mainfrom
ZacSweers:z/moduleName

Conversation

@ZacSweers

Copy link
Copy Markdown
Contributor

Resolves #1621

@ting-yuan

Copy link
Copy Markdown
Contributor

Could you run ./gradlew updateApi to update the api.base?

@ting-yuan

Copy link
Copy Markdown
Contributor

BTW, although this is a simple API, it'd be nice to have tests. For example, you may copy this and add a check after adding a logger.warn(moduleName) in the processor.

@ZacSweers

Copy link
Copy Markdown
Contributor Author

I added a test but was surprised to find that K1 and K2 have different behavior here 😰. The APIs appear to be the same in docs, but K1 uses the project module name and K2 appears to use the source set name. Lmk what you want to do, or if you're fine with the current difference and can explore more later

@ZacSweers

Copy link
Copy Markdown
Contributor Author

Weirder still, this test worked for me locally at first, and now... doesn't appear to run the processor at all? 🫠

@neetopia

neetopia commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

I added a test but was surprised to find that K1 and K2 have different behavior here 😰. The APIs appear to be the same in docs, but K1 uses the project module name and K2 appears to use the source set name. Lmk what you want to do, or if you're fine with the current difference and can explore more later

Inconsistent behavior is expected between K1 and K2, it is better to confirm with JB on this inconsistency to see if that's the expected behavior from their side, we are OK with inconsistent behavior if it is expected.

val gradleRunner = GradleRunner.create().withProjectDir(project.root)
gradleRunner.withArguments("build").build().let { result ->
// TODO figure out why K2 uses the source set name and K1 uses the project name
val expectedName = if (useKSP2) "workload" else "main"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the comment, should this be in the reversed order?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ting-yuan

Copy link
Copy Markdown
Contributor

Is the difference also observed in the mangled jvm names in K1 and K2? If so, for the purpose of the issue referenced, they have to be different in KSP too. Otherwise, either the KSP1 or KSP2 changes need to be adjusted.

By the way, don't Resolver.getJvmName or Resolver.mapToJvmSignature work for you?

@ZacSweers

Copy link
Copy Markdown
Contributor Author

Apologies, I completely forgot about this. Asking for clarification on the K2 behavior change in kotlin-lang slack here: https://kotlinlang.slack.com/archives/C03PK0PE257/p1709960362349149

By the way, don't Resolver.getJvmName or Resolver.mapToJvmSignature work for you?

Could you elaborate on this? I wasn't sure what you're referring to but maybe it's because I just haven't looked at this in a minute

@ZacSweers

Copy link
Copy Markdown
Contributor Author

I'm fairly convinced at this point that KtSourceModule.stableModuleName/KtSourceModule.moduleName are just plain broken in K2, and escalating this issue: https://youtrack.jetbrains.com/issue/KT-66713

@ZacSweers

Copy link
Copy Markdown
Contributor Author

@ting-yuan after feedback on the above issue, I think I've resolved this 🥳

@ting-yuan

Copy link
Copy Markdown
Contributor

Apologies, I completely forgot about this. Asking for clarification on the K2 behavior change in kotlin-lang slack here: https://kotlinlang.slack.com/archives/C03PK0PE257/p1709960362349149

By the way, don't Resolver.getJvmName or Resolver.mapToJvmSignature work for you?

Could you elaborate on this? I wasn't sure what you're referring to but maybe it's because I just haven't looked at this in a minute

I mean, should Resolver.getJvmName or Resolver.mapToJvmSignature just fit your use case? Given a KSDeclaration, they should return the mangled names in JVM.

@ZacSweers

Copy link
Copy Markdown
Contributor Author

It's possible, but there are so many added permutations for handling custom getters, is prefixes, and handling value classes that it's easier/more flexible to just have access to the module name directly

@ZacSweers

Copy link
Copy Markdown
Contributor Author

@ting-yuan @neetopia anything else you need from me for this?

@ting-yuan ting-yuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the patch! Would you mind to rebase it to ToT so that it can be cherry-picked into release branches by CI?

@ZacSweers

Copy link
Copy Markdown
Contributor Author

Rebase or is merging latest main ok?

@ZacSweers

Copy link
Copy Markdown
Contributor Author

@ting-yuan done but let me know if you'd prefer a full rebase. I try to avoid it after having done main merges due to code review and also git's propensity for uh, not handling mixing them well 😅

@ting-yuan

Copy link
Copy Markdown
Contributor

I'd really appreciate a full rebase. Our auto-cherry-picker doesn't work with merge very well 😢 Don't worry about the review. I'll go through it quickly again before merging.

@ZacSweers

Copy link
Copy Markdown
Contributor Author

I'm just going to open a fresh PR with the changes then

@ZacSweers

Copy link
Copy Markdown
Contributor Author

#1847

@ZacSweers ZacSweers closed this Apr 17, 2024
@ZacSweers ZacSweers deleted the z/moduleName branch April 17, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: expose an API in Resolver to retrieve the module name for handling mangling of internal members

4 participants