Skript: Function error in parsing

[TheLimeGlass]

Spigot 1.12.2

dev-34

Done in a chat event

[Blueyescat]

842

Still I can't see someone actively trying to fix it..

[Pikachu920]

There's a lot of bugs and not enough time. Also, function code is kinda messy

[bensku]

Pull requests fixing bugs are always more than welcome. Even more so than new features.

Gotta test this one at some point.

[TheBentoBox]

I can confirm this as well as a number of other function parsing bugs that, personally, have been a huge issue and have completely prevented me from updating past dev25 for a long time. I've made multiple attempts to upgrade and was never able to in part because of these issues:

---

Environment

I'm testing on Spigot 1.12.2 with Skript 2.2-dev34. I am using no Skript add-ons.

Test Script

This script provides two functions and two events which utilize those functions with various operations and situations within or surrounding the function call. This requires no add-ons and was, again, tested without them, and there are no obvious issues that should cause parsing errors in vanilla Skript.

function clamp(n: number, min: number, max: number) :: number:
    if {_n} < {_min}:
        set {_n} to {_min}
    else if {_n} > {_max}:
        set {_n} to {_max}
    return {_n}

function broken(p: player, i: item, name: string):
    broadcast "Or maybe not"

on script load:
    set {_min} to 10
    set {_max} to 20
    set {_n} to 50

    set {_store} to clamp({_n}, {_min}, {_max})
    set {_store} to clamp({_n}/10, {_min}, {_max})
    set {_store} to clamp({_n}*10, {_min}, {_max})
    set {_store} to clamp({_n}, {_min} + 5, {_max})
    set {_store} to clamp({_n}, "%{_min}%" parsed as an integer, {_max})

on right click holding stick:
    loop entities in radius 10 of player's location:
        push loop-entity direction from player to loop-entity
    broken(player, player's tool, display name of player's tool)

Errors

The following is the result when trying to load the above Skript in my local server:

>sk reload Test
[22:13:53 INFO]: [Skript] Reloading Test.sk...
[22:13:53 ERROR]: Functions cannot be used here. (Test.sk, line 17: set {_store} to clamp({_n}/10, {_min}, {_max})')
[22:13:53 ERROR]: Functions cannot be used here. (Test.sk, line 18: set {_store} to clamp({_n}*10, {_min}, {_max})')
[22:13:53 ERROR]: Functions cannot be used here. (Test.sk, line 19: set {_store} to clamp({_n}, {_min} + 5, {_max})')
[22:13:53 ERROR]: Functions cannot be used here. (Test.sk, line 20: set {_store} to clamp({_n}, "%{_min}%" parsed as an integer, {_max})')
[22:13:53 ERROR]: There's no loop that matches 'loop-entity' (Test.sk, line 24: push loop-entity direction from player to loop-entity')
[22:13:53 ERROR]: The function 'broken' requires at least 3 arguments, but only 2 are given. (Test.sk, line 25: broken(player, player's tool, display name of player's tool)')
[22:13:53 INFO]: [Skript] Encountered 6 errors while reloading Test.sk!

I'm not sure exactly how to describe what is causing the first four errors; it seems that literally any operation being performed within the arguments of a function cause it to fail to parse entirely, with the vague error message of "Functions cannot be used here" (which is, of course, not true). You can see that line 16 (the first call to clamp) did parse correctly, as the arguments were passed in "plainly". Any "fuss" breaks the functions.

The next error (There's no loop that matches 'loop-entity') is the same as I described over in #946, which illustrates that the issue with loops around a location still hasn't been fixed. Further conversation about this (if there is any to be had) should be done over in that issue, but it felt appropriate enough to group here while I was testing all of this.

Lastly, the final function call to the broken function fails just as Lime has described. What's interesting is just how broken that can get ("that" being Skript counting the arguments incorrectly). For example, if I change the last argument passed to broken() from display name of player's tool to player's name, the function now fails with The function 'broken' requires at least 3 arguments, but only 1 is given.. Messing around with the arguments further (such as changing player to target) seems to just give varying degrees of failure where Skript believes I've only passed one or two arguments.

Also, I've tried changing the function definition and call for the broken() function, but I haven't been able to draw any consistent conclusions. Changing the definition to only take two parameters (by e.g. removing the item parameter) but still calling it with the same three arguments gives me The 1st argument given to the function 'broken' is not of the required type player. as opposed to the expected error about passing too many arguments (3 instead of 2). Hilariously enough, this is the first time it would have been appropriate to tell me I passed in the wrong number of arguments, and it didn't, instead telling me the first argument wasn't of type player even though it was.

The only consistency in erroring seemed to be with mixing argument types. Calling function broken(p: player, name: text) with broken(player, player's name) fails, but calling function broken(p: text, name: text) with broken("%player%", player's name) passes. I seem to have little success when calling tons of previously working functions, though. Something serious was broken at some point.

---

I've yet to see many issues labelled as super high priority here, so I've held back and gone with medium, but (to be blunt) I consider these issues pretty serious and I'm honestly surprised anyone with any amount of complexity in their scripts is able to update to any of the recent releases. There have been apparent parsing errors the break completely valid existing scripts and I'm always surprised to see people running a version like dev34 with the sheer number of parsing errors I encounter while using it where previously there weren't any. Hopefully I'm just missing something or have an error in my setup.

[bensku]

With this example involving only numbers and arithmetic, I was able to find the source of problem. And it is... troubling, to say at least. This bug has nothing to do with some expressions being parsed wrong.

Function calls are all parsed completely wrong, but in some cases it appears to not cause issues that are visible to scripters. In other cases... Well, you have seen it.

Edit: Further investigation seems to indicate that I might be a bit incorrect. But still wondering how could I fix this stuff.

Edit: Found out that the list parsing seems to be the issue here. Following snippet crashes runtime:

set {_test} to 1 + (1, 2, 3)

The list (1, 2, 3) is parsed as a number for some reason.

[bensku]

I have made a fix for this and #842. But I worry that it might break other stuff. #946 is sadly not fixed by this little change.

Gotta say I want to get a new parser at some point. Took hours to figure out what was wrong and realize the scope of the issue. The fix is like 3 lines of code.

It will appear in next nightly build, which I encourage you to try. A bit more stable release is coming once we get some pull requests merged and maybe some other changes done.

[HYPExMon5ter]

A new parser is long overdue, but so much work and would prob break a lot of things

[TheBentoBox]

Agreed about the new parser; I imagine you'd even make up the dev time eventually given situations like this where the convoluted code makes an issue take hours longer to track down. Regardless, it's great to hear that you've found a fix for the function parsing issues. The issue in #946 is also of a somewhat lower priority IMO given that it can be "fixed" by removing the 's location from the loop definition, as I outlined over there. AKA, it can be fixed fairly easily by the user until it is properly fixed, whereas we have no control over the function parsing issues.

I'll try out that nightly build when it comes around!

[TheBentoBox]

@bensku I've tested the nightly build, it does seem to have fixed a lot of the errors but it's a bit hard to tell since it also seems any addon that relies on the slot class fails to enable, which for me is TuSKe, SkQuery (I'm using Lime's updated version), and MundoSK. The errors look like this:

[13:15:20 ERROR]: Error occurred while enabling TuSKe v1.8.2-beta (Is it up to date?)
java.lang.NoClassDefFoundError: ch/njol/skript/util/Slot

I'm not sure if the changes to the slot class can be made more compatible with these add-ons as to not break them, but if not then I'm sure they'll update after the next release to be compatible.

Also, there's a debug message that was left in which spams console both while loading scripts and the entire time the server is up. The class in the debug message isn't consistent, which is probably obvious:

[Skript] Checking class ch.njol.skript.expressions.ExprLoopValue

[HYPExMon5ter]

I’ve already told him about it breaking addons :P

[bensku]

Gotta remove debug stuff before release, of course.

I'll think about duplicating Slot class for addon compatibility. Or just moving it back there. It should be mostly compatible otherwise.