8000 Fix SignChangeEvent NPE (& external list mutability) by SoSeDiK · Pull Request #12423 · PaperMC/Paper · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix SignChangeEvent NPE (& external list mutability) #12423

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.

Alrea 8000 dy on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix SignChangeEvent NPE (& external list mutability) #12423

wants to merge 3 commits into from
+44 −30

Conversation

SoSeDiK
Copy link
Contributor
@SoSeDiK SoSeDiK commented Apr 13, 2025 •
edited
Loading
  • Updated SignChangeEvent class
    • Fix NPE on getLine() / getLines() / legacy String[] constructor
    • Updated the annotations to JSpecify, removed Paper comments and fully qualified imports as it's quite a small class & change
    • getLine() / line() / constructor now correctly show nullability
      • To clarify, this PR does not break the null contract, but simply adds a missing nullability annotations for those
    • Change lines handling to not allow external modifications (otherwise iffy things are possible like passing an immutable list, changing the amount of elements) and mandate 4 elements. Adds a method to set all the lines at once as an alternative.
      • That's changing behavior (updating the list won't affect event anymore), but I personally wasn't expecting this in the first place (it's not m 8000 entioned in javadoc, and differs from other similar places within API), and it's not a major break. The legacy getter didn't allow mutability already.

I'm sorry this grew a liiiiiiiiiitle bigger than initially planned as changes just snowballed 🫠
Can be easily split if any of the changes are debatable.
Edit: split into #12462

@SoSeDiK SoSeDiK requested a review from a team as a code owner April 13, 2025 10:03
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Apr 13, 2025
@AnttiMK
Copy link
AnttiMK commented Apr 13, 2025 •
edited
Loading

MM did a pull request on this way back which was never merged (#9468). That PR used empty components and a non-null transforming list instead of explicit nulls in the lists - would it be easier to work with the lines if those were used instead?

@SoSeDiK
Copy link
Contributor Author
SoSeDiK commented Apr 13, 2025 •
edited
Loading

MM did a pull request on this way back which was never merged (#9468). That PR used empty components and a non-null transforming list instead of explicit nulls in the lists - would it be easier to work with the lines if those were used instead?

Oh, haven't seen that PR, thanks for the info! Missed (de)serializeOrNull being a thing, updated.

I do agree that getting rid of nulls would be better, but this will break backwards compatibility, and sadly the event existed in this state for years now.

  1. By changing nulls, suddenly old "empty/untouched line" checks based on null will no longer pass.

  2. Similarly, changing lines getter to immutable will break plugins relying on its mutability.

    • There could be an API break for this one, since the mutability was never an API contract (or was it, without marking @Unmodifiable, hmm?) and likely wasn't intended, but suddenly breaking it isn't perfect either imo.

@lynxplay
Copy link
Contributor
8000 lynxplay commented Apr 14, 2025

We definitely cannot break the non null contract on the lines getters/setters.
We might be able to get away with null in the internal list but it would need to be transformed to empty().
If a legacy plugin calls setLine(0, null) and a follow up plugin calls line(0, line(0)), I think it is fine to remove the null value there. They boil down to the same thing once consumed by the server.

But yea, "breaking" (eventho yea, it has been technically broken) newer API in favour of older ones, *especially when this has been the case since 1.16.5 and hence been part of the API for more than 4 years now is not a change I'd be happy with.

@lynxplay
Copy link
Contributor

Any update on this? Given that it bundles the isInRaitOrWater check, it is a bit too on my radar.

SoSeDiK added 3 commits April 22, 2025 11:52
@SoSeDiK
Fix SignChangeEvent NPE and cleanup the class
854d7af 
getLine() / getLines() / legacy constructor no longer NPE
Sign lines now correctly show nullability
@SoSeDiK
Update SignChangeEvent's lines handling
6b31fcf 
Mandate exactly 4 lines
Prevent external lines modification
@SoSeDiK
Use #...OrNull instead of null checks
5f40895
@SoSeDiK
Copy link
Contributor Author
SoSeDiK commented Apr 22, 2025

Any update on this? Given that it bundles the isInRaitOrWater check, it is a bit too on my radar.

I've moved the first two commits into a separate PR (#12462) given that the sign changes seem stuck.

We definitely cannot break the non null contract on the lines getters/setters.

To clarify, the changes in this PR do not break the null contract. getLine/ line were always nullable and getLines/lines were (and currently are) returning nulls as well, just that the javadoc didn't cover that.

We might be able to get away with null in the internal list

If we remove the null internally, there are two things to consider:

  1. Are we fine breaking existing null checks in plugins? empty() will pass those
  2. lines currently returns the internal (array) list directly. We either need to break mutability or return a list that would reject/convert nulls on changes to the list (event constructor also need to convert nulls then)

@Machine-Maker, thoughts? With your prior changes in #9468, would be nice to hear opinions

@SoSeDiK SoSeDiK changed the title Fix SignChangeEvent NPE (& external list mutability) and misc javadoc changes Fix SignChangeEvent NPE (& external list mutability) Apr 22, 2025
@lynxplay
Copy link
Contributor

Ive moved the first two commits into a separate PR (#12462) given that the sign changes seem stuck.

thank you!

To clarify, the changes in this PR do not break the null contract. getLine/ line were always nullable and getLines/lines were (and currently are) returning nulls as well, just that the javadoc didn't cover that.

I do think MM's original PR is nicer in this regard yea, tho yea you are also right, I missed the fact that the component ones are nullable already. Presumably only due to the fact that legacy strings have that nasty habit.

Are we fine breaking existing null checks in plugins? empty() will pass those

I'd be at least. Something going from nullable to non null is generally fine as the non null version could have already been in there. Especially for components, for legacy strings yea, we might be able to find some hacky way to yield back null.

lines currently returns the internal (array) list directly. We either need to break mutability or return a list that would reject/convert nulls on changes to the list (event constructor also need to convert nulls then)

Ah yes, the bane of everyones existence, mutable lists. I hate them with a passion tho yea, that is the bit that I'd be most worried about breaking. MM's PR introduced a nag for this, which sounds like a solid solution for this issue.
Before investing a lot of time into this, lemme throw this PR at the rest of the team tho, so not all of this is based on my personal opinion 😅

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
Paper PR Queue
Status: Awaiting review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SoSeDiK @AnttiMK @lynxplay
0