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

DDC: "undefined" leaked by conditional return breaks default arguments value #36652

Open
greglittlefield-wf opened this issue Apr 16, 2019 · 1 comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dev-compiler

Comments

@greglittlefield-wf
Copy link
Contributor

greglittlefield-wf commented Apr 16, 2019

We're running into some cases where, in DDC, a Dart function is returning undefined, and when that value is being passed into a function with conditional arguments, it's treating it as an unspecified argument.

Reduced test case

void main() {
  // Prints "arg: default value".
  fnWithDefaultArgValue();
  // Prints "arg: returned value".
  fnWithDefaultArgValue(fnWithConditionalReturn(true));
  // In DDC, unexpectedly prints "arg: default value".
  // In dart2js, prints "arg: null" as expected.
  fnWithDefaultArgValue(fnWithConditionalReturn(false));
}

void fnWithDefaultArgValue([dynamic arg = 'default value']) {
  print('arg: $arg');
}

dynamic fnWithConditionalReturn(bool shouldReturn) {
  if (shouldReturn) {
    return 'returned value';
  }
}

Compiled DDC:

  foo.fnWithDefaultArgValue = function(arg) {
    if (arg === void 0) arg = "default value";
    core.print("arg: " + dart.str(arg));
  };
  foo.fnWithConditionalReturn = function(shouldReturn) {
    if (dart.test(shouldReturn)) {
      return "returned value";
    }
  };

Here fnWithConditionalReturn is returning undefined when shouldReturn is false, causing the if (arg === void 0) check to pass, even though the argument was specified.

To help guard against other cases where undefined is leaked, could this instead check arguments.length? Or should most of those leaks get fixed instead?


Also, tangentially related, is there a reason these parameters are initialized via separate statements, as opposed to in ES6 default parameter values?

@todd-beckman
Copy link

I second the question about the default parameters. I believe that would be the preferable option, but I am not sure if there was a reason against it.

If there is a good reason not to use default parameters, the arguments.length approach is pretty straightforward. I have a PR for this here: https://dart-review.googlesource.com/c/sdk/+/99714.

@vsmenon vsmenon added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dev-compiler
Projects
None yet
Development

No branches or pull requests

4 participants