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: return the assigned value in assignments #2773

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented Oct 17, 2023

In TS, assignment chain will assign result to each variable. But in AS, assignment chain will assign result to second variable and assign the value of second variable to the first variable.
It will cause incompatibility problem.
This patch assign result to a temporary local and assign it to each variable.
Fixes #2770.

In `makeAssignment`, `valueType` doesn't match witch valueExpr.
But there are no actual influence for current generated wasm.
@HerrCai0907
Copy link
Member Author

deps: #2772

@HerrCai0907 HerrCai0907 force-pushed the 2770-assignment-chain-cause-compatibility-issue branch from dc9df96 to f40e4a3 Compare October 17, 2023 09:30
@HerrCai0907 HerrCai0907 force-pushed the 2770-assignment-chain-cause-compatibility-issue branch from f40e4a3 to a18920f Compare October 18, 2023 03:22
@HerrCai0907 HerrCai0907 force-pushed the 2770-assignment-chain-cause-compatibility-issue branch from a18920f to b755426 Compare October 18, 2023 03:39
@HerrCai0907
Copy link
Member Author

WDYT @CountBleck @dcodeIO @MaxGraey ?

@HerrCai0907
Copy link
Member Author

ping @CountBleck

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.

Everything seems good here

return this.makeAssignment(
target,
this.convertExpression(valueExpr, valueType, targetType, false, valueExpression),
Copy link
Member

Choose a reason for hiding this comment

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

This code was better before, but this is okay.

@CountBleck CountBleck changed the title fix: avoid use getInstance in assignment chain fix: return the assigned value in assignments Nov 7, 2023
@HerrCai0907 HerrCai0907 merged commit 94fa207 into AssemblyScript:main Nov 8, 2023
13 checks passed
@HerrCai0907 HerrCai0907 deleted the 2770-assignment-chain-cause-compatibility-issue branch November 8, 2023 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assignment chain cause compatibility issue
2 participants