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

[Wildcard Variables][spec] Allow unnamed optional parameters to not have default value. #3807

Closed
lrhn opened this issue May 15, 2024 · 9 comments · Fixed by #3880 or #3938
Closed

[Wildcard Variables][spec] Allow unnamed optional parameters to not have default value. #3807

lrhn opened this issue May 15, 2024 · 9 comments · Fixed by #3880 or #3938
Assignees
Labels
feature Proposed language feature that solves one or more problems wildcard-variables The wildcard-variables feature

Comments

@lrhn
Copy link
Member

lrhn commented May 15, 2024

With "wildcard-variables", a parameter can have no name.
For example void foo(int _) { ... } does not introduce a name in the parameter scope, the parameter value is inaccessible.

Or stated differently: The parameter does not introduce a variable.

The purpose of optional parameter default values is to ensure that a parameter variable has a value when it's read, even if no argument value was passed. A parameter which cannot be read or used in any, one where there is no variable at all, way does not need a default value to initialize a variable.

Proposal: It is not a compile-time error for a optional non-initializing non-super parameter to not have a default value, even if its type is not nullable.

The existing clause is:

It is an error if an optional parameter (named or otherwise) with no default
value has a potentially non-nullable type except in the parameter list of an
abstract method declaration.

(Or of a forwarding factory constructor, they also cannot declare default values. And come augmentations, it will only apply to the fully augmented declaration, not the individual steps.)

For wildcard variables only, this should be changed to:

It is an error if an optional parameter (named or otherwise) with no default
value has a potentially non-nullable type except in the parameter list of an
abstract method declaration or a forwarding factory constructor, or if the
declared parameter name is _ and the parameter is not an initializing
formal or super parameter.

(If we have a better way of phrasing that a declaration is non-binding, we can use that instead of "the declared ... name is _".)

We have to exclude initializing formals, this._, and super parameters, super._ ifwhen we allow those, because the value of that argument is used for something, even if it cannot be referenced.
(If we make it possible to forward not having an argument, then super._ can safely be forwarded to another optional positional parameter without having a default value, it will just forward the absence of an argument and let the super-constructor use its default value. Maybe we should just do that anyway! Sub-proposal:

If an optional super-parameter corresponds to an optional parameter of the super-constructor, and no argument is given to
the super-paramater, then no argument is passed to the super-constructor either. It's a compile-time error if such an
optional super-parameter with an optional target has no default value, is potentially non-nullable, and the variable it
introduces in the parameter scope is referenced. Otherwise the variable will use the default value if no argument was
passed, even if no value is forwarded to the super constructor.

And a _-named parameter is never referenced, so that just works. As long as we can only omit trailing positional parameters, and we cannot combine explicit positional arguments to the super-constructor with positional super-parameters, the forwarding will also work correctly.
Or something.)

@lrhn lrhn added feature Proposed language feature that solves one or more problems wildcard-variables The wildcard-variables feature labels May 15, 2024
@scheglov
Copy link
Contributor

scheglov commented May 16, 2024

And come augmentations, it will only apply to the fully augmented declaration, not the individual steps.

When you say "fully augmented declaration", how does this reconcile with "The function augmentation specifies any default values. Default values are defined solely by the original function." from the augmentation specification? As it is specified now, it seems that you provide default values only on the original declaration, not in any other steps. Do we plan to change this?

@lrhn
Copy link
Member Author

lrhn commented May 16, 2024

As currently specified, its definitely the case that:

It is an error if an optional parameter (named or otherwise) with no default value has a potentially non-nullable type except in the parameter list of an abstract method declaration.

does not apply to augmenting method declarations, since they cannot declare default values, and they must declare parameters with the same optionality and type as the augmented declaration.

Then there are two ways to look at where the rule does apply:

  1. It's an error if a non-augmenting (base) syntactic declaration has such a parameter.
  2. It's an error if a semantic declaration (the result of applying all augmenting declarations to the base declaration) has such a parameter.

With the current specification, I'd say that the declaration you get from applying an augmenting method declaration to another method declaration, the augmented declaration (which is a base declaration or the result of applying some prior augmenting declarations to a base declaration), inherits the default values of the parameters of the augmented declaration.
If that's the model, then it doesn't matter which of the perspectives above we use, because the parameters of the augmentation result have the same types, same optionality, and same default values as the base declaration after every augmentation step.

I prefer the second view. I do so because I don't think it's a given that we will keep the "augmentations cannot add default values" rule. And because I think it's important that we use that perspective in cases where there is a difference, and that is the more useful perspective.

If we change the rule for default values, we can still say that an augmentation cannot change a default value, but if a base declaration has a parameter [Foo x], which would not even be valid without a default value, then I think it's perfectly reasonable to allow an augmenting declaration to add a default value, as [Foo x = const _NullFoo()]. No change, just filling in the blanks.
If we do that, then there should be no error unless the fully augmented declaration still has no default value. Any prior step in the augmentation chain, starting at the base declaration, doesn't have to be complete.

In general, I think most of the "completeness rules" we have for declarations (method of non-abstract class must have a body, non-nullable optional parameter must have default value, non-redirecting generative constructor must initialize all final fields, probably more), should only apply to the "fully augmented" declaration. That's where we require completeness.

Some of our rules for syntax are to avoid conflicts locally, or overspecifying (for example: a function declaration must not have two parameters with the same name, or a non-optional parameter must not have a default value), other rules are completeness rules like the ones above.

With augmentations, we should apply the conflict-like rules to every syntactic declaration, and the completeness rules only to the complete, fully augmented, semantic declaration.

The big task is to figure out which rules are which! (There may be some arguments there - for example I'd consider the (now rejected) "don't implement the same type twice" a local conflict-like rule, and wouldn't mind different augmentations adding the same interface, but I'm not sure everybody would agree.)

@kallentu
Copy link
Member

Proposal: It is not a compile-time error for a optional non-initializing non-super parameter to not have a default value, even if its type is not nullable.

I'm fine with this change. Seems to make sense and if it doesn't once we come to implement it, we'll revisit.

cc. @dart-lang/language-team if anyone else has any other opinions.

@kallentu kallentu changed the title Allow unnamed optional parameters to not have default value. [Wildcard Variables][spec] Allow unnamed optional parameters to not have default value. May 29, 2024
@kallentu kallentu self-assigned this Jun 4, 2024
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jun 7, 2024
…o default value.

Bug: dart-lang/language#3807
Change-Id: Ibeb29d3702b74379b64e8965c3ef9709c7bf2f41
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/369780
Commit-Queue: Kallen Tu <kallentu@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
@rakudrama
Copy link
Member

Should this be a valid method definition?

void foo([Uint8List _]) {}

You can't change _ to x without providing a default value, and as there is no valid default value since no constant of the type can be contrived. It seems odd to permit a method that can only have a wildcard parameter.

If the above is valid, how about this?

void bar([Never_]) {}

Notes on implementation for dart2js

dart2js implements argument defaulting for instance methods (and static methods that have tear-offs) by having multiple entry points (stubs) that add the missing arguments and then call the primary entry point (which expects all the arguments). The primary entry point does type checks (potentially checking the defaulted value). This avoids sentinels and a whole raft of js-interop problems with the JavaScript sentinel value undefined. We could, in principle, improve performance of spec-compliant code by adding more entry points and / or moving checks into the argument-defaulting stubs but that increases code size.

It is not clear how to modify the default-then-check pattern for wildcards. I suppose we could add a sentinel value that is outside the Dart type system, and modify the entry checks (i.e. _ as Uint8List) to first check the sentinel. At some point in the program, _ has a type like union(sentinel,Uint8List). We recently started compiling some argument-defaulting stubs via the full SSA compiler, so to work properly, the sentinel value and its type would need to play well with the rest of the type system and type-based optimizations. We can't just hack it to 'pass zero, and check for zero as a sentinel'.

It is this second part, making sure we can represent the possibility of a sentinel in the dart2js internal type system, that makes implementing "unnamed optional parameters without a default value" a large work-item for dart2js. We can do the work, but is the feature worth the effort?

I suggest initially that unnamed optional parameters with a non-nullable type require a default value.

@leafpetersen
Copy link
Member

Should this be a valid method definition?

void foo([Uint8List _]) {}

You can't change _ to x without providing a default value, and as there is no valid default value since no constant of the type can be contrived. It seems odd to permit a method that can only have a wildcard parameter.

I think there's pretty broad interest in allowing all parameters to have non-constant defaults. In any case, yes I think our general perspective is that this should be allowed. If I'm overriding a method, and it has three optional parameters, and I don't care about the first two, why force me to list out a default for them if I'm never going to use them?

It is not clear how to modify the default-then-check pattern for wildcards.

Given the constraints you describe above, what about making the primary entry point expect not all of the arguments, but rather all of the non-wildcard arguments. You would then (in spec compliant mode) need to move any type checks for the wildcard arguments only into the stubs. This has a code size cost, but only in spec compliant mode, and only when the feature is used, making it pay as you go. Would this help?

I suggest initially that unnamed optional parameters with a non-nullable type require a default value.

I am open to reconsidering this if really necessary, but I'm also somewhat resistant to designing the language around the particulars of this calling convention. Each individual decision may seem small, but cumulatively they weigh on us as language tech debt.

@rakudrama
Copy link
Member

I think there's pretty broad interest in allowing all parameters to have non-constant defaults.

If the changes to support [Foo _] are seen as part of the cost for non-constant defaults, then I don't mind, but would want a clearer idea of 'where the puck is headed'.

What worries me about non-constant defaults is that tends to force an implementation strategy of defaulting inside the method. It becomes harder to make a call fast because of what you know at the call-site, since the defaulting is constrained to happen in the scope of the parameters or instance.

In any case, yes I think our general perspective is that this should be allowed. If I'm overriding a method, and it has three optional parameters, and I don't care about the first two, why force me to list out a default for them if I'm never going to use them?

It is not clear how to modify the default-then-check pattern for wildcards.

Given the constraints you describe above, what about making the primary entry point expect not all of the arguments, but rather all of the non-wildcard arguments. You would then (in spec compliant mode) need to move any type checks for the wildcard arguments only into the stubs. This has a code size cost, but only in spec compliant mode, and only when the feature is used, making it pay as you go. Would this help?

Not really, but I didn't mention all the constrains. Currently the main entry point, since it does checks, is also the dynamic call entry point and the Function.apply entry point. These two work differently in how they default optional arguments - at a dynamic call site we still statically know the 'selector', i.e. shape of the arguments, so we call the correct stub.

In Function.apply we don't know the selector statically, so we call the main entry. This requires assembling the missing arguments from metadata. We can't move the checks into the metadata easily since they can depend on instance type variables. Once a program contains Function.apply (i.e. most large applications), about half of the closures escape enough to appear to potentially be applied, so increasing the size of the Function.apply metadata for optional arguments is a large burden, even if only a few closures are actually passed to Function.apply.

I suggest initially that unnamed optional parameters with a non-nullable type require a default value.

I am open to reconsidering this if really necessary, but I'm also somewhat resistant to designing the language around the particulars of this calling convention. Each individual decision may seem small, but cumulatively they weigh on us as language tech debt.

I agree that "unnamed optional parameters do not need a default value" is a nice feature.

But we could add it later. If we add it later, none of the back-ends need to handle it now (wasm is also broken) and wildcard parameters behave just like ordinary parameters plus the static constraint that they cannot be used. That is what users need to write today. How many potentially-unnamed parameters are actually optional? I see lots of compelling examples for required arguments but not so many for optionals.

@lrhn
Copy link
Member Author

lrhn commented Jun 26, 2024

I agree that the combination of being positional, optional and non-nullable is rare, and wanting to ignore the value of an optional parameter that is not nullable is exceedingly rare. (It's there for a reason, and it's not nothing (not null), so ignoring it seems like it's going to miss some significant semantics.

It may be mostly in tests that it turns up. Maybe only in tests of wildcards (there's a reason I stumbled on it).

I'd like the feature, because it feels right.
It feels off to have to provide a default value that is never going to be used for anything, just to satisfy a compiler.
If it's hard to implement on a shorter schedule, we can delay it, but I don't want to postpone it indefinitely.

(But then, we've postponed #1076 for 11 years, and it should really have been there since day 1. It's always been implementation constraints that have gotten in the way, not that it's not an awesome idea. This one is far from as impactful.)

@kallentu
Copy link
Member

During our weekly meeting, the language team has decided to remove this from the spec.
I'll do the relevant work to remove the tests and implementation.

We can perhaps do it in a future release, but knowing our track record, we probably won't get to it.

@leafpetersen
Copy link
Member

Not really, but I didn't mention all the constrains.

I think my proposal handles dynamic calls fine (you have all of the same selectors as before with all of the same semantics as before), but I don't entirely follow the constraints around Function.apply. In any case, we discussed this in the language team meeting this morning and agreed to remove it from the spec for this feature.

Decision: we will not allow unnamed optional parameters to not have a default value in this feature release. We will file an issue to consider it as a future feature, possibly as part of non-const default values

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jul 29, 2024
…al parameters for wildcards.

Reverting https://dart-review.googlesource.com/c/sdk/+/371560.

Bug: dart-lang/language#3807
Change-Id: I3933d7d6117b90cc0a091c8e3ce1e5bcf8061ec0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/377722
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Kallen Tu <kallentu@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems wildcard-variables The wildcard-variables feature
Projects
None yet
5 participants