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

Fix ExprDifference issues #5406

Merged

Conversation

APickledWalrus
Copy link
Member

@APickledWalrus APickledWalrus commented Jan 28, 2023

Description

This PR aims to fix some issues, mainly with the initialization stage of ExprDifference.

There were multiple issues, but the main one (which is why #5331 had test failures) is that it does not properly compare known types. For example, if we had two types, string and number, the resolved class info is object (it's the shared superclass). Of course, this expression has handling for variables that allows object as a valid classinfo. However, this meant that the string and number difference would be accepted. This caused Skript to interpret the following line incorrectly in some cases:
difference between "%{_a}%" parsed as a number and "%{_b}%" parsed as a number
It should be interpreted as:
difference between ("%{_a}%" parsed as a number) and ("%{_b}%" parsed as a number)
However, because of the faulty checks, it would sometimes be interpreted as:
(difference between ("%{_a}%" parsed as a number) and "%{_b}%") parsed as a number

So, I've cleaned up the class, and the new logic is as follows:

If we have two known types:

  • Attempt to convert them to each other - fail if no conversions can be made

If we have one known type:

  • Attempt to convert object to the known type. I believe this will always succeed with new converters, but it also allows us to verify that the known type can actually have a difference operation performed on it

If we have no known types:

  • Resolve during runtime when we know types

In the PR I mentioned above, ExprParse was sometimes registered before ExprDifference which is why this error occurred. It's not problematic right now because ExprDifference is registered before ExprParse (but we shouldn't depend on load order to prevent bugs!!)

Thorough review of the logic and additional testing is likely needed


Target Minecraft Versions: Any
Requirements: None
Related Issues: none?

@APickledWalrus APickledWalrus added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jan 28, 2023
Copy link
Contributor

@kiip1 kiip1 left a comment

Choose a reason for hiding this comment

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

Thanks :)

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Nice PR my associate ⚡

@TPGamesNL TPGamesNL self-requested a review January 28, 2023 10:35
We will now attempt to convert into the type that has a math. If the first type has a math but the second type can't be converted, we will then try the second type's math
@TheLimeGlass TheLimeGlass self-requested a review February 16, 2023 13:44
@APickledWalrus APickledWalrus changed the base branch from master to dev/feature September 16, 2023 18:08
@APickledWalrus APickledWalrus added the needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. label Nov 8, 2023
@APickledWalrus APickledWalrus removed the needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. label Nov 22, 2023
@sovdeeth sovdeeth added 2.8 Targeting a 2.8.X version release feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. labels Dec 30, 2023
@sovdeeth sovdeeth merged commit 7d2a5b5 into SkriptLang:dev/feature Jan 1, 2024
4 checks passed
sovdeeth added a commit that referenced this pull request Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants