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

priority of a forwarded static extension #9680

Closed
vonagam opened this issue Jul 2, 2020 · 8 comments · Fixed by #9682
Closed

priority of a forwarded static extension #9680

vonagam opened this issue Jul 2, 2020 · 8 comments · Fixed by #9682
Milestone

Comments

@vonagam
Copy link
Member

vonagam commented Jul 2, 2020

Example:

@:using(Main.FooTools) 
class Foo {
  public function new() {}
}

@:using(Main.BarTools) 
@:forward 
abstract Bar(Foo) from Foo {}

class FooTools {
  public static function ext(that: Foo)
    trace('Foo');
}

class BarTools {
  public static function ext(that: Bar)
    trace('Bar');
}

class Main {
  public static function main() {
    var bar: Bar = new Foo();
    bar.ext(); // traces Foo, not Bar
  }
}

Right now resolution order for a field of abstract with forwarding is:

  • a field of abstract
  • a field of forwarded
  • an extension for forwarded
  • an extension for abstract

Shouldn't last two be other way around?

@RealyUniqueName RealyUniqueName added this to the Design milestone Jul 2, 2020
@RealyUniqueName
Copy link
Member

I agree, they should.

@vonagam
Copy link
Member Author

vonagam commented Jul 3, 2020

By the way, this will fix an example in last comment of #5924.

@Simn
Copy link
Member

Simn commented Jul 9, 2020

The original intent of @:forward was to allow forwarding field access to fields that actually exist on the underlying type. The interaction with static extensions seems pretty unfortunate, but at this point I don't think we should break it.

The behavior described here is an implementation detail which comes from the call stack. Given an abstract A over U with an unconditional @:forward, we start out like this:

actual field on A
recurse into U because of @:forward
static extension on A
... something about `this` and the underlying type that I don't understand right now (on A)
@:resolve on A

Expanding the recursion we then get this:

actual field on A
actual field on U
static extension on U
... something about `this` and the underlying type that I don't understand right now (on U)
@:resolve on U
static extension on A
... something about `this` and the underlying type that I don't understand right now (on A)
@:resolve on A

The question now is what we want instead. I think this actually goes beyond the static extension and both the mysterious this thing and @:resolve should be affected as well.

If we agree that static extensions on the abstract should take priority, then the only logical answer is to have this order:

actual field on A
actual field on U
static extension on A
static extension on U
... something about `this` and the underlying type that I don't understand right now (on A)
... something about `this` and the underlying type that I don't understand right now (on U)
@:resolve on A
@:resolve on U

Before we continue, we'll have to agree if this is indeed what we want. I don't have much intuition for this and merely go by the pattern here.

@Simn Simn added the discussion label Jul 9, 2020
@Simn
Copy link
Member

Simn commented Jul 9, 2020

The weird this thing was originally added here:

Simn@17ee33c

And then modified here:

Simn@57784e9

I still don't get what this is doing because this should already be typed as the underlying type anyway.

Simn added a commit that referenced this issue Jul 9, 2020
@RealyUniqueName
Copy link
Member

Now that there's no mysterious "this" thing involved the resolution order would look like this:

actual field on A
actual field on U
static extension on A
static extension on U
@:resolve on A
@:resolve on U

which makes sense to me.

@Simn
Copy link
Member

Simn commented Jul 15, 2020

That makes sense to me too, but it's not quite what #9682 implements, I think.

@vonagam
Copy link
Member Author

vonagam commented Jul 15, 2020

Yes, #9682 didn't take into account existence of @:resolve.

So, to implement it, maybe we can go with some additional field in TypeFieldConfig that will contain a list of abstract wrappers above?

@vonagam
Copy link
Member Author

vonagam commented Jul 24, 2020

With new priority for @:using the list should be updated like so (module includes global here):

actual field on A
actual field on U
type static extension on A
type static extension on U
module static extension on A
module static extension on U
@:resolve on A
@:resolve on U

Because if you simply wrap a type with a transparent (with forward and direct casts, optionally transitive) abstract, it shouldn't give module static extension on U more priority than type static extension on U. But it will if they are not split as above. If extensions are checked together it will be like so:

type static extension on A
module static extension on A (if A is transparent, then it is the same as "module static extension on U")
type static extension on U
module static extension on U

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants