Skip to content

Improve Sheets taint-related errors by storing and re-throwing initial exception#2775

Draft
noobanidus wants to merge 2 commits intoneoforged:1.21.11from
noobanidus:1.21.x-sheets-taint
Draft

Improve Sheets taint-related errors by storing and re-throwing initial exception#2775
noobanidus wants to merge 2 commits intoneoforged:1.21.11from
noobanidus:1.21.x-sheets-taint

Conversation

@noobanidus
Copy link
Copy Markdown
Contributor

Statically loading Sheets before NeoForge's registries have been finalized, often caused by referencing Sheets (or a class that references Sheets) statically in an EventBusSubscriber prevents modded WoodType added via Sheets::addWoodType from being properly registered.

This results in getSignMaterial and getHangingSignMaterial to return null for these types.

Further complicating this is the fact that instances which use this Material (such as in GuiSignRenderer::renderToTexture) often use function references to determine the render type.

Compiled, this transforms the section into a synthetic lambda and results in unclear NullPointerExceptions that reference the parameters of the lambda ($$3, for example) rather than a clear dereference indicating that the Material is null such as (1.21.1 example):

java.lang.NullPointerException: Cannot invoke "net.minecraft.client.resources.model.Material.buffer(net.minecraft.client.renderer.MultiBufferSource, java.util.function.Function)" because "$$3" is null
	at TRANSFORMER/minecraft@1.21.1/net.minecraft.client.gui.screens.inventory.SignEditScreen.renderSignBackground(SignEditScreen.java:57) ~[client-1.21.1-20240808.144430-srg.jar%23610!/:?] {re:mixin,re:classloading}
	at TRANSFORMER/minecraft@1.21.1/net.minecraft.client.gui.screens.inventory.AbstractSignEditScreen.renderSign(AbstractSignEditScreen.java:155) ~[client-1.21.1-20240808.144430-srg.jar%23610!/:?] 

Currently, Sheets contains a warning message which is displayed whenever it is statically loaded before registries have been loaded (and the mod loader isn't in an error state), but it is possible to miss this as it generally occurs during mod loading and a large amount of data is written to the log at this point.

Based on discussions with XFactHD, throwing an exception at this point would result in further confusion, although the new error screen may resolve that.

This patch solves that confusion by storing statically storing the exception and then re-throwing it the first time getSignMaterial or getHangingSignMaterial is called.

While this results in an IllegalStateException rather than the current NullPointerException, the relevant stack trace more accurately shows the actual cause of the issue rather than the unclear error with the synthetic lambda.

I'm uncertain how possible it would be to create a test mod for this.

Relevant issue: MysticMods/Roots#1214

Statically loading `Sheets` before NeoForge's registries have been
finalized, often caused by referencing `Sheets` (or a class that
references `Sheets`) statically in an `EventBusSubscriber` prevents
modded `WoodType` added via `Sheets::addWoodType` from being properly
registered.

This results in `getSignMaterial` and `getHangingSignMaterial` to return
null for these types.

Further complicating this is the fact that instances which use this
`Material` (such as in `GuiSignRenderer::renderToTexture`) often use
function references to determine the render type.

Compiled, this transforms the section into a synthetic lambda and
results in unclear NullPointerExceptions that reference the parameters
of the lambda (`$$3`, for example) rather than a clear dereference
indicating that the `Material` is null.

Currently, `Sheets` contains a warning message which is displayed
whenever it is statically loaded before registries have been loaded (and
the mod loader isn't in an error state), but it is possible to miss
this.

Throwing an exception at this point would potentially result in further
confusion.

This patch solves that confusion by storing statically storing the
exception and then re-throwing it the first time `getSignMaterial` or
`getHangingSignMaterial` is called.

While this results in an `IllegalStateException` rather than the current
`NullPointerException`, the relevant stack trace more accurately shows
the actual cause of the issue rather than the unclear error with the
synthetic lambda.
@neoforged-automation neoforged-automation Bot added the 1.21.10 Targeted at Minecraft 1.21.10 label Oct 31, 2025
@neoforged-pr-publishing
Copy link
Copy Markdown

  • Publish PR to GitHub Packages

@Technici4n
Copy link
Copy Markdown
Member

Why is this specific to the sign materials? What we could also do is adding a ModLoadingIssue to force an error screen to be displayed.

@XFactHD
Copy link
Copy Markdown
Member

XFactHD commented Nov 2, 2025

This applies to sign and hanging sign materials and decorated pot pattern materials, the former two due to WoodTypes often only being "registered" (it's not an actual registry, it's just a map in WoodType) when a block using one is classloaded and/or instantiated and the latter due to iterating the decorated pot pattern registry to populate the material map. For the sign materials (Neo)Forge adds a helper to register them after the fact but it appears to not be used consistently as people expect the iteration in static init to work fine. In the past this also applied to banner patterns (which have been a registry for a while and were the initial motivation for this check in static init) but those use computeIfAbsent() instead of eager computation now.

@noobanidus
Copy link
Copy Markdown
Contributor Author

Why is this specific to the sign materials? What we could also do is adding a ModLoadingIssue to force an error screen to be displayed.

Further, this PR should be considered "on hold" (at least for 1.21.10) as the recently merged FML update for the early loading screen should allow for the IllegalStateException to be thrown in static initialization, although there are still some issues with it which I believe sharrte is working on resolving.

@noobanidus noobanidus marked this pull request as draft November 3, 2025 01:52
@neoforged-automation
Copy link
Copy Markdown
Contributor

@noobanidus, this pull request has conflicts, please resolve them for this PR to move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.21.10 Targeted at Minecraft 1.21.10 needs rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants