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 super._? #3792

Closed
Tracked by #55661
kallentu opened this issue May 9, 2024 · 10 comments
Closed
Tracked by #55661

[Wildcard Variables][spec] Allow super._? #3792

kallentu opened this issue May 9, 2024 · 10 comments
Assignees
Labels
specification wildcard-variables The wildcard-variables feature

Comments

@kallentu
Copy link
Member

kallentu commented May 9, 2024

Let's talk about what we want to do with super._ in this issue.

Previous Discussion

I just checked, and the icky thing I disagree with is disallowing super._.

We should allow it.
If it breaks the current desugaring specification of super parameters, then define super._ as introducing a fresh name instead of no name.

Originally posted by @lrhn in #55661

I added the rule that super._ is an error because the desugaring will turn it into code that doesn't work (or code that does something completely different! - if _ in super(..., _, ...) will now denote a static or top-level declaration).

We could try to enable super._ by introducing an implicit renaming step that makes it possible for the superinitializer to denote this parameter. However, I think that would be an anomaly, and it is yet another exception to remember while coding.

So I'd prefer that we keep super._ as an error. Developers may then do the rename themselves. First case, it's in the same library:

// Current code. Breaks because `super._` will be an error.



class A {

  final int _;

  A(this._);

}



class B extends A {

  B(super._);

}
// Migrated code. No problem, no rename lints, no code outside this library can know.



class A {

  final int __;

  A(this.__);

}



class B extends A {

  B(super.__);

}

Other approaches can be used as well, of course. For example, the initializing formal in A could be turned into a normal parameter with some other name (say, __), such that the name of the instance variable wouldn't have to change, and we'd have A(int __) : _ = __;.

In the other case, where A and B are declared in two distinct libraries, we might introduce a lint message which would then need to be ignored:

class OtherB extends A {

  OtherB(super._);

}
class OtherB extends A {

  // ignore: matching_super_parameters

  OtherB(super.__);

}

In general, we will continue to allow developers to use _ as the name of some declarations. But I do not think we should bend over backwards to make it convenient to do so, at the expense of the consistency and comprehensibility of these parts of the language.

Originally posted by @eernstg in #55661

I think we should allow super._ because I don't expect users to expect an exception, so it not working is a surprising exception to them.

If the author doesn't use the name of a positional super.x parameter, it's reasonable to assume that changing it to super._ is fine. After all, they can change it to super.y or super._arglebargle without any problem.

The reasoning that the specification uses the name is meaningless to the user. Can't it just not do that? Yes, yes it can. It's an accident of specification that the name is used, not anything inherent to the feature. It used to be a safe assumption that the name given to the parameter would still be in scope and refer to the same value at the point of the super constructor invocation, so the specification chose to specify the meaning as a desugaring to an invocation that references the parameter variable. That's no longer safe if we allow super._, but that shouldn't break the wildcard feature by disallowing a meaningful use, it just means that the specification of super parameters shouldn't make that assumption any more, and should be rewritten to work anyway.

We can make it work. How we do that is up to us, it shouldn't matter to the user that we wrote a specification for super parameters that wasn't forward compatible. That's our problem, and we should solve it, and allow super._.

Originally posted by @lrhn in #55661

@kallentu
Copy link
Member Author

kallentu commented May 9, 2024

cc. @dart-lang/language-team

@kallentu kallentu changed the title [Wildcard Variables] Allow super._? [Wildcard Variables][spec] Allow super._? May 9, 2024
@natebosch
Copy link
Member

it not working is a surprising exception to them.

I doubt that any author will see this and have a chance for it to surprise them

I don't think it would cause trouble at all to disallow _ as a field name, nor do I think it will cause trouble at all if we allow it but it causes other syntax edge cases when it's used.

The only member of a class which we should expend effort to allow naming _ is a generative constructor. Allowing it in other places, or disallowing it other places, is a neutral decision from the author perspective. We should choose whatever is easier to spec and implement. Allowing super._() as a super constructor invocation is useful against currently idiomatic Dart. Allowing super._ forwarding formal parameter could be allowed or disallowed and in practice no author will care.

@stereotype441
Copy link
Member

@lrhn said:

I think we should allow super._ because I don't expect users to expect an exception, so it not working is a surprising exception to them. If the author doesn't use the name of a positional super.x parameter, it's reasonable to assume that changing it to super._ is fine. After all, they can change it to super.y or super._arglebargle without any problem.

I find myself agreeing with @lrhn here. It seems to me that if this works (which the spec says it does):

class C {
  var _;

  C(this._); // OK.
}

then it would be surprising for this not to work:

class D extends C {
  D(super._);
}

@lrhn continued:

The reasoning that the specification uses the name is meaningless to the user. Can't it just not do that? Yes, yes it can. It's an accident of specification that the name is used, not anything inherent to the feature.

