[Wildcard Variables] DDC Implementation

[kallentu]

This issue tracks the work needed for the wildcard variables feature to be supported in DDC.
Spec: https://github.com/dart-lang/language/blob/main/working/wildcards/feature-specification.md

We may need to do some additional work to support multiple wildcard parameters in functions.

class A {
  int add(int a, int b) => 3;
}

class C implements A {
  int add(int _, int _) => 3;
}

cc. @sigmundch Feel free to tag or reassign as you see fit.
I'd also appreciate more details so we don't lose that context.

[sigmundch]

Thanks @kallentu

• the work here depends a lot on what kind of lowering we end up doing in the CFE
• the lowering will likely convert the wildcard variables into something, potentially a variable declaration that is never read.
• if the lowering uses the existing representation of variable declarations and guarantees uniqueness of variable names like today parameters have, then the backend may not need to do anything special for it to generate valid JavaScript. However, if the variable is represented by a new kernel node or a node that breaks current invariants, we may need some relatively small amount of work to emit valid JavaScript function declarations. The work could be similar to what DDC already does for temporary variables in Let expressions.
• Aside from implementing the feature, the debugger support may not need much work either. We need however to have consistent semantics in expression evaluation. If in the body of the method an expression to be evaluated uses _, we need to decide if it is rejected and, if not, what to generate in that case.

[nshahan]

Depending on how the CFE lowers the wildcards, and how we want them to appear (or not appear) in the debugger there could likely be some collaboration needed between DDC and DWDS to get it working.
cc @bkonyi

[kallentu]

If there's work for web debugging, I've created an issue to track that: #55752

[bkonyi]

I was under the assumption that the spec specified that wildcard variables won't be bound to a variable and that there will be no way to reference them. Is that not the case? Regardless, I don't think we want them to appear in the debugger, especially since we can have multiple wildcard variables in the same scope.

[sigmundch]

Correct - I think the nuance here is that in the code we emit in JavaScript we need to introduce a placeholder name to generate a valid function signature.

While our intent was to eventually make dwds model all of its state based on what we know from the dart program, there were cases in the past where it created its internal model based on the Chrome debugger's state of the application. If that is still the case today, we may need to do some work to ensure those synthetic variables are properly ignored/hidden.

[nshahan]

@natebiggs @kallentu

This test is now causing a compiler crash.
language/wildcard_variables/multiple/local_declaration_catch_rethrow_async_test

log:
https://dart-ci.appspot.com/log/ddc-hostasserts-linux-d8/ddc-hostasserts-linux-d8/101/language/wildcard_variables/multiple/local_declaration_catch_rethrow_async_test

The error makes it appear that the wildcard variable isn't being given a unique name by the CFE but I'm not sure because it is also touching some of the newly added async logic. Could one of you investigate?

[kallentu]

@natebiggs @kallentu

This test is now causing a compiler crash. language/wildcard_variables/multiple/local_declaration_catch_rethrow_async_test

log: https://dart-ci.appspot.com/log/ddc-hostasserts-linux-d8/ddc-hostasserts-linux-d8/101/language/wildcard_variables/multiple/local_declaration_catch_rethrow_async_test

The error makes it appear that the wildcard variable isn't being given a unique name by the CFE but I'm not sure because it is also touching some of the newly added async logic. Could one of you investigate?

I think the new async logic just changed the error produced. I haven't pushed the CL to make the names unique yet: https://dart-review.googlesource.com/c/sdk/+/373621

I'll submit it today though.

[kallentu]

I've submitted it now. I'll take a look at test results once they come in.

[biggs0125]

Yes, I saw this before I submitted. This was a RuntimeError before, we didn't have anything in the compiler that checked the uniqueness of names within a scope. So the test would compile fine and then throw when the JS got parsed since there was code of the form: function (_, _) {...} which is illegal.

Since the new async transform does some funky stuff with scopes and variable declarations, I added an assertion that now catches illegal declarations like that. So now it's a compile time error with compiler assertions on.

Thanks for the fix Kallen!

[kallentu]

1d52761 is submitted now. Seemed to land okay and the tests on DDC are now passing.

The CFE implementation is now done and if there's anything left for dart2js, ddc, you're free to run with it.

[kallentu]

And with this issue, is there any more work that needs to be done in DDC? Can we close the issue or are we still evaluating?
@nshahan @biggs0125

[biggs0125]

Thanks Kallen! We should just verify the behavior of these wildcard variables in the debugger. I think the thing to do here is to hide them but I'm guessing that's not happening by default.

I know there is some subset of synthesized variables that the debugger is already hiding by prefixing the variable name with a specific string. We can probably just use a similar pattern here.

[bkonyi]

Correct, we're not hiding these variables in the debugger when connected to a web app:

It also looks like we're not hiding them with native apps either:

[biggs0125]

Thanks for checking Ben! Do we want to:

1. Add logic to the debugger to exclude variables prefixed with _#wc? It seems like DDC is ascii encoding the # so that might make it more tricky.
2. DDC (and I'm assuming the VM) already has logic to prefix synthetic variables so they get ignored by the debugger. We could do the same thing here.

[nshahan]

I'm surprised that this regex isn't catching these but maybe I'm misunderstanding how it is used.
https://github.com/dart-lang/webdev/blob/main/dwds/lib/src/debugging/dart_scope.dart#L23

[bkonyi]

The VM actually marks these variables as invisible based on a flag provided in the kernel, so we don't rely on any sort of prefix to filter these variables out. However, I think we'll need to take the prefix approach for DWDS since we won't have access to the kernel.

As long as we don't send the variables in service protocol responses, the implementation details shouldn't matter too much.

[bkonyi]

I'm surprised that this regex isn't catching these but maybe I'm misunderstanding how it is used. https://github.com/dart-lang/webdev/blob/main/dwds/lib/src/debugging/dart_scope.dart#L23

I'm wondering if some of Kallen's changes hadn't rolled through to Flutter last time I tried this, but it looks like we're correctly omitting these variables for web applications now:

[nshahan]

Thanks @bkonyi! I'm going to close this out as work complete with no more known work expected. Anything that comes up now should be considered a bug.