Skip to content

Kotlin: stub trap .class files when extracting a class from Kotlin source #11510

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

Merged

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Nov 30, 2022

This will be useful for annotations, because other extractors are less likely to see a binary version of the class stripped of its source-only annotations and introduce an inconsistency.

@smowton smowton requested a review from a team as a code owner November 30, 2022 18:39
@smowton smowton force-pushed the smowton/fix/kotlin-populate-source-class-files branch 2 times, most recently from ff2d27b to 6c79985 Compare December 2, 2022 20:06
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

Added some minor comments/questions.

Only top-level non-class declarations need the IrFile's expected class name inserting
These resulted from the Java compiler exploring NotNull and other Kotlin-emitted annotations, which it no longer does because it finds a .class trap file already present and truncates its class-graph walk
These occur because the Companion field is odd, being extracted from source but not having an associated FieldDeclaration, leading to PrintAst enumerating the node differently depending on whether it has a source-file location or not but in either case choosing not to show it.
This was testing that a signature inconsistency occurs, but this now manifests as a db inconsistency which can't be used as a test expectation because specific tuple numbers are liable to change with the environment.
…a signature

We could revert this by allowing useType to avoid triggering class-instance extraction when used just for its signature result
This is actually a bug, which we should follow up on subsequently.
@smowton smowton force-pushed the smowton/fix/kotlin-populate-source-class-files branch from 6c79985 to fc3ed22 Compare December 6, 2022 18:38
@smowton smowton requested a review from a team as a code owner December 6, 2022 18:38
@github-actions github-actions bot added the Java label Dec 6, 2022
@smowton smowton force-pushed the smowton/fix/kotlin-populate-source-class-files branch from fc3ed22 to 00f323c Compare December 6, 2022 20:32
@smowton
Copy link
Contributor Author

smowton commented Dec 6, 2022

@tamasvajk all comments addressed, and test changes accepted -- two notable changes that were needed:

  1. Disabled the suspend function test that would now result in an inconsistency that can't easily be recorded as a test expectation due to brittleness (9f722a7)
  2. Fixed a bug where directly exposed fields (@JvmField et al) didn't get a static modifier, leading to missing dataflow (00f323c)

Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

Looks okay to me apart from some CI test failures. I also added a minor comment.

@smowton
Copy link
Contributor Author

smowton commented Dec 7, 2022

@tamasvajk done

@smowton smowton merged commit 9f9a516 into github:main Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants