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

Do extensions on a class apply to this within that class? #470

Closed
bwilkerson opened this issue Jul 23, 2019 · 9 comments
Closed

Do extensions on a class apply to this within that class? #470

bwilkerson opened this issue Jul 23, 2019 · 9 comments
Labels
extension-methods question Further information is requested

Comments

@bwilkerson
Copy link
Member

Given the following code:

class C {
  void invokeExtensionMethod() {
    methodOnExtension();
  }
}
extension E on C {
  void methodOnExtension() {}
}

is the invocation of methodOnExtension valid?

I'm guessing the answer is "yes" because the target of the invocation is this, which matches the type being extended by E.

@leafpetersen
Copy link
Member

Based on the second to last paragraph of this section I believe that the answer is "yes", though it's not 100% clear that that paragraph was intended to be talking about references inside of classes instead of just inside of classes.

So, yes.

@bwilkerson
Copy link
Member Author

If I'm understanding that section correctly, then references to this within an extension are treated the same as any reference to a variable to the same type. When resolving this.m() we first look in the type of this (the type being extended) for a declaration of m, and then, if that fails, we look for an extension method.

And if there are conflicting extensions of the extended type, then it's an error (as opposed to favoring the member from the enclosing extension).

@leafpetersen
Copy link
Member

That is correct. But note that if we write that as just m() instead of this.m(), then the rule changes slightly: we first look to see if m is a member in the current extension declaration, and if so prefer that. Otherwise, we rewrite it to this.m() (if applicable) and continue with extension method resolution as usual.

@eernstg
Copy link
Member

eernstg commented Jul 24, 2019

We need to make a small adjustment here regarding the forms that do not have an explicit this and that occur in an extension declaration. The section mentioned above has the following:

Like for a class or mixin member declaration, the names of the extension
members, both static and instance, are in the lexical scope of the extension
member body.

That's true, given the specification of which scopes exist and how they are nested inside each other.

However, the current rules governing resolution of an identifier reference (static part here, dynamic part here) specify that a reference id to an instance member declaration is resolved by transforming it into this.id and proceeding with that. So we basically ignore the lexical scoping when it comes to instance members (because we are casing on the kind of declaration that id resolves to), and instance members are only considered via the transformation to this.id.

The same thing applies to unqualified function invocations (here): We check that a number of criteria don't apply (id is not a type, not a library prefix, etc), and then we transform the invocation to this.id<...>(...).

However, I believe that we agree that we want these rules to be able to select the instance member as if by lexical scoping. I gave a draft specification of how we could achieve this in #328. That proposal is aimed at specifying the meaning of a declaration named this in general, and it does not take extension methods into account (but it easily generalizes to cover extensions).

So for the original question in this issue, 'Do extensions on a class apply to this within that class?', we're presumably in the situation where methodOnExtension is not in the lexical scope at the call site, and then the current rules (as well as any variants on the table, including #328) would cause methodOnExtension() to be transformed into this.methodOnExtension() and then processed as such. And, as @leafpetersen mentioned, that would definitely allow for the invocation to resolve to the extension method.

@bwilkerson
Copy link
Member Author

I don't have an opinion about which order of lookup we use, but I do think it would be confusing to users if the lookup behaved differently in extensions than it does in classes. I'll proceed for the time being assuming that m() will be treated just like this.m() when preceding steps have failed.

@eernstg
Copy link
Member

eernstg commented Jul 24, 2019

But if you use the current rules for unqualified function invocations in an extension method body then you will transform m() to this.m() even in the case where the enclosing extension E contains a declaration named m, and then this.m() may resolve to an instance method of the syntactic receiver (based on the on type) or an extension method in some other extension. But it should resolve to the m declared in E.

In a class there is no such discrepancy because the transformation from m() to this.m() will preserve the property that it resolves to the instance method (and it also covers the case where m is not in the lexical scope at all, because it was added to the interface of the class by some superinterface), so we only need to do something extra because that's not true in an extension method.

(The feature spec uses the phrase 'instance member of the extension' many times. We may get around this discrepancy by saying that they are not instance methods, but if we call them 'extension methods' or something like that then we still need to change the rules about unqualified function invocations and identifier references in order to say what to do when the declaration with the requested name turns out to be an 'extension method'; and as long as they are called 'instance methods' then our current rules are applicable, but they will give rise to the transformation from m() to this.m() also in cases where this is not intended).

@bwilkerson
Copy link
Member Author

But it should resolve to the m declared in E.

I obviously haven't thought about it as deeply as you have, but why do you say that it should?

Consider this simple case, under the assumption that unqualified references to extension members are always bound to the extension member even when they're implemented in the extended class:

class C {
  String get name => 'C';
}

extension E on C {
  String get name => 'E';

  void printNameTwice() {
    print('$name, $name');
  }
}

In this case, I can see why these might seem like the right semantics. It would be confusing for printNameTwice to print 'C, C' when name is "clearly" defined to return 'E'.

But consider a slightly more complex case, under the same assumption:

class C {
  String get name => 'C';

  String get displayName => name;
}

extension E on C {
  String get name => 'E';

  void printNameTwice() {
    print('$name, $displayName');
  }
}

Isn't it even more confusing for printNameTwice to print 'E, C'?

I think one downside to performing local lookup inside the extension is that it produces the same behavior that a user would expect if members in extensions could override members in the extended class. But members in extensions don't override the members in the extended class and I'm concerned that this might cause some users to think they can.

If we changed the semantics so that lookup works the same both inside and outside the extension, even though it would be confusing for the first example to print 'C, C', I think it would be easier overall for users because they wouldn't have to remember that there are two different lookup algorithms.

@bwilkerson bwilkerson added the question Further information is requested label Jul 24, 2019
@eernstg
Copy link
Member

eernstg commented Jul 25, 2019

The reason why I said that it should resolve to the m declared in E is that this is the intention which is clearly behind the text in the feature specification:

Like for a class or mixin member declaration, the names of the
extension members, both static and instance, are in the lexical scope
of the extension member body. That is why MySmart above can invoke
the static smartHelper without prefixing it by the extension name. In the
same way, instance member declarations (the extension members) are
in the lexical scope.

.. so bool get isOdd => !isEven; in the example following the above paragraph will call MyUnaryNumber.isEven and not int.isEven:

The unqualified isEven of isOdd resolves lexically to the isEvent getter
above it, so it is equivalent to MyUnaryNumber(this).isEven, even if there
are other extensions in scope which define an isEven on List<Object>.

If the rule had been "transform the unqualified isEven to this.isEven, then proceed with that" then we would be forced to resolve that call using the standard rules for this.isEven, and that might yield an instance method of List or an extension method on List<...> (or on some other type that List<Object> matches). But we want it to resolve to MyUnaryNumber.isEven, because the lexically visible declaration should be given a high priority.

Lasse and I have discussed this topic area several times—starting at the time when I wrote #328, where I wanted to make sure that we maintain the existing rationale for lookups: "The nearest lexically visible declaration wins."

That is true today: If the nearest lexically visible declaration is top-level, static, or local then we just use that; otherwise we transform id to this.id, but that is still guaranteed to resolve to the nearest lexically visible declaration named id in the case where that is an instance member of the enclosing class.

(It will also ensure that we can access an inherited member when there is no declaration named id in scope at all, but the fact that we will select a top-level declaration in scope rather than an inherited declaration again illustrates that the rule in Dart generally is "the nearest lexically visible declaration wins".)

I'm just pointing out that we need to adjust the rules for identifier reference resolution and unqualified function invocation resolution in order to preserve that property when we add extension declarations containing 'instance methods'.

With respect to the example:

class C {
  String get name => 'C';

  String get displayName => name;
}

extension E on C {
  String get name => 'E';

  void printNameTwice() {
    print('$name, $displayName');
  }
}

I believe that "the nearest lexically visible declaration wins" is a meaningful and understandable property to maintain for our detailed name resolution rules: name in printNameTwice resolves to E.name and name in C.displayName resolves to C.name, and you could say that this is meaningful because those declarations are "the most visible" ones at the call site.

When the developer explicitly writes this.name we start from scratch and consider this as any other syntactic receiver, so in that case we surely want to use the same rules as we do everywhere else when we see e.name where e has type List<Object> (and with e.name we never care about which declarations named name are in the lexical scope).

@bwilkerson
Copy link
Member Author

Thanks! I think I understand much better now. I was confused before and thought you were saying something different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-methods question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants