Not executing event trigger under specific circumstances when event has already been cancelled

[stefvanschie]

I decided to create an issue out of this first to get some feedback on the idea.

When working with events, often times you want to cancel the event while not doing anything else. Examples of this may be stopping the weather from changing, stopping the time from cycling, stopping certain players from chatting, etc. When making such an event, QuickSkript (probably as well as most other interpreters) will execute the skript code inside this event every time the event is called.

When using Java, we get an interesting option when registering our event handlers. We can tell Bukkit that we do not want to receive the event when the event has already been cancelled by specifying the ignoreCancelled parameter in our annotation and setting it to true. While this is not always desirable, one place where you can always make use of this, is when your event only cancels the event, while doing nothing else. Executing this event when it has already been cancelled would be useless: the cancellation state would stay the same while wasting time by executing unnecessary code.

The way we currently register events would allow us to specify an option as stated above as well in the form of a boolean parameter. Since the elements have been parsed before we register the event, we can analyze these elements in advance. My idea is to analyze these elements and then see if we can ignore this event when it has already been cancelled. In order to determine whether this is possible we do the following things:

• Check whether the event is cancellable or not: if the event is not cancellable, the parameter doesn't matter and we can stop analyzing elements further.
• Check all elements and see if they do not modify any states. States imply that when the code would run, the effects of this can be seen after the code has been fully executed. We do this instead of just checking for a cancel event statement, because then we can also test for code that for example has an if statement before a cancel event statement. Note that we do not accept elements which change states which are only visible when the event isn't cancelled. Say you have an event which changes a message which only gets printed when the event is executed. You could make the assumption, since the message doesn't get shown when the event is cancelled, we can flag this event as well. However this may not be true. When a low priority event cancels the event and then our event gets ignored, it could be that a higher priority event uncancels the event. If we would ignore our event, the message would not be set even though the event will get executed after all. Underneath this post there are examples with whether they would or would not get flagged to ignore the event when cancelled.

After these checks have been executed and returned successfully, we can set this event to be ignored when the event has been cancelled. Furthermore, this system will only ignore events when cancelled when there is absolute certainty this is possible; there will be no incorrectly marked events.

By having such a system we could reduce overhead and execution times by preventing unnecessary code from being executed. This would guarantee that no code is executed when it's not needed and any heavy computations these events might be doing would only be done when absolutely needed.

It has to be noted however that when we get a feature in the future to possibly set the cancellation state of an event to false (the event will no longer be cancelled), that we have to detect this and not flag any event which has such an expression.

Examples for pieces of code. Note that not everything shown here is already implemented.

on join:
    set join message to ""
    stop

This would not get flagged: the underlying event isn't cancellable.

on weather change:
    cancel event

This will get flagged: the WeatherChangeEvent is cancellable, and there are no pieces of code that would perform modifications.

on pick up:
    if player has permission "hubcore.admin":
        stop
    else:
        cancel event

This will get flagged: the PlayerPickUpItemEvent is cancellable, and there are no pieces of code that would perform modifications (permission checks don't do modifications and neither does the stop).

If no objections are made against this idea, I'd be happy to implement it.

[Trigary]

I personally believe that this should be postponed until additions are made by this project to the Skript language: it should be solved by adding some kind of options field to the event declaration. The event priority should also be specifiable there.

Code analysis is nice, but I think it should be done outside of this project. I personally believe that the trend of warning about constant conditions (which I started) is also stretching it. Some sort of pre-parser/analyzer, which takes care of the precomputed values as well, would be nice. Its main goal could just be to just warn about stuff, not actually to fix them.

[stefvanschie]

I agree that we should have some option to specify whether we want to ignore the event when it has been cancelled. As far as I know, there is currently no option for this in Skript, but we can probably come up with some way of specifying this.

As for the event priorities, SkriptLang currently has an option in the config to specify a global priority. There are no per event priorities, although we have this issue: SkriptLang/Skript#197
I'm not certain if it'll ever get added though, seeing as the issue was created on September 25, 2016, and is still open. The issue referencing it (SkriptLang/Skript#1634), does give an example of a possible way to add this which I personally think looks pretty good:

on <event> with priority <priority>:

We could probably implement something like this.

The main reason I made my suggestion of automatically detecting if we can flag an event is because it will improve scripts that already exist and ones that our made for SkriptLang or some other interpreter than this one, without the code needing to change.

But I still like the idea of a possible pre-parser/analyzer like you mentioned. So maybe we can put this on hold for now and if we have a pre-parser/analyzer in the future, we can revisit this idea.