Interestingly enough, I went and looked back at the super-parameters spec, and as far as I can tell, it's not actually specified in terms of de-sugaring to a representation that uses the names. In point of fact, the semantics are directly specified in terms of the indices of positional parameters. E.g. there is this text:

We define the associated super-constructor parameter for each super-parameter p of C as follows:

  • If p is a positional parameter, let j be the number of positional super-parameters of C up to and including p in source order. The associated super-constructor parameter of p is the jth positional parameter of D (1-based), if D has that many positional parameters.
  • If p is a named parameter with name n, the associated super-constructor parameter is the named parameter of D with name n, if D has a named parameter with that name.

So AFAICT there would be absolutely no problem, spec wise, if we went ahead and supported super._.

But even if there were, I would rather remedy such a problem by fixing the spec so that super._ works, rather than prohibiting it.

@lrhn
Copy link
Member

lrhn commented May 9, 2024

The point of super._ is not to initialize a field, it's to pass a positional parameter to a super constructor, without caring about giving it a name, because all it's doing is get forwarded.

class Point {
  final int x, y;
  Point(this.x, this.y);
}

class ColorPoint extends Point {
  final Color color; 
  ColorPoint._(super._, super._, this.color);
  // More constructors
}

It's not something I expect to see a lot, public constructors should have descriptive names, but the point here is that the names are completely irrelevant to the language.
The declaration could be

ColorPoint._(super.banana, super.apricot, this.color);

and it would work just as well. The name isn't important, so I can't give any good explanation for why using super._ instead should not work.

Which means that if we make it not work, we need to explain that to users, and if we just make it work, we have nothing to explain.
Not working is an exception, and it's for no good reason.

@natebosch
Copy link
Member

I can't give any good explanation for why

Do we need to? I don't think any author will care why we disallow it. An explanation of "it was easier to implement" would be sufficient. The pattern shows up only twice in .dart files, this isn't going to be an impactful decision either way.

It's not obvious to me that disallowing it actually would be any easier to spec and implement, but I do think we should place much higher weight on that over the potential confusion of a tiny fraction of users who might see a diagnostic, or the even tinier fraction of users who will care when they do.

@lrhn
Copy link
Member

lrhn commented May 10, 2024

I'm not worried about existing super._s, I'm worried about future ones, written by people who expect it to work, because there is no obvious reason for it not to work.

If the current specification hadn't singled out super._ and disallowed it, would anybody have questioned that? It could just say:

Using super._ works like today, it forwards the (positional) argument's value to the super constructor invocation, but doesn't introduce a local variable in the initializer list scope, just like this._ doesn't.

(That is an easier specification.)

So let's assume we replace the part about super._ in the specification with that sentence.

Does anyone then have a good reason to change that?

If not, I think that's what the spec should say.

(Of the two, this._ is actually the weirder one, because it uses the name _ for something, then turns around and says that you can't have a local variable with that name and value anyway. You can still refer to the field in the body.)

@eernstg
Copy link
Member

eernstg commented May 14, 2024

Ideally, _ is not binding, ever

@natebosch wrote

The only member of a class which we should expend effort to allow naming _ is a generative constructor.

Plus 100 on that!

(.. Except pet peeve: I don't agree that a constructor whose declaration denotes it as C._ has the name _. There is no location whatsoever in the code where _ resolves to that constructor, so we might as well do as the spec has always done, and say that the name is C._ (even though we may have expressions like C<int>._(), and names don't otherwise get actual type argument lists inserted in the middle). Anyway, this means that we don't even have to have a special exception for constructors because they never have the name _.)

The point is that we may want to allow instance variables etc. to have the name _ because it might be too breaking to disallow it, but it should never (in my opinion) be a goal to support such names at the expense of anything else. So, @natebosch, we agree completely on that.

After a while (and some migration), the statement "_ is a wildcard" would then be true, for all practical purposes without exception. Simplicity does matter.

How about making an exception for super._?

@lrhn wrote:

I'm not worried about existing super._s, I'm worried about future ones, written by people who expect it to work, because there is no obvious reason for it not to work.

The obvious reason is that the value of a declaration whose name is _ is inaccessible (assuming simplicity), and it is a surprising exception if this value does not get ignored.

This is good for the code comprehensibility: Scan the code, notice some local declarations (incl. parameters) where the declared name is _, and proceed to read other parts of the code knowing that those declarations are not referenced by other parts of the code.

You may need to think about the side effects if it is a local variable with an initializer, but otherwise those declarations can just be ignored. This is simpler than thinking about the side effects and thinking that those variables might be used all over the place, in ways that we don't yet understand.

Seeing a formal parameter declaration like super._ would then imply that there is a parameter, but it is ignored. This is an oxymoron, given that the whole point of a super parameter is that it is used, and passed on to the super constructor.

An instance variable named _ will just put some poison into this kind of simplicity, and it's hardly good style anyway.

So I still think we shouldn't bend over backwards in order to make super._ work.

How about this._, then?

It could be claimed that if we support this._ just so there can be an instance variable named _ (not a wildcard) then we also ought to support super._ such that we can pass it on:

class A {
  final int _;
  final int j;
  A(this._) : j = 1; // `this._` would be OK, but `j = _` would be an error.
}

class B extends A {
  final int k;
  B(super._) : k = 1; // `super._` is proposed to be OK, but `k = _` would be an error.
}

Searching through existing code, I don't see any instance variables named _ anyway. (Exception: a couple of extension types do have it, which could make sense because the name of the representation variable might as well have been standardized, and it seems reasonable to avoid making it public).

Note that, except for possibly some noise from the lint matching_super_parameters, we can rename the positional parameter freely, which means that we don't need to use super._, it works just as well with any other name.

This works in the opposite direction here (example from this comment):

class Point {
  final int x, y;
  Point(this.x, this.y);
}

class ColorPoint extends Point {
  final Color color; 
  ColorPoint._(super._, super._, this.color);
  // More constructors
}

... but, again, I do not think this kind of declaration is so desirable that we should bend over backwards in order to enable it. I'd much prefer if there is good support for completion in IDEs such that it can easily be written as super.x, super.y.

My conclusion

I do want to emphasize the simple interpretation where "_ means inaccessible", as consistently as possible.

On the other hand, it is a mess to have support for this._ and not for super._.

Nevertheless, super._ provides a very, very tiny claimed benefit, and any other name than _ can freely be used with super (which is not the case for this._).

So I'm still in favor of keeping the rule that super._ is an error.

@lrhn
Copy link
Member

lrhn commented May 14, 2024

"_ is a wildcard"

My problem with that problem statement is that it doesn't tell me anything about how it actually works.
I have some (wild) associations to "wildcard", but mostly it means "can be used as anything", which is kind of the opposite of what it means here: "can be used for nothing".
(The word works for patterns, a "wildcard pattern" is a pattern that can be used to match anything. It doesn't work that well for declarations.)

So not a useful functional description.

That's why I (try to) consistently talk about this feature as "_ being non-binding" or "_ not introducing a name".
That is what I'm aiming for, no more and no less. If you use _ where you would have used a name, then it still works exactly the same, except that you can not refer to the thing by name any more, because rather than introducing _ as a name, it introduces nothing.

It's fair to argue that _ should be non-binding in more places, but that doesn't change whether super._ should be allowed and be non-binding.

The obvious reason is that the value of a declaration whose name is _ is inaccessible (assuming simplicity), and it is a surprising exception if this value does not get ignored.

Being inaccessible and being ignored is not the same thing. An optional parameter named _ is inaccessible, but if it's optional and non-nullable, then it better have a default value, which will be used.
(Or should we allow an optional parameter named _ to not have a default value. That does seem safe and useful. We probably should!)

Or a this._ which initializes the field named _, but just doesn't introduce a _ name into the lexical scope.
We keep supporting that because it's breaking not to, not because we think instance members named _ is a great idea.
But we do follow through on the "does not introduce a name" part, the this._ does not introduce a _ binding in the parameter scope. That's consistent. The this._ works just like this.x would, storing its value into the corresponding location, but not adding a _ to the lexical scope.

That's what super._ acts like too. The value is not accessible in the parameter scope where the _ "declaration" occurs, meaning we do not introduce a name for it. Other than that, it works just as a super.x would, which means its value is it's just stored directly into the position where it will be used, in the argument list passed to the super-constructor.
It's inaccessible, not ignored.
It's not the _ which makes the value be used in the super-constructor invocation, it's the super. The name doesn't matter, we could make it anything, which is why naming it _ seems so obviously correct to me.

It could be claimed that if we support this._ just so there can be an instance variable named _ (not a wildcard) then we also ought to support super._ such that we can pass it on:

I'm not making that claim. The name of a positional super parameter, or the name of the positional parameter it's forwarded to, has nothing to do with whether it's passed on or not.
The point is precisely that if we have a declaration, super.x, which introduces a name into the lexical scope. and that name is never referenced, the obvious step would be to change it to super._ to avoid polluting the namespace with an unnecessary name. And I cannot find a argument for why that shouldn't work.

We allow super._, super.__ to work today for super-parameters with a name nobody references. It's a regression to not allow super._ to keep working, and it's consistent to make it not introduce a name in its scope.

@eernstg
Copy link
Member

eernstg commented May 15, 2024

The language team just decided that super._ as a positional formal parameter in a generative constructor is not a compile-time error, and it will pass on the given actual argument to the corresponding superconstructor parameter, just like super.id for any identifier id.

@kallentu
Copy link
Member Author

Updated the spec: ae95904.
Closing. Thanks for the discussion, everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specification wildcard-variables The wildcard-variables feature
Projects
None yet
Development

No branches or pull requests

5 participants