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

Avoid NPE by defaulting to enum name when no language node available in EnumUtil's toString. #6375

Merged

Conversation

sovdeeth
Copy link
Member

Description

If there isn't a language node available for an enum value, EnumUtil#toString() will return null. This is used in EnumClassInfo's Parser, which requires the toString to be non-null. Since we can pretty easily return null, this leads to an NPE later down the line when you attempt to send <some expression that returns that enum value>.

I've chosen to replace it with the enum value's name, so the user can still understand what it is, even if there's no lang node for them to use. Since this is fixing something that should only occur when we haven't yet updated to a newer minecraft version, I think this is an appropriate bandaid.

Prior to change: (creative has been manually removed from default.lang)

[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! [Skript] Severe Error:
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! Something went horribly wrong with Skript.
[17:32:47 ERROR]: #!#! This issue is NOT your fault! You probably can't fix it yourself, either.
[17:32:47 ERROR]: #!#! It looks like you are using some plugin(s) that alter how Skript works (addons).
[17:32:47 ERROR]: #!#! Here is full list of them:
[17:32:47 ERROR]: #!#! skript-reflect v2.4 (https://github.com/SkriptLang/skript-reflect)
[17:32:47 ERROR]: #!#! We could not identify which of those are specially related, so this might also be Skript issue.
[17:32:47 ERROR]: #!#! You should try disabling those plugins one by one, trying to find which one causes it.
[17:32:47 ERROR]: #!#! If the error doesn't disappear even after disabling all listed plugins, it is probably Skript issue.
[17:32:47 ERROR]: #!#! In that case, you will be given instruction on how should you report it.
[17:32:47 ERROR]: #!#! On the other hand, if the error disappears when disabling some plugin, report it to author of that plugin.
[17:32:47 ERROR]: #!#! Only if the author tells you to do so, report it to Skript's issue tracker.
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! Stack trace:
[17:32:47 ERROR]: #!#! java.lang.NullPointerException: Cannot invoke "String.toCharArray()" because "msg" is null
[17:32:47 ERROR]: #!#!     at Skript.jar//ch.njol.skript.util.chat.ChatMessages.fromParsedString(ChatMessages.java:424)
[17:32:47 ERROR]: #!#!     at Skript.jar//ch.njol.skript.effects.EffMessage.execute(EffMessage.java:137)
[17:32:47 ERROR]: #!#!     at Skript.jar//ch.njol.skript.lang.Effect.run(Effect.java:49)
[17:32:47 ERROR]: #!#!     at Skript.jar//ch.njol.skript.lang.TriggerItem.walk(TriggerItem.java:61)
[17:32:47 ERROR]: #!#!     at Skript.jar//ch.njol.skript.lang.TriggerItem.walk(TriggerItem.java:88)
[17:32:47 ERROR]: #!#!     at Skript.jar//ch.njol.skript.command.Commands.handleEffectCommand(Commands.java:201)
[17:32:47 ERROR]: #!#!     at Skript.jar//ch.njol.skript.command.Commands$2.lambda$onPlayerChat$0(Commands.java:300)
[17:32:47 ERROR]: #!#!     at org.bukkit.craftbukkit.v1_20_R3.scheduler.CraftFuture.run(CraftFuture.java:88)
[17:32:47 ERROR]: #!#!     at org.bukkit.craftbukkit.v1_20_R3.scheduler.CraftScheduler.mainThreadHeartbeat(CraftScheduler.java:480)
[17:32:47 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.b(MinecraftServer.java:1633)
[17:32:47 ERROR]: #!#!     at net.minecraft.server.dedicated.DedicatedServer.b(DedicatedServer.java:446)
[17:32:47 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.a(MinecraftServer.java:1512)
[17:32:47 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.w(MinecraftServer.java:1214)
[17:32:47 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:321)
[17:32:47 ERROR]: #!#!     at java.base/java.lang.Thread.run(Thread.java:833)
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! Version Information:
[17:32:47 ERROR]: #!#!   Skript: 2.8.0 (custom version)
[17:32:47 ERROR]: #!#!     Flavor: selfbuilt-unknown
[17:32:47 ERROR]: #!#!     Date: unknown
[17:32:47 ERROR]: #!#!   Bukkit: 1.20.4-R0.1-SNAPSHOT
[17:32:47 ERROR]: #!#!   Minecraft: 1.20.4
[17:32:47 ERROR]: #!#!   Java: 17.0.2 (OpenJDK 64-Bit Server VM 17.0.2+8)
[17:32:47 ERROR]: #!#!   OS: Windows 11 amd64 10.0
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! Server platform: Paper
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! Current node: null
[17:32:47 ERROR]: #!#! Current item: send the gamemode of the player to event-command sender
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! Thread: Server thread
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! Language: english
[17:32:47 ERROR]: #!#! Link parse mode: DISABLED
[17:32:47 ERROR]: #!#!
[17:32:47 ERROR]: #!#! End of Error.
[17:32:47 ERROR]: #!#!

After change:
image


Target Minecraft Versions: any
Requirements: none
Related Issues: #6370

@sovdeeth sovdeeth added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.8 Targeting a 2.8.X version release labels Jan 30, 2024
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

I do have a slight concern about users thinking that these names may be supported in syntax (when they aren't). Do you think it would be better to just display <none>? Or maybe that isn't a big deal and we should format it to be more readable (e.g. MY_ENUMERATOR -> my enumerator

LMK what you think :)

@sovdeeth
Copy link
Member Author

sovdeeth commented Feb 1, 2024

I do have a slight concern about users thinking that these names may be supported in syntax (when they aren't). Do you think it would be better to just display <none>? Or maybe that isn't a big deal and we should format it to be more readable (e.g. MY_ENUMERATOR -> my enumerator

LMK what you think :)

I considered that but I thought the all caps might be a clue that it's not usable in Skript. I think it would be confusing if it was formatted to be readable, though.

None would work, but feels too limiting imo.

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Should be okay as is. I realize <none> would not work as it would not actually be "none", but just a string containing "none"

Copy link
Member

@UnderscoreTud UnderscoreTud left a comment

Choose a reason for hiding this comment

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

Could it have some kind of debug log message that a language node is missing

@APickledWalrus
Copy link
Member

Could it have some kind of debug log message that a language node is missing

this is done at startup, it got kind of spammy in the past when it was each time it encountered a missing entry

@sovdeeth sovdeeth added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Feb 1, 2024
@APickledWalrus APickledWalrus merged commit 5dd60e5 into SkriptLang:dev/patch Feb 1, 2024
4 checks passed
ShaneBeee pushed a commit to ShaneBeee/Skript that referenced this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants