Some ConvertedExpressions fail to convert

[Mr-Darth]

Skript/Server Version

[21:49:43 INFO]: [Skript] Skript's aliases can be found here: https://github.com/SkriptLang/skript-aliases
[21:49:43 INFO]: [Skript] Skript's documentation can be found here: https://skriptlang.github.io/Skript
[21:49:43 INFO]: [Skript] Skript's tutorials can be found here: https://docs.skriptlang.org/tutorials
[21:49:43 INFO]: [Skript] Server Version: git-Paper-350 (MC: 1.18.2)
[21:49:43 INFO]: [Skript] Skript Version: 2.6.3
[21:49:43 INFO]: [Skript] Installed Skript Addons: None
[21:49:43 INFO]: [Skript] Installed dependencies: None

Bug Description

This issue has probably been present in Skript for a long time, but it didn't become apparent until PR #4260 (part of Skript 2.6.2+), which removed a 7 years old chunk of code added by Mirreski, based on another 9 years old code by Njol.

That code had the (side?) effect of catching some common literals and returning them with their proper type (instead of becoming UnparsedLiterals later on). With it removed, these literals can now become UnparsedLiterals (with Object return type). This leads to some issues later on, in LiteralLists.

When a LiteralList is formed with UnparsedLiterals, the return type of the list will be Object. Even if the list's UnparsedLiterals are converted (LiteralUtils.defendExpression(Expression)), the return type remains unchanged - still Object.

An expression like loop-value will use that Object return type. If loop-value is used in a syntax taking in a specific type (like itemtype), the expression will be wrapped in a ConvertedExpression to convert the value to the required type. But here, another problem appears. There are no Object -> X converters, so Skript will try to improvise and return a wrong converter or not return anything at all.

With the wrong converter, the expression will fail to return. On the other hand, if no converter is found, an error will be issued at parse time saying the expression is not of the required type.

To sum it up: SkriptParser makes UnparsedLiteral which becomes part of LiteralList which keeps Object return type. Some expressions then use that return type. Those expression get wrapped in a ConvertedExpression which might use the wrong converter which will result in 💥 (ok, maybe not that dramatic)

(Keep in mind that #4260 did not cause the whole of this, it only made the problem obvious by making it apply to some common types. The issue can be reproduced on other types even without that PR.)

Expected Behavior

The expression should return the correct value (i.e. the converter must not fail).

There are a few different ways this could've gone differently to reach proper behaviour:

• UnparsedLiterals shouldn't be created if there are no conflicts
• The LiteralList should have changed its type; because all syntaxes taking in %object(s)% (and therefore allowing UnparsedLiterals) should already defend their expressions, this is a viable fix
• Converters should account for this issue
• There might be a limited number of expressions with this problem (I only know of loop-value), so it could be fixed from there
• Other ways I didn't think of

Steps to Reproduce

on load:
    loop 1, 2 and 3:
        broadcast loop-value + 1

The above code prints:

1
1
1

This is because, as described above, 1, 2 and 3 will form a LiteralList of UnparsedLiterals. The loop-value expression will use an Object return type, so it needs to be converted to Number. There isn't any Object -> Number converter, so Skript uses something else. (It looks like it uses Integer -> Long (because that's the first 'acceptable' converter it could find) and it fails because the looped value is a Long, not an Integer.)

Types like Boolean that have no registered converters will simply throw an error. (Because Boolean was not actually covered in the code removed by that PR, this issue also happens on older versions.)

This can also be reproduced with other literals (except Strings - they get handled beforehand).

Errors or Screenshots

No response

Other

A workaround is using the default value expression on the bad expression. For the above code, one can do loop-value ? {_} to fix it. This is because converting default values works slightly different.

Agreement

• I have read the guidelines above and affirm I am following them with this report.

[bluelhf]

i can't believe this is real

[TPGamesNL]

Solution implemented in #5047 matches 'The LiteralList should have changed its type; because all syntaxes taking in %object(s)% (and therefore allowing UnparsedLiterals) should already defend their expressions, this is a viable fix', because calling defendExpression changes the exprs in it, but not its type.

Issue will be kept open tho, converter lookup should be changed for Object -> X, i.e. it shouldn't return nonsense like it currently does. For example, a converter that only looks up the actually used converter at runtime, when the 'Object' is a more specific type (because we have the value).

[Mr-Darth]

Alright, back with more issues regarding converters :)

function bananas() :: numbers:
    set {_hello::*} to 1 and 2
    return reversed {_hello::*}

Returns absolutely nothing.

This seems to be because EffReturn tries converting the expression to match the return type, so ExprReversedList gets wrapped in a ConvertedExpression using, again, the wrong converter (Integer -> Long 😜).
Note: Using the variable without reversing it does not cause this issue because, as with default values (see workaround explained above), converting them works differently.

PR #5047 does not fix this issue (I've tested) because this has nothing to do with LiteralLists. PR #4566 doesn't seem to fix it either, but type hints can probably help here.
The root cause of all this nonsense is the flawed lookup of Object -> X converters.

[TPGamesNL]

Did another PR, but also not the main issue here :)
#5052

It does fix the issue described above tho

[TheLimeGlass]

TPGamesNL wanted to keep this issue open as there is more to this issue. Keeping the PR available tag present aswell as complete, to make maintainers read as to why this issue shouldn't be closed. Also assigned issue to TP.