feat: support 'this' type on methods and properties

[mattjohnsonpint]

Fixes #2904

Changes proposed in this pull request:

• Fixes support for polymorphic this type on class methods and properties
• Reports a "not implemented" for fields declared with the this type (would be nice, but is complicated)
• Adds compiler tests for this feature

To clarify, the this type was already implemented, but was returning the base type instead of the calling instance type.

• [x] I've read the contributing guidelines
• [x] I've added my name and email to the NOTICE file

[mattjohnsonpint]

FYI, I considered trying to bind the prototype instance to the derived type, but I couldn't quite get that way to work. So instead, I resolve the current "this" to use as the class instance instead. It works, but let me know if there's a better way to approach this. Thanks.

[mattjohnsonpint]

Could we create an override method for inheritance class ...

I explored this a bit. It would have side effects, even without using the this return type.

For example, given:

class X {
  private foo: i32 = 0;

  doSomething(): void {
    this.foo = 1;
  }
}

class Y extends X {
}

... we can't just transform it to this:

class X {
  private foo: i32 = 0;

  doSomething(): void {
    this.foo = 1;
  }
}

class Y extends X {
  doSomething(): void {
    this.foo = 1;
  }
}

... because field foo is declared private.

The resolver implementation does effectively create an alias of the member, but in doing so it uses the original prototype, bound to the base instance. It doesn't create an actual override.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

[snydergd]

@HerrCai0907 does @mattjohnsonpint make sense in their explanation? Is there anything else to consider? I think it would be great to get support for this feature.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

[snydergd]

I am wondering if there are still any outstanding concerns with this or if it could be merged.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

[snydergd]

I plan to look at this to see if I can make sense of it. Last week was busy and I haven't had a chance yet.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

[snydergd]

@mattjohnsonpint Would you be able to answer this question about the outer type check? All I can think that it could be is type.kind == NodeKind.NamedType -- removing it does seem to still pass the tests, but the bootstrap build also seems to work when I run it, so I am not sure that that is it.

[mattjohnsonpint]

TBH, it's been so long since I wrote it, I don't recall directly. But I'll take another look and see if it triggers my memory. 😅

[mattjohnsonpint]

@snydergd - I replied on @HerrCai0907's code review question. I did mean the entirety of the if condition. If you just remove the first part, then other tests fail. Keep in mind you have to npm run build before re-running any tests or the bootstrap.

[mattjohnsonpint]

Switching this back to a draft, because I found a few easy ways to break things using this approach. For example, just copying the same tests for class Y extends X to another class Z extends X will fail, because the mutated function signature for Y is in the instances cache used by getResolvedInstance / setResolvedInstance.

I'll dwell on it some more and revise it at some point. Feel free to take over though, I think the problem is clear enough - even if the solution isn't yet.