-
Notifications
You must be signed in to change notification settings - Fork 6k
Upstream AndroidX KlibDumpParser #5470
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
base: master
Are you sure you want to change the base?
Conversation
Add KlibDumpParser from AndroidX repository into util-klib-abi
Thanks @ivandev0, that sounds good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for a delay with the review. And thank you for contributing the parser, I really appreciate it!
There are a few implementation specific questions I have, but the major one is about separating parsing a merged dump file produced by BCV and parsing a dump for a single klib-target emitted by this module.
Otherwise, it looks good, nice job!
import java.io.File | ||
import kotlin.io.path.Path | ||
|
||
fun getJavaResource(fileName: String): File { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be unused
qualifiedName = abiQualifiedName, | ||
signatures = currentSignatures(), | ||
annotations = AbiAnnotationListImpl.EMPTY, // annotations aren't part of klib dumps | ||
isInline = false, // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify what's left here (I mean, the reason for leaving the // TODO
)?
var signatureVersions: MutableSet<AbiSignatureVersion> = mutableSetOf(), | ||
) | ||
|
||
class KlibDumpParser(klibDump: String, private val fileName: String? = null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a KDoc for public API describing why and how one can use it?
It's also worth describing what parameters should contain and how to interpret them.
You can check LibraryAbi*
KDoc from the neighboring package for an example.
*/ | ||
private val abiInfoByTarget = mutableMapOf<String, MutableAbiInfo>() | ||
|
||
/** Parse the klib dump tracked by [cursor] into a map of targets to [LibraryAbi]s */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cursor
is private and it's an implementation detail, so I don't think it should be referenced here.
What's worth including in KDoc:
- what keys the returned map may contain, what do they correspond to;
- are there any exception this function might throw.
compilerVersion = "", | ||
abiVersion = "", | ||
irProviderName = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these properties have a null value instead?
|
||
@Test | ||
fun parseAFunctionWithSingleContextValue() { | ||
val input = "final fun context(kotlin/Int) my.lib/bar()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the way context parameters are rendered has changed to
final fun (context(kotlin/Int)).my.lib/bar()
) | ||
} | ||
|
||
private fun parseAbiQualifiedName(parentQualifiedName: AbiQualifiedName?): AbiQualifiedName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not work for top-level functions belonging to a root package (there's no parent qualified name and parseAbiQualifiedName
returns null
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And there are problems with other top-level declaration residing in a root package too.
private val classModifierRegex = Regex("^(inner|value|fun|open)") | ||
private val functionKindRegex = Regex("^(constructor|fun)") | ||
private val functionModifierRegex = Regex("^(inline|suspend)") | ||
private val packageNameRegex = Regex("^[a-zA-Z0-9.]+") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to backticks, the name could be virtually anything, but from the dump parsing perspective it would be safe to assume it's anything but /
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For symbol names, things seem to be worse, because actual terminator depends on a context ((
for function, {
for classes). But you already have a parseValidIdentifierAndMaybeTrim
to deal with the naming mess, so maybe it could be modified to handle all supported names too.
import org.junit.jupiter.api.Assertions.assertThrows | ||
import kotlin.toString | ||
|
||
class KlibDumpParserTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be also great to modify GenerateLibraryAbiReaderTests.kt
to generate tests that'll parse dumps generated for LibraryAbi
tests.
I was using them (kotlin/compiler/testData/klib/dump-abi/content
) to catch a few issues mentioned in the review, so its definitely worth including them as part of regular test runs.
private val abiInfoByTarget = mutableMapOf<String, MutableAbiInfo>() | ||
|
||
/** Parse the klib dump tracked by [cursor] into a map of targets to [LibraryAbi]s */ | ||
fun parse(): Map<String, LibraryAbi> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a conceptual problem here, let me explain.
When it comes to serializing KLIB ABI dumps, utli-klib-abi
module is only responsible for generating dumps for a particular target. It's BCV who merges multiple dumps together and produce a single dump with // Targets:
annotations around some declarations.
So when it comes to parsing dumps back, it seems like it should be done in reverse order: BCV should split the merged file back into several individuals dumps and KlibDumpParser
should work with resulting single-target dumps.
Besides ephemeral elegance, there are practical reasons to do so:
- currently,
KlibDumpParser
assumes a specific merged dump format, that is maintained elsewhere, so it might be hard to keep everything in sync. String
keys here are in fact KLibTarget from BCV, but there's no way to refer it from here- BCV is already capable of reading a merged dump, providing information about targets and filtering out content corresponding to a specific target, so this work could be already delegated to it.
WDYT about separating these two parsing concerns and keeping only single-target dump parsing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that totally makes sense. I'll start reworking it.
Thanks for the review! All those comments make sense, I'll take a look shortly. |
Add KlibDumpParser from AndroidX repository into util-klib-abi
As discussed with @fzhinkin, this is a slightly edited version of the KlibDumpParser from our repo. Most changes are in the tests to remove explicit references to androidx classes as well as some tests which parsed full dumps from androidx libraries because I wasn't sure that was appropriate to include. I left the original copyright but of course changed packages.
It's a large PR, I considered trying to break this up into pieces but I figured I'd start here and break it up if necessary.
Looking forward to your feedback, thanks!