Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Syntax Simplification #7636

Draft
wants to merge 7 commits into
base: dev/feature
Choose a base branch
from

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Feb 21, 2025

Description

Skript has always had this little method on Expression called simplify(), which has never been implemented. It's intended to take expressions and, if possible, simplify them to Literals or similarly simplify the code tree.
This PR attempts to properly implement that idea.

The PR revolves around the Simplifiable<S> interface. This interface defines the simplify(step, source) method that elements will implement. There's two critical ideas here. First, there are multiple steps of simplification. This arises from issues where elements may be able to be simplified at different points in time, like how ExprArithmetic can only be simplified once the arithmetic chain is fully formed. Second, every element should forward simplify calls to its children prior to attempting to simplify itself. This allows the element the best chance of being able to simplify.

Every element is simplified immediately after init() in SkriptParser. This step is the IMMEDIATE step, and is the simplest and most useful step. At this point, an element knows everything about its children and is fully initialized. Most elements can determine now whether they can be simplified or not. Some, like the aforementioned ExprArithmetic, cannot. This is where the following steps come into play.

The PARENT step is triggered by any parent element being simplified. When an element is simplified in the IMMEDIATE step, it should call simplify(Step.PARENT, this) on its children to indicate that the parent wants to simplify, and who the parent is. This gives the children a chance to see if they can now simplify and do so, allowing the parent to more aggressively simplify itself. Since each element should forward simplification attempts to its children, this allows for elements to wait for the right condition, and then simplify an entire stack. For example, ExprArithmetic will wait until it sees a simplify call from a parent that is not ExprArithmetic. It now knows that it has the full chain and can safely simplify itself. Its parent can then do the same, and so on.

Finally, the SCRIPT step is triggered after the script is fully parsed. This runs through all the elements one last time and gives them a final chance to simplify, given all the information that's available. I'm not certain this step is necessary, and I have not yet implemented it.

As of Feb 21st 2025, the main API is implemented and functional, though every single element with children needs an implementation of simplify(). This is ongoing.

One major downside with the current design is that the toStrings of element may no longer match what the user wrote. This could affect error messages, and I'll be testing to see what the impact is. The addition of SimplifiedLiteral sidesteps this issue by keeping a reference to the source expression.

In addition, this shifts work from runtime to parsetime and could have a significant negative impact if things are simplified just to be thrown out because a parent failed to parse. Parsetime impacts will be monitored for any significant performance loss.

Guide for Reviewers
This pr changes hundreds of files and would be challenging to review in completion. You can fairly safely ignore changes to all the expressions/conditions/effect classes, as they are all simple implementations of simplify() that just call simplifyChild on their children. The main meat of the review would be in the other classes.


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

Adds the API for simplification, implements it on some core expression classes and a few specific ones for testing.
@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Feb 21, 2025
@sovdeeth sovdeeth requested a review from a team February 22, 2025 20:57
@TheLimeGlass
Copy link
Contributor

TheLimeGlass commented Feb 23, 2025

Why are you adding this method to each element? Make it automatic when all you're doing in the newly added method is basic context like the expressions used in the syntax.

You'd have to make a new object that collects the expressions from the init like how RandomSK does it's collection of expressions per class.

I do not like the addition of a required override method, when this can be done simplier, pun intended.

The implementation classes look fine. How do you plan on dealing with UnparsedLiterals?

@sovdeeth
Copy link
Member Author

sovdeeth commented Feb 23, 2025

Why are you adding this method to each element? Make it automatic when all you're doing in the newly added method is basic context like the expressions used in the syntax.

You'd have to make a new object that collects the expressions from the init like how RandomSK does it's collection of expressions per class.

I do not like the addition of a required override method, when this can be done simplier, pun intended.

The implementation classes look fine. How do you plan on dealing with UnparsedLiterals?

I do fear that not all expressions should call simplify on all their children, but that can be handled by overrides. I didn't think of catching the expressions during init, that's a good idea. Wish i thought of that before spending 3 hours implementing them lol.

I don't plan on handling unparsed literals in any special capacity.

@TheLimeGlass
Copy link
Contributor

TheLimeGlass commented Feb 23, 2025

I do fear that not all expressions should call simplify on all their children, but that can be handled by overrides. I didn't think of catching the expressions during init, that's a good idea. Wish i thought of that before spending 3 hours implementing them lol.

I don't plan on handling unparsed literals in any special capacity.

The way RandomSK does it is actually a really good way of handling this problem, it was ahead of it's time. Unfortunately I think it would require an API change.

Also if an expression can't be simplified, do you think it would be worth it to return it as a String literal of it's toString ClassInfo?

@sovdeeth
Copy link
Member Author

I do fear that not all expressions should call simplify on all their children, but that can be handled by overrides. I didn't think of catching the expressions during init, that's a good idea. Wish i thought of that before spending 3 hours implementing them lol.
I don't plan on handling unparsed literals in any special capacity.

The way RandomSK does it is actually a really good way of handling this problem, it was ahead of it's time. Unfortunately I think it would require an API change.

Also if an expression can't be simplified, do you think it would be worth it to return it as a String literal of it's toString ClassInfo?

What do you mean? Why would a string literal be relevant?

@sovdeeth
Copy link
Member Author

The way RandomSK does it is actually a really good way of handling this problem, it was ahead of it's time. Unfortunately I think it would require an API change.

Unfortunately this method isn't feasible, after looking into it. Since simplify can return an entirely new reference, it has to update the actual variables being used in the class. If the list of expressions was stored and simplified, then that list of expressions would be valid, but the existing variables in the class itself that were assigned from that original list now point to possibly invalid expression objects. I don't think there's any nice way around this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants