-
Notifications
You must be signed in to change notification settings - Fork 102
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
disable side effect in default parameter. #327
Comments
I was trying to fix tuchida@8770306 . I found two problems. First. This code fails to compile when add static function f1(a : number, b : number = ++a) : void {
log a;
log b;
} Second, It is not expected in the case of closure has side effects. I do not know how to deal with this case. static function f2(a : number, b : () => number = () => ++a) : void {
log b(); // expect 2
log a; // expect 2 but 1
} Do you have a good idea? |
Thank you for looking into the issue. IMO we should not allow the modification of preceding arguments within the default argument expression. Or please let me know if you have any reason to support such behaviour. |
For example. class _Main {
static function fn(a : number, b : () => number = () => a) : void {
log b(); // expect 1
a = 10;
log b(); // expect 10 but 1
}
static function main(args : string[]) : void {
_Main.fn(1);
}
} If not allow the modification of |
It might work if the output JavaScript code like this. class _Main {
static function fn(a : number, b : () => number = () => a) : void {
log b(); // expect 1
a = 10;
log b(); // expect 10 but 1
}
static function main(args : string[]) : void {
_Main.fn(1);
_Main.fn(10, () => 20);
}
} var NO_PARAM = {};
function _Main$fn$N(a, b) {
if (b === NO_PARAM) {
b = function () {
return a;
};
}
console.log(b());
a = 10;
console.log(b());
}
function _Main$main(args) {
_Main$fn$N(1, NO_PARAM);
_Main$fn$N(10, function() {
return 20;
});
}; Can decreased number of function calls. But many changes you will probably need. |
The answer is no, practically, since the feature has only been introduced lately.
Such design was considered when we introduced the default arguments, but had been dismissed for two reasons: that the model is complicated, and that it would be slow. Current design will run fast together with the inline optimizer, since the default argument expressions can be inline-expanded in the caller side by the optimizer. |
Thanks!
How do you like tuchida@bbaabac ? |
Thank you for looking into the issue. What I mean is that modification of preceding arguments should disallowed only within the default argument expressions, and it should continue to be allowed at other locations. i.e.
should be considered valid, OTOH
should be invalid. |
This case. static function fn(a : number, b : () => number = () => a) : void {
log b(); // expect 1
a = 10;
log b(); // expect 10 but 1
}
static function main(args : string[]) : void {
_Main.fn(1);
}
Or should disallowed modification of 'a', because it is referenced in the default argument expressions? |
Thank you for the clarification.
IMO Below is a valid example using default argument expression that prints
|
Having said that, it might not be necessary to disallow the modification of preceding arguments within default argument expressions. Just documenting the behavior might be sufficient. |
The very interesting case of override method. |
The override example illustrates the feature of the compiler that is already being used. That is why I believe it is better to consider the default argument expressions as something that is expanded at the caller-side which in turn means that the return value of |
jsx#327 (cherry picked from commit bbaabac) Failed to compile when `bin/jsx --minify t/run/400.constant-param- within-default-argument-expressions.jsx`. The minifier count reference to the original `ArgumentDeclaration`, and not clone but I think need replace `LocalVariable` within `LocalExpression` like `MemberFunctionDefinition#clone`.
I run this JSX's code.
I expected to output
2
, but1
.(Sorry. I missed #326)
This is compiled JavaScript code.
I think it should be this.
The text was updated successfully, but these errors were encountered: