Skip to content

feat: support more types in switch statements #2926

New issue

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

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

Already on GitHub? # to your account

Merged
merged 5 commits into from
Jun 3, 2025

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented May 30, 2025

Fixes #2919
Fixes #648

Changes proposed in this pull request:

  • Fixes compiler error when using a switch statement with certain types.
    • All numeric types that were already supported
    • Add support for strings, floats, and 64-bit ints
  • Adds tests to validate

I've reused the existing equality comparison logic, so any behavior for == is retained for switch cases, including overloads, etc.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

as does not use string pool..so it is impossible to compare two string directly with ptr. string.eq must be used for each check.

@mattjohnsonpint
Copy link
Contributor Author

as does not use string pool..so it is impossible to compare two string directly with ptr. string.eq must be used for each check.

... oh. I see. I was thrown off because the naive test passed, but this one fails:

assert(doSwitchString("o" + "n" + "e") == 1);

because the concatenated resulting string is in a different variable and thus a different pointer than the "one" in the switch case.

I'll add a special case for strings. I think it should be good for the rest though.

@mattjohnsonpint mattjohnsonpint marked this pull request as draft May 30, 2025 23:27
@CountBleck
Copy link
Member

I wonder if this should just be using the == operator overload for all objects that have them...

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented May 31, 2025

Working on a revision. I think I can reuse some existing code.

I found that string equality isn't handled by the TypeKind.String case of makeEq, but rather by the __eq function that's implemented as an operator overload for == on the String class.

Much of the logic to determine whether to call makeEq or invoke an overload is already present in compileCommutativeCompareBinaryExpression, but I need to split left from right to do this correctly in a switch statement, since the "left" is compiled once and shared across switch cases.

@mattjohnsonpint mattjohnsonpint marked this pull request as ready for review May 31, 2025 20:11
@mattjohnsonpint
Copy link
Contributor Author

This is now updated with the revisions. I've split the existing compileCommutativeCompareBinaryExpression method such that the core logic can be re-used when evaluating the switch cases. I've also added more tests to show that it works as expected. Thanks.

@mattjohnsonpint
Copy link
Contributor Author

@HerrCai0907 any other adjustments you would like?

Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

LGTM.

@HerrCai0907 HerrCai0907 requested a review from CountBleck June 3, 2025 02:59
@HerrCai0907 HerrCai0907 changed the title fix: support more types in switch statements feat: support more types in switch statements Jun 3, 2025
Copy link
Member

@CountBleck CountBleck left a comment

Choose a reason for hiding this comment

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

LGTM. And wow, these method names (compileCommutativeCompareBinaryExpressionFromParts) are starting to get longer and longer 😛

@CountBleck CountBleck force-pushed the more-switch-types branch from 153ca02 to 9029772 Compare June 3, 2025 22:54
@CountBleck CountBleck merged commit 3defefd into AssemblyScript:main Jun 3, 2025
14 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strings aren't supported in switch statements Support more types in switch conditions
3 participants