stefvanschie/quickskript

QuickSkript aims to be a Skript parser and executor with high speed

quickskript's Issues

Variable behaviour

Since I want to see this project succeed, I'll try to help with some specifications (since I know the project owner isn't a Skript user himself)

So, let's start with variables, shall we?!

Ok, first: Variables are persisted across (re-)loads.
This means that variables need to be stored somewhere. They will also need to be resolved to Psi elements before they're passed to the executor method.

Also, variables can be null, but, and I'll repeat it, but no nulls should be able to reach the executor methods.

Also, some types can be parsed from strings. These can implement their own parser.

Also, in case support for Skript addons gets added in the future, I'll leave this here:

Tests

A lot more tests should be added: while the codebase has been expanded, the count of tests hasn't increased. These tests should do more than just testing the individual PsiElements, but tests for those would also come in handy if someone has the patience to write them.

Addition of the Mockito framework should be considered.

TextMessage changes

Issues with its current state:

• It is essentially a wrapper around String: it can do nothing which wasn't possible if it were to store a String instance in O(N) time, but it has a higher memory requirement.
• It is mutable, but also a simple data type (see PsiStringLiteral). This could cause some issues if we are not careful enough.
• The naming doesn't always fit its function: for example, the PsiWorldFunction class uses this class to store the name of a World - something that is not a message.

Proposed changes:

• Make it a class which just stores a String instance and rename it to PsiString. An alternative is to have it as a utility class and use String instances instead.
• Make it immutable, create an #add(String) method which returns a new class instance.
• Make the parsers, getters able to convert alternative color code chars into real color codes and vice versa.
• Use String functions instead of regex to make the methods have a time complexity of O(N). Can use the static methods of the ChatColor enum whenever possible.

Don't shade the JetBrains annotations for Bukkit

There are two reasons for this:

• they are available at runtime (Bukkit/Spigot/Whatever shades them in)
• we are not using any of their runtime features

Is shading them into core necessary? Maybe, I don't know.

Incorrect license in style_guide.md

The "style_guide.md" file contains the following line:

This project uses The Unlicense license, therefor no license or copyright information at the top.

That statement is false, since the porject uses the MIT license, as seen in the "LICENSE" file.

Custom syntaxes and reflection

From what I've seen, this is a pretty promising project, but it's going to take a while to implement all the features Skript has.

Based on that, I'm suggesting adding something similar to the skript-mirror addon, which allows you to use Java code inside the script, and create custom syntaxes that can be used easily.

Github and docs:
https://skript-mirror.gitbook.io/docs
https://github.com/btk5h/skript-mirror

I have no idea how complicated (or if it's possible) to implement this suggestion. If you liked the suggestion, I can give more ideias/details about it.

Thanks.

Unified factory and class type registration

Instead of exposing PsiElementFactory's CLASS_TYPES variable through a getter, a method should be added to the PsiElementFactory class which can be used to register new function factories. This method should take the function factory's instance, the function's Class and the function's returned Class as parameters. This would make the registration of new functions easier since the registration would only have to be done at one place, leaving less room for errors. In case this method is not private, this would also allow easy add-ons.

Improved QuickSkript version acquisition

PsiSkriptVersionExpression currently calls getClass().getResourceAsStream("/app.properties") in its constructor to acquire the QuickSkript version. I see two slight issues with this:

• It is done in the constructor, therefore executed once for each element instance, while it really only needs to be executed once. It is not a huge issue, since this is definitely not "hot" code, but still suboptimal and triggers me. It could be done in a static block instead, or even better, it could be exposed through a getter in some central class that the core module has.
• The app.properties file can be found in the core module. This module is designed to be shaded into some other jar. What if that other jar actually must use this generic file /app.properties for some reason, eg. a framework? I suggest placing the file in a directory or renaming it, but I would rather research how these situations are usually handled first.

Multiple skript pattern matches

The PSI currently has issues in the way it tries to match a given element. Given a factory which will produce children the skript pattern may incorrectly assume which characters in the input belong to which element. Given a skript pattern as follows %type%[a] and the text somethinga, the matcher may incorrectly assume that somethinga should be part of the %type% part and that the [a] is in this case omitted. In this case the PSI will attempt to further match somethinga across all factories and may end up not finding any. It will assume the provided text was incorrect and stop the parsing altogether. It doesn't, however, try a different possibility for matching the pattern in the hope it would lead to a correct result. In this case the alternative pattern would be one where the %type% would be occupied by something and [a] by a. The something which is now further parsed in the PSI may have yielded correct results. A more concrete example of this is the ban effect: ban %texts/offline players% [(by reason of|because [of]|on account of|due to) %text%]. Providing the text ban the player because "reason" will incorrectly assume that the player because "reason" is a valid player, while in fact the player is the player we're looking for and everything else is part of the optional group at the end.

Currently the greedy option is used as a quick fix to change how pattern matching behaves in the hope that changing this option on some patterns fixes this issue. This has some issues however. First of all, you'd have to figure out which patterns should be marked as greedy and which ones shouldn't. Second of all, the actual pattern we're looking for may not be matched even with the different greedy options.

It'd be better to have a way to get all possible matches for a given input and attempt all of these combinations to see which one matches. That way we don't have to specify a greedy option nor do we have to worry about possible inputs not being matched correctly at all - all correct combinations are tested, so every possible combination will be tested and matched if a correct one is possible.

The reason this is tagged as help wanted is because I'd like advice in a way how we can get all these matches in a sort of clean matter. The current pattern matching already has a bunch of structural oddities and readability is difficult. Fixing these issues while also providing an option to get all matches would be beneficial.

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

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.

Including relevant information in parse/execution errors

Currently, only the actual cause of the exceptions and their source file are logged. This may not help all that much when trying to debug the issue. More relevant information, eg. the file section / line number or the event / command name should be considered to be included.

SkriptLoader instances, less static

Currently, the static modifier is used even in singleton factory instances (for the Patterns). This makes it nearly impossible to later get rid of all these unnecessary variables. Why would we want to get rid of them, why are they unnecessary? All Skript plugins are loaded at startup, there is no need to keep all of the factory instances in memory after the loading is done.

These singleton (but partially static) factories are collected in static utility classes. Instead of this, I propose to make a SkriptLoader instance which would hold the factory instances, instead of having them as static. SkriptLoader instances might even offer easier expandability if Skript libraries are to become supported. They could also be cached or re-created for whatever reason (eg. loading more Skripts after the initial startup) - allowing more flexibility for both QuickSkript and other plugins dependent on QuickSkript.

In case this proposal gets a green light, I will gladly get this done. I would just rather get a response before spending time making these changes.

Event handling change proposal

The handling of Bukkit events is simple: you either call Skript event(s) or you don't. In case of some Bukkit events, this is even simpler: if the event fires and there is a registered Skript event, it will always get called.

Registering these kinds of simple events has been made simpler in a recent commit of mine, but this could be further improved: there is no need for a proxy object, the AbstractEvent.

In case of simple events, Bukkit event classes can be mapped to a collection SkriptEvent instances. A single event listener can execute all SkriptEvent instances. In case of non-simple events, the Bukkit event classes could be mapped to a collection of SkiptEvent, Predicate<? extends Event> pairs. The SkriptEvents would only get executed if their Predicate pairs return true.

This would get rid of the AbstractEvent class, make registering events simpler, improve performance by a non-benchmarkable amount all without (hopefully) losing any features.

PS: In case I get a green light, I will gladly get this done. I would just rather get a yes/no answer before spending time with this.