Skip to content
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

Restore ABI stability for annotatable and documentable builders #1580

Merged
merged 3 commits into from
May 29, 2023

Conversation

ZacSweers
Copy link
Collaborator

Resolves #1579

Originally tried to be clever by using value classes to get the impl without the overhead, and it almost worked except that the resulting bytecode still tried to cast the underlying apply to the value class instead of the enclosing builder it was inlined into. Fell back to a more manual approach of just adding explicit overrides that just called super.

@ZacSweers ZacSweers requested review from JakeWharton and Egorand May 29, 2023 14:21
Copy link
Collaborator

@Egorand Egorand left a comment

Choose a reason for hiding this comment

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

Hmm, really confused why removing these redundant overrides breaks binary compatibility..

@JakeWharton
Copy link
Collaborator

Because the generic return type T winds up in bytecode as its lower bound which is the interface Builder rather than the concrete spec Builder.

Also the binary compatibility validator seems to be a complete garbage tool and doesn't fail the build when you do this. Do we need to switch to metalava or japicmp, or does it just have terrible default behavior?

@ZacSweers ZacSweers merged commit 8b2c0fb into master May 29, 2023
@ZacSweers ZacSweers deleted the z/abiFixes branch May 29, 2023 14:37
@ZacSweers
Copy link
Collaborator Author

I think it focuses only on being a change detector and relies on code review to decide if the change is ok. It doesn't do what japicmp does as far as "is this compatible with a previous release" kind of checks, but it's also more accurate in some ways. Maybe there's a mode like that that I'm missing though

@ZacSweers
Copy link
Collaborator Author

Filed a FR here: Kotlin/binary-compatibility-validator#140

@Egorand
Copy link
Collaborator

Egorand commented May 30, 2023

Should we set up japicmp in the meantime?

@ZacSweers
Copy link
Collaborator Author

Seems like a good idea

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.

1.14.0 has breaking ABI changes to documentable and annotatable builders
3 participants