dart-lang / language

Design of the Dart language
Other
2.68k stars 205 forks source link

Should `super` be callable? #1631

Open stereotype441 opened 3 years ago

stereotype441 commented 3 years ago

Here's a brain teaser for my fellow parsing nerds (I suspect @eernstg and @lrhn will particularly want to weigh in).

Is the following code valid?  If so, what do we expect its output to be?

f(x, [y]) => 'f(x=$x, y=$y)';

class B {
  call<T, U>(x) => '$this.call<$T, $U>($x)';

  operator<(x) => '($this < $x)';
}

class C extends B {
  toString() => 'C()';
  
  void test() {
    print(f(super<int, int>(0))); // (1)
  }
}

extension on Type {
  operator>(x) => '($this > $x)';
}

main() {
  C().test();
}

Stop reading now if you want to guess before I spoil it for you.

AFAICT, the answer depends who you ask:

It seems like we ought to fix this disparity. Anyone have an opinion about which interpretation we should pick as the "correct" one? We aren't in danger of breaking anyone because the CFE rejects the code today.

Personally I favor the analyzer's interpretation, partly because it just feels right to me, and partly because I suspect it will be an easy fix to the CFE (leave the shared parser as is, and remove whatever CFE logic is preventing super from being callable).

bwilkerson commented 3 years ago

This suggests that the analyzer is interpreting it as a call to f with one argument, and that argument is an invocation of super.call with type arguments <int, int> and argument 0.

That is correct. You can validate the AST structure being produced by

lrhn commented 3 years ago

Guess before I get spoiled: No, but only because we forgot it in the grammar or semantics. (That seems to be the usual answer around super.)

The super+something rules are all special cases. It's not an object invocation, so if we didn't write the special-case rule, it doesn't work.

My opinion is that it should work for every invocation you can do on this, as long as the superclass has a non-abstract implementation of the corresponding member. If you can do foo(this<a,b>(0)), you should be able to do foo(super<a,b>(0)). Anything else is just confusion to users.

eernstg commented 3 years ago

We're approaching an old goal: Execute an email! We just need to define a bunch of operators and extension methods, and then anything will mean something! ;-)

I agree with @lrhn that we must have omitted that case from the set of super constructs by accident, and it would improve on the syntactic consistency if we add that case. However, it's not a practical problem, because we can use .call today in order to get the behavior of the potential new syntax.

The spec parser does not bring up any parsing problems if we add a new alternative to <primary>:

<primary> ::= <thisExpression>
    |    'super' <unconditionalAssignableSelector>
    |    'super' <argumentPart>
    |    ... ;
stereotype441 commented 3 years ago

Ok, it sounds like we have a soft consensus for changing the front end so that expressions of the form super(arguments) and super<typeArguments>(arguments) are allowed when there is an appropriate call method in the superclass. I suspect this would be a fairly low impact change because these expressions would essentially desugar to super.call(arguments) and super.call<typeArguments>(arguments) (which are already supported). @johnniwinther, does that seem right to you? What do you think of the idea?

eernstg commented 3 years ago

Here is a proposal for the corresponding language specification update.

leafpetersen commented 3 years ago

LGTM, assuming that there aren't unexpected CFE/backend costs to supporting this.

johnniwinther commented 3 years ago

Support for this (at least for the example code) seems to require 4 lines of code so it should be ok from the CFE perspective.

eernstg commented 3 years ago

Did you impose a constraint on the line length? :grin:

stereotype441 commented 3 years ago

Ok, after discussion in the language team meeting, we're going to move forward with this. @eernstg has already updated the spec (https://github.com/dart-lang/language/pull/1636) so all that remains is to write tests, and make the CFE behavior consistent with the analyzer. I'll write some language tests and then turn this over to the front_end team.