Fix ExprSets conflicting

[TheLimeGlass]

Description

Fixes ExprSets conflicting with set {_players::*} to all players
The error mainly is present when parsing syntaxes with %players% not the above example.
The expression ExprSets should be returning false silently without an error to allow Skript to continuously keep checking for other syntaxes. Essentially grabbing Player EntityData instead of ExprSets.
We don't want to error when Skript checks ExprSets as it should be returning all the players on the server, not using the classinfo to collect all the players (which has no supplier causing this error).

---

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

[UnderscoreTud]

I'm sorry, how does removing a simple log fix an issue (which doesn't even exist)? Can you show me an example of syntax that didn't work before this PR, and how it works after

[TheLimeGlass]

I'm sorry, how does removing a simple log fix an issue (which doesn't even exist)? Can you show me an example of syntax that doesn't work before this PR, and how it works after

In the parser when it collects the pattern to the syntax to then call the init of it, if it errors at any point in the iteration, it'll stop.
Not causing an error makes it continue in the iteration returning false, and if it doesn't find any other syntaxes, it'll state cannot understand as no pattern matches.

[sovdeeth]

I'm also not sure what the original bug was, do you have a reproducible example? I don't think I've ever encountered it.

[UnderscoreTud]

What? That is just not how it works. The parser iterates through all the elements, finding one whose pattern matches the user's input and returns it if its init returns true, otherwise it continues iterating through the rest of the elements repeating the process. Logs are only accumulated after the whole process's done, they do not affect this whatsoever. And you have yet to provide an example of how this changes anything.

[TheLimeGlass]

What? That is just not how it works. The parser iterates through all the elements, finding one whose pattern matches the user's input and returns it if its init returns true, otherwise it continues iterating through the rest of the elements repeating the process. Logs are only accumulated after the whole process's done, they do not affect this whatsoever. And you have yet to provide an example of how this changes anything.

This pull request solved the issue, so that's crazy. Not sure what you're on about. Can you explain how that isn't how it works?

I did provide an example. This error got the error to properly go away.

[Fusezion]

This pull request solved the issue, so that's crazy. Not sure what you're on about. Can you explain how that isn't how it works?

I did provide an example. This error got the error to properly go away.

I'm with Tuddler on this, I haven't had any issues at all with this class, and what error? Screenshot?

[UnderscoreTud]

Give me the proper Skript code you tested. And run it before and after this PR and show me the results please.

[TheLimeGlass]

Give me the proper Skript code you tested. And run it before and after this PR and show me the results please.

You can literally call a non believe syntax and it'll throw ExprSets

set {_example::*} to some fake syntax where all players

That's a problem.
My Fix

[00:22:54] [Server thread/INFO]: Line 2: (listenUp.sk)
[00:22:54] [Server thread/INFO]:     Can't understand this expression: 'some fake syntax where all players'
[00:22:54] [Server thread/INFO]:     Line: set {_example::*} to some fake syntax where all players

2.7.1

[00:27:14 INFO]: [Skript] Line 2: (listenUp.sk)
[00:27:14 INFO]:     You cannot get all values of type 'player'
[00:27:14 INFO]:     Line: set {_example::*} to some fake syntax where all players

[sovdeeth]

I cannot replicate on 2.7.1:

[TheLimeGlass]

I cannot replicate on 2.7.1:

Updated my past post with evidence

[sovdeeth]

Updated my past post with evidence

Can you provide your /sk info please?

[TheLimeGlass]

Updated my past post with evidence

Can you provide your /sk info please?

This is a bug fix not an issue report.

[sovdeeth]

Updated my past post with evidence

Can you provide your /sk info please?

This is a bug fix not an issue report.

I understand, but I want to be able to replicate the issue so I can, you know, test if it's fixed.

[TheLimeGlass]

Updated my past post with evidence

Can you provide your /sk info please?

This is a bug fix not an issue report.

I understand, but I want to be able to replicate the issue so I can, you know, test if it's fixed.

[01:07:08 INFO]: [Skript] Skript's aliases can be found here: https://github.com/SkriptLang/skript-aliases
[01:07:08 INFO]: [Skript] Skript's documentation can be found here: https://docs.skriptlang.org/
[01:07:08 INFO]: [Skript] Skript's tutorials can be found here: https://docs.skriptlang.org/tutorials
[01:07:08 INFO]: [Skript] Server Version: git-Paper-171 (MC: 1.20.1)
[01:07:08 INFO]: [Skript] Skript Version: 2.7.1 (skriptlang-github)
[01:07:08 INFO]: [Skript] Installed Skript Addons: None
[01:07:08 INFO]: [Skript] Installed dependencies: None

on load:
    set {_example::*} to some fake syntax where all players

[00:27:14 INFO]: [Skript] Line 2: (listenUp.sk)
[00:27:14 INFO]:     You cannot get all values of type 'player'
[00:27:14 INFO]:     Line: set {_example::*} to some fake syntax where all players

[Moderocky]

I don't seem to be able to reproduce this issue on the current master release.

Or at the head fbf2e4 of patch branch.

Or with 2.8.0-dev on feature branch.

Do you think the error might be coming from a different place?

[sovdeeth]

Likewise, I'm not sure how you're having it occur, Lime.

[TheLimeGlass]

Ok mark my word when a user reports it.

[sovdeeth]

Ok mark my word when a user reports it.

I would love to know why it's showing up for you but not for us, because that indicates a rather insidious issue. Maybe if you sent your server environment as a zip?

[sovdeeth]

@Fusezion helped out and found that using of all players is a key requirement to replicating this, and with that info I can replicate on 2.7.1. That said, I don't think simply removing the error is the correct response to this issue. Tud is currently investigating further.

[sovdeeth]

Using the nightly from this PR, I don't think anything changed for the user behavior. I think it should be MATCHES_EVERYTHING just on principle, but I don't think it fixed the issue.

[TheLimeGlass]

Using the nightly from this PR, I don't think anything changed for the user behavior. I think it should be MATCHES_EVERYTHING just on principle, but I don't think it fixed the issue.

It is pattern matches everything. Please build the branch yourself.

[sovdeeth]

It is pattern matches everything. Please build the branch yourself.

I know, I'm agreeing with that change. I just didn't have it solve the issue for me. I'll try a self-build anyway, though.

edit: Yup, same thing:

[TheLimeGlass]

It is pattern matches everything. Please build the branch yourself.

I know, I'm agreeing with that change. I just didn't have it solve the issue for me. I'll try a self-build anyway, though.

edit: Yup, same thing:

It solved the issue for me. Tud was saying an error should be printed, but I can reset this pull request back to the original commit, the original issue I had already solved. I can't take this seriously when you stated the issue never existed to begin with. Please do a clean reset test. Just changing the expression type solved the issue for me.

[UnderscoreTud]

The issue is not really with ExprSets or its priority, but rather with how patterns are matched.

First we have to see how a %type% pattern element is matched. Let's consider the pattern %itemtype% with sauce and the input string dirt with sauce. The pattern, internally, looks like %itemtype%-> with sauce. The way Skript matches a type element is by first matching everything after it, then trying to find an expression suitable for that type. Basically, it first and foremost makes sure with sauce is there, then it considers whether dirt is a valid itemtype.

Back to the issue at hand now, Skript just happens to have the pattern [(all [[of] the]|the)] %*entitydatas% [(in|of) ([world[s]] %-worlds%|1¦%-chunks%)] in ExprEntities. Do keep in mind that there's nothing inherently wrong with it.

Now consider those two elements' patterns, with the input some fake syntax of all players. Remember how Skript does it:

• it first tries the optional group, but doesn't match so it moves forward in the pattern, then comes a space literal but it's irrelevant here;
• now comes %entitydatas%, but Skript proceeds to check for the stuff after it; this means, after a few tries, it will have split up the input into some fake syntax and of all players; the latter is matched against [(in|of) ([world[s]] %-worlds%|1¦%-chunks%)] and this is where it fails, because all players cannot be collected using a Supplier and is not a world either, leading to the error;
• assuming Skript wouldn't have failed on the previous step, it would've only now attempted to parse some fake syntax as entitydata.

TL;DR It's not ExprSets, or any other expression for that matter, but rather the way type pattern elements are matched against the input strings.

[TheLimeGlass]

The issue is not really with ExprSets or its priority, but rather with how patterns are matched.

