Skip to content

Syntax Simplification #7841

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

Merged

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Apr 30, 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 via a Simplifiable interface. A config option is added to allow users to opt out if issues arise.

Every element is simplified immediately after init() in SkriptParser. In addition, it may be called at any time by parent elements, though it should be ensured that no other objects maintain critical references to the element, as simplify may return new objects.

ExprArithmetic is a special case that can only be simplified once all elements in the chain have been parsed and ordered, so it requires checking the ParsingStack to see if it's the top-level element in the chain. If so, it can then call simplify recursively on all elements in the chain.

Todo:

  • Properly simplify ConvertedExpressions. The existing method was stripping the converting part off a lot of converted expressions.
  • Implement simplify on many more expressions that could support it.

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

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Apr 30, 2025
@sovdeeth sovdeeth requested review from UnderscoreTud and a team as code owners April 30, 2025 21:12
@sovdeeth sovdeeth requested review from Romitou and removed request for a team April 30, 2025 21:12
@sovdeeth sovdeeth mentioned this pull request Apr 30, 2025
@sovdeeth sovdeeth requested a review from APickledWalrus April 30, 2025 21:24
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far. Do you think Arithmetic tests that verify the SyntaxElement is a Literal would be possible?

@sovdeeth sovdeeth requested a review from APickledWalrus April 30, 2025 22:45
@sovdeeth sovdeeth added the 2.12 Targeting a 2.12.X version release. Project ID: 7 label May 2, 2025
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good. I might prefer to see the classes of the new simplification package under ch.njol. Mainly, given that rewritten classes are possible in the future, it'll make compatibility easier to handle

@sovdeeth sovdeeth requested a review from APickledWalrus May 14, 2025 01:01
@sovdeeth sovdeeth moved this to In Review in 2.12 Release May 14, 2025
@sovdeeth sovdeeth marked this pull request as draft May 14, 2025 21:38
@sovdeeth sovdeeth requested a review from APickledWalrus May 20, 2025 03:15
@sovdeeth sovdeeth moved this from Awaiting Merge to Needs Reviews in 2.12 Release Jun 3, 2025
@sovdeeth sovdeeth removed this from 2.12 Release Jun 5, 2025
@sovdeeth sovdeeth moved this to In Review in 2.12 Release Jun 5, 2025
Copy link
Contributor

@Burbulinis Burbulinis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, although aren't there more expressions that can implement simplify under org/skriptlang/skript/?

@Override
public String toString(@Nullable Event event, boolean debug) {
if (debug)
return "[" + sourceExpr.toString(event, true) + " (SIMPLIFIED)]";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we mention what it was simplified to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would get rather confusing, the toString's already rather busy.

@skriptlang-automation skriptlang-automation bot added needs reviews A PR that needs additional reviews and removed needs reviews A PR that needs additional reviews labels Jun 9, 2025
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Re-request after conflict resolution for approval 🙂

@sovdeeth sovdeeth requested a review from APickledWalrus June 27, 2025 04:04
@github-project-automation github-project-automation bot moved this from In Review to Awaiting Merge in 2.12 Release Jun 30, 2025
@sovdeeth sovdeeth added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. and removed needs reviews A PR that needs additional reviews labels Jul 1, 2025
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label Jul 1, 2025
@APickledWalrus APickledWalrus merged commit d725212 into SkriptLang:dev/feature Jul 1, 2025
5 checks passed
@skriptlang-automation skriptlang-automation bot added the completed The issue has been fully resolved and the change will be in the next Skript update. label Jul 1, 2025
@github-project-automation github-project-automation bot moved this from Awaiting Merge to Done in 2.12 Release Jul 1, 2025
@skriptlang-automation skriptlang-automation bot removed needs reviews A PR that needs additional reviews feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. labels Jul 1, 2025
@sovdeeth sovdeeth linked an issue Jul 3, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed The issue has been fully resolved and the change will be in the next Skript update. enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Optimizations of redundant syntaxes
3 participants