Skip to content

Do not generate visibility bridge for package-private default methods (fixes #1118) #1366

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 9, 2025
Merged

Do not generate visibility bridge for package-private default methods (fixes #1118) #1366

merged 3 commits into from
Apr 9, 2025

Conversation

sgerke-1L
Copy link
Contributor

@sgerke-1L sgerke-1L commented Mar 27, 2025 •
edited
Loading

Minimal reproducer for issue #1118 with fix.

The root problem is that the JVM doesn't generate a visibility bridge for package-private default methods, but byte-buddy does. This was discussed in raphw/byte-buddy#1795.

It might be that we can use decorate instead of redefine to side-step the problem, but I'm not experienced enough with mockk to judge if this can work.

The previous approach of not generating the bridge methods by deselecting them with a custom visibility bridge strategy can be found in the commit history and might be an alternative if decorate is not viable.

@sgerke-1L sgerke-1L changed the title Minimal reproducer for issue #1118 Do not generate visibility bridge for package-private default methods (fixes #1118) Mar 31, 2025
@sgerke-1L sgerke-1L mentioned this pull request Mar 31, 2025
Closed
3 tasks
@marcelstoer
Copy link
Contributor

marcelstoer commented Apr 7, 2025 •
edited
Loading

@sgerke-1L thanks for your analysis!

By comparing the Javadocs for the two methods I concluded that the existing redefine() is what we likely want.

Decorate Redefine
Decorates a type with net.bytebuddy.asm.AsmVisitorWrapper and allows adding attributes and annotations. A decoration does not allow for any standard transformations but can be used as a performance optimization compared to a redefinition, especially when implementing a Java agent that only applies ASM-based code changes.
Important: Only use this mode to improve performance in a narrowly defined transformation. Using other features as those mentioned might result in an unexpected outcome of the transformation or error. Using decoration also requires the configuration of an Implementation.Context.Factory that does not attempt any type transformation.		 Redefines the given type where any intercepted method that is declared by the redefined type is fully replaced by the new implementation.
Note: When a user redefines a class with the purpose of reloading this class using a net.bytebuddy.dynamic.loading.ClassReloadingStrategy, it is important that no fields or methods are added to the redefined class. Note that some Implementations implicitly add fields or methods. Finally, Byte Buddy might be forced to add a method if a redefined class already defines a class initializer. This can be disabled by setting with(Implementation.Context.Factory) to use a Implementation.Context.Disabled.Factory where the class initializer is retained as is.
Source: https://github.com/raphw/byte-buddy/blob/master/byte-buddy-dep/src/main/java/net/bytebuddy/ByteBuddy.java#L1110 Source: https://github.com/raphw/byte-buddy/blob/master/byte-buddy-dep/src/main/java/net/bytebuddy/ByteBuddy.java#L897

@Raibaz can we ask you to approve the workflow run to get full test coverage, thanks.

@Raibaz
Copy link
Collaborator

Raibaz commented Apr 7, 2025

Sure, and thanks a lot to both of you for the in-depth analysis!

@marcelstoer marcelstoer mentioned this pull request Apr 7, 2025
Closed
@marcelstoer
Copy link
Contributor

marcelstoer commented Apr 8, 2025 •
edited
Loading

After revisiting raphw/byte-buddy#1795 I changed my mind. True, the Byte Buddy Javadoc does warn against the use of decorate() except for a limited set of use cases. Ours, while not for performance reasons, seems to fall into exactly this category as we only use the decorator for the visitor pattern.

I'd be happy to see this land and released soon.

@sgerke-1L
Copy link
Contributor Author

Thanks for your support @marcelstoer. I still can't judge the full consequences of using decorate over redefine (as I don't know all details of how mockk is interacting with the decorated/redefined class), but I also think it might be worth a try. If there are examples in the wild that this change breaks, then there will be bug reports and new test-cases to be added.

@Raibaz
Copy link
Collaborator

Raibaz commented Apr 8, 2025

I definitely agree with @sgerke-1L.

@sgerke-1L
Copy link
Contributor Author

Btw, is it possible to run the android tests as well or would that have to be done locally?

@Raibaz
Copy link
Collaborator

Raibaz commented Apr 8, 2025

The instrumented tests for api level 30 have been run, I see that as the first check in the list.

@Raibaz Raibaz merged commit 9d1b6d6 into mockk:master Apr 9, 2025
22 checks passed
@sgerke-1L sgerke-1L mentioned this pull request Apr 11, 2025
Merged
2 tasks
@marcelstoer marcelstoer mentioned this pull request Jun 24, 2025
Closed
@renovate renovate bot mentioned this pull request Jul 24, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sgerke-1L @marcelstoer @Raibaz