Skip to content

Conversation

@modmuss50
Copy link
Member

I have been meaning to make this change for a long while, its best to be done during a snapshot series.

@Player3324
Copy link
Contributor

Meh, I like assert but not in all cases affected by the PR.

Assert is well suited for double-checking anything that has already been verified elsewhere or must be valid if the implementation works as intended. It is not a tool to check api compliance, let alone network protocol adherence. It is a bit like an in-place unit test without the code distortion and overhead a regular test may require.

Unlike the explicit check assert is free and clearly communicates its purpose when only used like above.

@modmuss50
Copy link
Member Author

I think its valid use cases are so limited its just easier to not use it at all to remove the confusion. We have seen in the past were external factors (other mods) have caused the assertions to fail, but havent been noticed until someone decides to run the game with them enabled.

In FAPI we run with them enabled here: https://github.com/FabricMC/fabric/blob/1.21.8/build.gradle#L22 but most people dont, meaning if they do something to trigger one of these cases they wont find it at dev time.

If we want to skip some of these checks in prod, it should query loader to see if its a dev env or not.

@Earthcomputer
Copy link
Contributor

One situation where assert is useful is to tell IntelliJ's dataflow analysis that a certain expression is true, where it is obviously true from a human perspective. An example would be assert MinecraftClient.getInstance().world != null. In this case you could replace it with Objects.requireNonNull though.

@modmuss50
Copy link
Member Author

Objects.requireNonNull is what we already use everywhere, as you can see in the PR there wasnt a single usecase for that. There is always a tottaly reasonable alternative to assert for the cases where it is acceptable.

@modmuss50 modmuss50 added the status: merge me please Pull requests that are ready to merge label Aug 29, 2025
@modmuss50 modmuss50 merged commit 974c632 into FabricMC:1.21.9 Aug 29, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup status: merge me please Pull requests that are ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants