Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

extension methods in derived classes with separate compilation #138

Closed
jmesserly opened this issue Apr 16, 2015 · 2 comments
Closed

extension methods in derived classes with separate compilation #138

jmesserly opened this issue Apr 16, 2015 · 2 comments
Assignees

Comments

@jmesserly
Copy link
Contributor

Noticed an interesting case in the SDK, with TypedData:

abstract class NativeTypedArrayOfDouble
    extends NativeTypedArray
        with ListMixin<double>, FixedLengthListMixin<double>

when we're compiling NativeTypedArray, we don't know that List will be mixed in later, so we don't know to generate this.length as an extension method pattern. In this case we could figure it out, but for separate compilation it doesn't seem like we can know. Maybe we can introduce a "transition" only in the derived class, but we need to be careful to handle inheritance:

// JS
class Derived extends BaseThatHasLengthButNotList /*Dart: implements List*/ {
  // redirect old one to new one
  get length() { return this[core.$length]; }
  // implement new name in terms of old one
  get [core.$length]() { return super.length; }
}

We can end up in the reverse situation too: mixin has raw length but superclass has List.length. Or similarly with an interface:

// dart
abstract class HasALength { int get length; }
class CustomList extends ListBase<String> implements HasALength { /*...*/ }
class DoesNotImplementList implements HasALength { /*...*/ }

someMethod(HasALength hal) {
  // this call site must be generated as `hal.length`
  // since we don't know if `length` is an extension or not.
  expect(hal.length, 9000);
}

My sense is that for a member like forEach, we have an "extension" bit. Whenever possible that bit is kept the same as super class. If there's a conflict, due to interfaces or mixins, we need to "merge" and have one redirect to another.

(I used length as the example because that's what I was seeing. But it's not a great example, because we don't need to do this for length which is identical in JS and Dart. I'll file another bug on that.)

Perhaps a simpler solution:

  • always generate Dart members with normal names
  • when compiling the Dart Array members, use extension method names
  • if a type implements Iterable/List, add redirects:
// Only when compiling the Dart Array impl methods
Array.prototype[core.$where] = ... impl of where ... 

// For all call sites where type is just Iterable or List.
// If we know a more precise type we don't need:
// Dart: Iterable<int> myIter;
myIter[core.$where](...)

// For normal Dart code implementing an iterator:
// Dart: class MyIter implements Iterable<int> { ... lots of methods ... }
class MyIter extends core.Object {
  where(...) { ... }
  ...
} 
dart.implements(MyIter, Iterable$(int))

by saying it "implements Iterable" the runtime will add forwarders that look like:

// this could even be mixed in
class ImplementsIterableForwarders {
  [core.$where](...) { return this.where(...); }
}

The result this simplifies things: we don't need to generate "extension methods" except from our controlled core lib code. It's sort of like there's an imaginary subclass:

class DartList extends Array {
  // dart impls
}

but we've superimposed it onto JS Array in a safe way.

@jmesserly
Copy link
Contributor Author

fyi @jacob314

@jacob314
Copy link
Contributor

I agree that using when the class implements iterable/list is the point where we should add the redirects from extension method names to regular method names.

jmesserly pushed a commit that referenced this issue Apr 21, 2015
…ifier

We were tracking currentClass for no real reason.

Also attempted to discern what visitSimpleIdentifier was actually doing. It now prefers the original element (the "field" not the synthetic getter/setter) whenever possible. Previously was a mismash.

This little change tripped on the (larger, not fixed yet) issue #138 ... but I think I found a way around the issue for now, by tweaking the private TypeDataArray class.

[email protected]

Review URL: https://codereview.chromium.org/1095563003
@jmesserly jmesserly self-assigned this May 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants