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

Make _flower series of decompilers coexist #871

Conversation

meiMingle
Copy link
Contributor

As the title says, this is to implement #866. This PR refers to the method in class-file-indexer to relocate the package path of the _flower series decompiler to achieve the purpose of coexistence without interfering with each other.

@meiMingle meiMingle marked this pull request as draft October 29, 2024 17:17
…ompleted the related configuration functions
@meiMingle meiMingle marked this pull request as ready for review October 31, 2024 18:35
@@ -38,7 +38,8 @@ dependencies {
api(libs.openrewrite)
api(libs.regex)
api(libs.bundles.jasm)
api(libs.vineflower)
// api(libs.vineflower)
api( project(':recaf-relocation'))
Copy link
Owner

Choose a reason for hiding this comment

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

I know that you put in a fair amount of effort to get these decompilers to all work with one another, but I am rather against having such things like this relocation module in the main repository.

Additionally, the amount of differences between these decompilers is rather small since they are all forked off one-another.

IMO these alternative flowers should be offered as plugins which are independently shading their respective flower code into the plugin jars. Decompiler implementations and config containers can be registed on the fly so there's no concern with lessened capability by having them offered separately.

Plus, given some of our user interactions I have a strong feeling that having nearly the same decompiler three times would be confusing to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, the amount of differences between these decompilers is rather small since they are all forked off one-another.

Although I know that they are forked from each other, I cannot agree that their differences are small. In fact, after years of evolution, they are now very different in terms of interface form, number of controllable parameters, and compilation results.
In terms of the number of parameters, vineflower: 64, fernflower: 45, forgeflower: 47.
The following three figures are the decompilation results of the three flower decompilers for software.coley.instrument.io.ByteBufferSanitizer in %APPDATA%/Recaf/agent/agent-1.4.2.jar in the Recaf running directory. The source code of the corresponding class is located in ByteBufferSanitizer. Among them, only fernflower obtained the equivalent result to the source code.
Vineflower
Fernflower
Forgeflower

When decompiling some internal enumeration classes, vineflower reports an error, while fernflower and forgeflower can get the decompiled results. Below are the decompiled results of the three flower class decompilers for jadx.api.JadxArgs$RenameEnum in jadx-core-1.5.0.jar

Vineflower-1
Fernflower-1
Forgeflower-1

IMO these alternative flowers should be offered as plugins which are independently shading their respective flower code into the plugin jars. Decompiler implementations and config containers can be registed on the fly so there's no concern with lessened capability by having them offered separately.

Maybe this is one of the possible solutions, but I don't know how to create a plug-in for Recaf yet. Is there any sample for reference?

Plus, given some of our user interactions I have a strong feeling that having nearly the same decompiler three times would be confusing to them.

For ordinary users, they mostly use the default CFR decompiler and use the default configuration. For professional users, they should know clearly which compiler they want to use and what configuration parameters they want to use. Maybe I need to further optimize the configuration name to make it more clearly match the parameter name in the corresponding decompiler official documentation.

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

Attention: Patch coverage is 79.13043% with 96 lines in your changes missing coverage. Please review.

Project coverage is 64.91%. Comparing base (e6b5b7f) to head (5dc5478).

Files with missing lines Patch % Lines
...ces/decompile/fernflower/FernflowerDecompiler.java 18.18% 18 Missing ⚠️
...vices/decompile/forgeflower/ForgeflowerConfig.java 85.08% 16 Missing and 1 partial ⚠️
...s/decompile/forgeflower/ForgeflowerDecompiler.java 19.04% 17 Missing ⚠️
...ervices/decompile/fernflower/FernflowerConfig.java 86.11% 14 Missing and 1 partial ⚠️
...ervices/decompile/vineflower/VineflowerConfig.java 94.15% 9 Missing ⚠️
...ervices/decompile/fernflower/FernflowerLogger.java 33.33% 8 Missing ⚠️
...vices/decompile/forgeflower/ForgeflowerLogger.java 33.33% 8 Missing ⚠️
...ces/decompile/vineflower/VineflowerDecompiler.java 76.47% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #871      +/-   ##
============================================
+ Coverage     64.54%   64.91%   +0.37%     
- Complexity     2984     2994      +10     
============================================
  Files           368      374       +6     
  Lines         15610    15892     +282     
  Branches       2138     2160      +22     
============================================
+ Hits          10076    10317     +241     
- Misses         4493     4529      +36     
- Partials       1041     1046       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@meiMingle
Copy link
Contributor Author

Following @Col-E 's suggestion, this PR will be provided as a separate plugin fernflower-recaf-plugin Currently this plugin is initially available, but there are still some issues that need to be resolved.

@meiMingle meiMingle closed this Nov 18, 2024
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.

3 participants