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

SimplePropertyExpression fromType argument throws unparsed literal if not lower case #5452

Closed
1 task done
JakeGBLP opened this issue Feb 16, 2023 · 7 comments
Closed
1 task done
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. completed The issue has been fully resolved and the change will be in the next Skript update. priority: lowest "Nice to have" updates that are not required (tiny low impact bug fixes or QoL enhancements).

Comments

@JakeGBLP
Copy link
Contributor

JakeGBLP commented Feb 16, 2023

Skript/Server Version

[17:25:51 INFO]: [Skript] Skript's aliases can be found here: https://github.com/SkriptLang/skript-aliases
[17:25:51 INFO]: [Skript] Skript's documentation can be found here: https://docs.skriptlang.org/
[17:25:51 INFO]: [Skript] Skript's tutorials can be found here: https://docs.skriptlang.org/tutorials
[17:25:51 INFO]: [Skript] Server Version: git-Paper-384 (MC: 1.19.3)
[17:25:51 INFO]: [Skript] Skript Version: 2.6.4
[17:25:51 INFO]: [Skript] Installed Skript Addons:
[17:25:51 INFO]: [Skript]  - Lusk v1.0.0
[17:25:51 INFO]: [Skript]  - skript-reflect v2.3 (https://github.com/TPGamesNL/skript-reflect)
[17:25:51 INFO]: [Skript]  - SkBee v2.7.4 (https://github.com/ShaneBeee/SkBee)
[17:25:51 INFO]: [Skript]  - SkriptLanguage v1.2.0
[17:25:51 INFO]: [Skript] Installed dependencies: None

Bug Description

When registering a simple property expression, if the fromType argument of the register method is not lowercase (ie: object -> Object, block -> Block), it will throw an unparsed literal exception or whatever that is,

Expected Behavior

Skript should make it lowercase and also send a console message or something saying that it's a better practice to have it lower case to begin with.

Steps to Reproduce

public class ExprTest extends SimplePropertyExpression<Object, Float> {
    static {
        register(ExprSlipperiness.class, Float.class, "test", "Object"); // <--
    }

    @Override
    public @NotNull Class<? extends Float> getReturnType() {
        return Float.class;
    }
    @Override
    @Nullable
    public Float convert(Object o) {
        return Float.valueOf(10);
    }

    @Override
    protected @NotNull String getPropertyName() {
        return "test";
    }
}
  • I have read the guidelines above and affirm I am following them with this report.
@JakeGBLP
Copy link
Contributor Author

The code formatting is weird, i hope that's readable

@APickledWalrus
Copy link
Member

I'm not sure if case actually matters. The issue here is likely that you are using Object without explicitlyhandling UnparsedLiterals. To do this, you can call LiteralUtils#defendExpression for each object type expression. Then, to make sure it's still safe, return LiteralUtils#canInitSafely for init where the parameters are your defended expressions

@JakeGBLP
Copy link
Contributor Author

Does skript have a class where this happens? I think I happen to need to do this in a few of my classes

@TheLimeGlass TheLimeGlass added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. priority: lowest "Nice to have" updates that are not required (tiny low impact bug fixes or QoL enhancements). labels Feb 16, 2023
@TheLimeGlass
Copy link
Contributor

TheLimeGlass commented Feb 16, 2023

Should be addressed for SimplePropertyExpressions though. They should be needing to override the init method to handle the defending.

This is a fundamentally known issue. Njol would always use the getConverteredExpression to the Object.class which is essentially what the defend expression LiteralUtils methods do, but also account for expression lists. This is something that should not be needing to be done. At the minimum it shouldn't be needed in SimplePropertyExpression.

@JakeGBLP
Copy link
Contributor Author

Do I close the issue?

@TheLimeGlass
Copy link
Contributor

No

@TheLimeGlass TheLimeGlass added the PR available Issues which have a yet-to-be merged PR resolving it label Aug 2, 2023
@TheLimeGlass TheLimeGlass added completed The issue has been fully resolved and the change will be in the next Skript update. and removed PR available Issues which have a yet-to-be merged PR resolving it labels Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. completed The issue has been fully resolved and the change will be in the next Skript update. priority: lowest "Nice to have" updates that are not required (tiny low impact bug fixes or QoL enhancements).
Projects
None yet
Development

No branches or pull requests

3 participants