First we have to see how a %type% pattern element is matched. Let's consider the pattern %itemtype% with sauce and the input string dirt with sauce. The pattern, internally, looks like %itemtype%-> with sauce. The way Skript matches a type element is by first matching everything after it, then trying to find an expression suitable for that type. Basically, it first and foremost makes sure with sauce is there, then it considers whether dirt is a valid itemtype.

Back to the issue at hand now, Skript just happens to have the pattern [(all [[of] the]|the)] %*entitydatas% [(in|of) ([world[s]] %-worlds%|1¦%-chunks%)] in ExprEntities. Do keep in mind that there's nothing inherently wrong with it.

Now consider those two elements' patterns, with the input some fake syntax of all players. Remember how Skript does it:

* it first tries the optional group, but doesn't match so it moves forward in the pattern, then comes a space literal but it's irrelevant here;

* now comes `%entitydatas%`, but Skript proceeds to check for the stuff _after_ it; this means, after a few tries, it will have split up the input into `some fake syntax` and `of all players`; the latter is matched against `[(in|of) ([world[s]] %-worlds%|1¦%-chunks%)]` and this is where it fails, because `all players` cannot be collected using a Supplier and is not a world either, leading to the error;

* assuming Skript wouldn't have failed on the previous step, it would've only now attempted to parse `some fake syntax` as entitydata.

TL;DR It's not ExprSets, or any other expression for that matter, but rather the way type pattern elements are matched against the input strings.

Yes.
ExprSets having a very transient pattern doesn't help Skript out in figuring this out, so we have to adjust how it works.
In my case just changing the parsing order fixes it. So Skript grabs ExprEntities before ExprSets. @Fusezion can you test that this is the case for you aswell since you found this issue too?
The other way is to have it not error as I originally had, so that Skript can keep looking for the correct input in the pattern.
I do agree that more context could be provided to find the best result.

[UnderscoreTud]

Are we now supposed to change all expressions to accommodate this?

P.S. You mentioned the wrong person.

[TheLimeGlass]

Are we now supposed to change all expressions to accommodate this?

P.S. You mentioned the wrong person.

No, Skript is capable of finding the correct pattern in most cases. We have to do small adjustments like this pull request.

[Pikachu920]

regardless of the rest of the discussion I am a little cautious about using PATTERN_MATCHES_EVERYTHING when the pattern doesn't match everything. if that's part of the desired fix, maybe we need a more specific ExpressionType

[sovdeeth]

It solved the issue for me. Tud was saying an error should be printed, but I can reset this pull request back to the original commit, the original issue I had already solved. I can't take this seriously when you stated the issue never existed to begin with. Please do a clean reset test. Just changing the expression type solved the issue for me.

I would love if you could send your exact server environment as a zip because I cannot for the life of me replicate the fix. No matter what I do, build it myself, clean, reinstall, run the nightly, it still shows the same error. If anyone else can replicate the fix, please send me your environment too, because I really would like to know what's wrong with my environment in that case.

nightly:

self-built with clean build:

[TheLimeGlass]

It solved the issue for me. Tud was saying an error should be printed, but I can reset this pull request back to the original commit, the original issue I had already solved. I can't take this seriously when you stated the issue never existed to begin with. Please do a clean reset test. Just changing the expression type solved the issue for me.

I would love if you could send your exact server environment as a zip because I cannot for the life of me replicate the fix. No matter what I do, build it myself, clean, reinstall, run the nightly, it still shows the same error. If anyone else can replicate the fix, please send me your environment too, because I really would like to know what's wrong with my environment in that case.

nightly:

self-built with clean build:

I didn't test the fix on 2.7.1. I use a custom 2.8.0-dev version on my server (basically all my pulls in one jar), so I just cherry picked this fix to it, and it solved it. Something on 2.8.0 solves the issue. It's probably my literal reparsing and event value collecting pull requests saving the day.
I bet on 2.7.1 it would require no error like this pull request originally did in order to fix, otherwise if you can replicate that this gets fixed on 2.8 it can be targeted to 2.8.

I personally still thing just removing the error is good, but others had opinions about keeping it, so it would be up for debate.

[sovdeeth]

I merged this branch into dev/feature and built off of that, and still got the same issue. Can you send your Skript jar at the very least, please?

[UnderscoreTud]

@sovdeeth I already explained the issue up above. It has nothing to do with this pull request nor the expression its attempting to "fix". You can't really call it a bug because it is behaving as intended and you can't really do anything about it without majorly hurting the parse time