Skip to content

Add descriptor for field scheme inside ClassInfo - #9

Open
danirod12 wants to merge 4 commits into
neoforged:mainfrom
danirod12:add-field-desc
Open

Add descriptor for field scheme inside ClassInfo#9
danirod12 wants to merge 4 commits into
neoforged:mainfrom
danirod12:add-field-desc

Conversation

@danirod12

@danirod12 danirod12 commented May 10, 2024

Copy link
Copy Markdown
Contributor

Byte-code allow you to have same field names if descriptors are different. Remapper was crashing if so happened.

File for testing: ObfProblem-1.0-SNAPSHOT.zip

Caused by: java.lang.IllegalStateException: Duplicate key a (attempted merging values private final org/example/obfproblem/ObfProblem/a I and private final org/example/obfproblem/ObfProblem/a Ljava/lang/String;)
        at java.base/java.util.stream.Collectors.duplicateKeyException(Collectors.java:135)
        at java.base/java.util.stream.Collectors.lambda$uniqKeysMapAccumulator$1(Collectors.java:182)
        at java.base/java.util.stream.ReduceOps$3ReducingSink.accept(ReduceOps.java:169)
        at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
        at net.neoforged.art.internal.ClassProviderImpl$ClassInfo.<init>(ClassProviderImpl.java:122)

Byte-code allow you to have same field names if descriptors are different. Remapper wes crashing if so happened
@shartte

shartte commented May 10, 2024

Copy link
Copy Markdown
Contributor

Why does this need to be supported if Mojang doesn't actually have this case within their obfuscated code?

@danirod12

Copy link
Copy Markdown
Contributor Author

PaperMC team uses this project for remapping plugins on runtime to have mojang mappings on runtime since MC 1.20.5/6, more info:

https://forums.papermc.io/threads/important-dev-psa-future-removal-of-cb-package-relocation.1106/

Some plugins have obfuscation with same field names and different descriptors, so all fails. To see example, please, check mentioned issue above

@shartte

shartte commented May 11, 2024

Copy link
Copy Markdown
Contributor

PaperMC team uses this project for remapping plugins on runtime to have mojang mappings on runtime since MC 1.20.5/6, more info:

https://forums.papermc.io/threads/important-dev-psa-future-removal-of-cb-package-relocation.1106/

Some plugins have obfuscation with same field names and different descriptors, so all fails. To see example, please, check mentioned issue above

Okay, wait. You mean to tell me that some paper plugins obfuscate... their own code (?)
And we should cater to that?

@danirod12

Copy link
Copy Markdown
Contributor Author

Basically, yes. It is a common practice for large resources like anti-cheats and I am talking not only about premium resources.
The Mojang itself may add such obfuscation if you are relying on them, so why not to make the lib universal?

@Matyrobbrt Matyrobbrt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You also missed the spot where class infos are created from runtime Classes.

Comment thread src/main/java/net/neoforged/art/internal/ClassProviderImpl.java Outdated
Comment thread src/main/java/net/neoforged/art/internal/ClassProviderImpl.java Outdated
Comment thread src/main/java/net/neoforged/art/internal/ClassProviderImpl.java Outdated
Comment thread src/main/java/net/neoforged/art/api/ClassProvider.java Outdated
danirod12 added 2 commits May 11, 2024 23:34
Added raw storage "fieldName -> info" to keep performance
Previous separator space changed to dot separator
Changed runtime class fields retrieving also
@danirod12
danirod12 requested a review from Matyrobbrt May 11, 2024 20:44
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.

4 participants