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

Override and default types #11410

Open
ncannasse opened this issue Nov 24, 2023 · 4 comments
Open

Override and default types #11410

ncannasse opened this issue Nov 24, 2023 · 4 comments
Assignees

Comments

@ncannasse
Copy link
Member

Look at the following sample:

class A {
  public var x : Int;
  public function foo( v : A ) : Int {
    return v.x;
  }
}

class B extends A {
   override function foo(v) return v.x * 2;
}

class Test {
  static function main() {
    var x : B = null;
    x.foo({ x : 0 });
  }
}

In this case, we somewhat innocently changed the type of B.foo to be more general that what it should be.
Sadly, in HashLink and some other static platforms, this will create an allocation for each call of foo because we're casting an "A" to "{ x:Int}".

I think the override argument types should be inherited by default from the super class definition if they have no explicit type. That would both improve the completion and avoid this kind of hidden error.

@ncannasse
Copy link
Member Author

Ping @Simn for comment

@Simn
Copy link
Member

Simn commented Nov 24, 2023

This requires splitting up check_overriding into 1. finding the field and 2. doing the actual checks. That way we can then:

  1. find the original field
  2. type the override field
  3. do the actual checks

However, we have to keep overload in mind because in that case we can't always do 1. before we've done 2. in order to determine what exactly we're overriding.

@Simn Simn self-assigned this Nov 24, 2023
@ncannasse
Copy link
Member Author

I think we will need to have a late check for types anyway because of potential typing recursions.

@Simn
Copy link
Member

Simn commented Nov 25, 2023

Yes, although this already happens very late anyway, so I don't think we'll run into any typing pass issues here.

I realized that this reminds me of #10569. Another case is implementing an interface method:

class C implements I {
    public function new() {}

    public function test(s) {
        $type(s); // Unknown<0>, should be String
    }
}

function main() {
    var c = new C();
    $type(c.test); // (s : Unknown<0>) -> Void
}

That first $type should be the same case as the issue here, but I was quite surprised that even the second $type still has the monomorph, which means unification didn't occur yet. I see in the dump that it eventually becomes String -> Void, but that seems to happen too late right now.

I'll look into cleaning this behavior up and make it consistent for all these cases where the typing of a field depends on the type of another field.

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

No branches or pull requests

2 participants