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

Don't drop Record attribute for records with no components #118

Merged

Conversation

Gegy
Copy link
Contributor

@Gegy Gegy commented Aug 8, 2023

Although they are not strictly required by the JVM specification, Record attributes are usually preserved by Proguard. When determining which attributes are used, NonEmptyAttributeFilter is chained with AttributeUsageMarker in order to mark all non-empty attributes as used. This includes Record attributes: however, they are additionally filtered based on the number of components of the record. This means that any record with no components will not have its Record attribute marked as used, and it will be dropped.

This is a fairly low-impact issue; it seems inconsistent and unexpected, though I can see that it may also be intentional.

Related issue: MC-264564.

Thanks!

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@rubenpieters
Copy link
Contributor

Hi @Gegy , thanks for sharing this related issue. We haven't heard of any similar reports like this. Can you provide a scenario or show an example of a case where it is problematic that these empty Record attributes are dropped?
You mention in the linked issue that it is needed for both the JDK's libraries and modding utilities , can you show an example of what you are trying to do and how it is resulting in issues?

@Gegy
Copy link
Contributor Author

Gegy commented Aug 8, 2023

Hi! This primarily affects modding toolchains, particularly where these toolchains are built around applying patches to decompiled source and recompiling from that patched source. A non-record class that extends the Record type is not valid, and the generated indy instructions are generally not handled very well by decompilers without the attribute.

This is of course however an issue that can be resolved rather trivially in modding toolchains (e.g. this PR which adds the attribute back based on the superclass) - so I can see that this might not be considered an issue worth addressing here. (I don't imagine making decompilers happy to be a goal of Proguard 😄) It mostly seems inconsistent, as these record attributes are generally not strictly required anyway.

I have also set up a simple example repository demonstrating this case:

public record EmptyRecord() {
}

public record NormalRecord(String foo) {
}

After obfuscation, these decompile with FernFlower to:

// Missing Record attribute, decompiles as a normal class that extends Record
public final class a extends Record {
    public a() {
    }

    public final String toString() {
        return this.toString<invokedynamic>(this);
    }

    public final int hashCode() {
        return this.hashCode<invokedynamic>(this);
    }

    public final boolean equals(Object o) {
        return this.equals<invokedynamic>(this, o);
    }
}

// Record attribute is preserved with 1 or more components
public record b(String a) {
    public b(String foo) {
        this.a = foo;
    }

    public String a() {
        return this.a;
    }
}

@rubenpieters
Copy link
Contributor

rubenpieters commented Aug 8, 2023

Thanks for the explanation.

It mostly seems inconsistent, as these record attributes are generally not strictly required anyway.

Indeed. Note that if you disable the dontshrink and -keepattributes '*' options that you have enabled in your config, then all Record attributes are removed from the class files in a consistent manner, which is the default behavior for ProGuard.
Given this, it does make sense that empty record attributes are kept if these attributes are requested to be kept. Since an empty record attribute still indicates different information than no record attribute at all.

@rubenpieters rubenpieters merged commit 4ad4114 into Guardsquare:master Aug 9, 2023
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.

None yet

2 participants