Skip to content

Conversation

@MrHell228
Copy link
Contributor

No description provided.

@aromaa
Copy link
Member

aromaa commented Oct 6, 2025

We should redirect it to the vanilla one so we don't accidentally return different results from different APIs.

@MrHell228
Copy link
Contributor Author

What is someone implements Key? Should it use -o.compareTo((Key) this) or convert given Key to ResourceLocation?

@MrHell228
Copy link
Contributor Author

Moreover, vanilla comparison doesn't look reachable from API, currently it will always use adventure comparator

@aromaa
Copy link
Member

aromaa commented Oct 6, 2025

What is someone implements Key?

Thinking about this, I think we want to defer vanilla calls to adventure to be consistent.

Moreover, vanilla comparison doesn't look reachable from API

The ResourceLocation#compareTo is reachable when casted to Comparable. The Key#compareTo is only reachable when explicitly called by user code.

@MrHell228
Copy link
Contributor Author

MrHell228 commented Oct 6, 2025

Thinking about this, I think we want to defer vanilla calls to adventure to be consistent.

With @Overwrite?

@aromaa
Copy link
Member

aromaa commented Oct 6, 2025

That should be fine, yes.

Comment on lines 54 to 55
public int compareTo(final ResourceLocation location) {
return ((Key) this).compareTo((Key) (Object) location);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public int compareTo(final ResourceLocation location) {
return ((Key) this).compareTo((Key) (Object) location);
public int compareTo(final Object obj) {
return ((Key) this).compareTo((Key) obj);

After looking at the implementations of ResourceLocation#compareTo(ResourceLocation) and Key#compareTo(Key), they seem completely equivalent to me. I don't think we need to overwrite one or the other. What I'm more worried about is the synthetic method ResourceLocation#compareTo(Object) which is casting and delegating to ResourceLocation#compareTo(ResourceLocation). I think this one should be overwritten to delegate to Key#compareTo(Key) instead.

Copy link
Contributor Author

@MrHell228 MrHell228 Oct 23, 2025

Choose a reason for hiding this comment

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

Makes sense. Didn't know it works via synthetic method. I guess now ResourceLocation#compareTo(ResourceLocation) should not be reachable from the API and no CCE should be thrown if anyone implements Key on their own (or if KeyMixin is removed which would result in Adventure KeyImpl being used).

Copy link
Member

@aromaa aromaa left a comment

Choose a reason for hiding this comment

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

Thank you!

@aromaa aromaa merged commit a6f8537 into SpongePowered:api-14 Oct 25, 2025
7 checks passed
@MrHell228 MrHell228 deleted the api-14-fix-key-comparing branch October 25, 2025 18:26
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.

3 participants