Skip to content

accessor allows this parameter but is not checked or enforced. #36883

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

Closed
tadhgmister opened this issue Feb 20, 2020 · 3 comments · Fixed by #36889
Closed

accessor allows this parameter but is not checked or enforced. #36883

tadhgmister opened this issue Feb 20, 2020 · 3 comments · Fixed by #36889
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Milestone

Comments

@tadhgmister
Copy link

TypeScript Version: 3.7.5 and 3.8 (91ffa1c)

Search Terms:
getter, get, set, setter, accessor property, this parameter
Code
In the following, x.a and x.b() are invalid for the same reason but x.a doesn't give a type error.

interface Unimplemented {
    calculate(): number;
}

class Demo {
   get a(this: Unimplemented) {
        return this.calculate();
    }
    b(this: Unimplemented) {
        return this.calculate();
    }
}
const x = new Demo();
console.log(x.a); // no type error, fails at runtime

console.log(x.b()) // correctly gives following error:
/* The 'this' context of type 'Demo' is not assignable to method's 'this' of type 'Unimplemented'.
     Property 'calculate' is missing in type 'Demo' but required in type 'Unimplemented'.ts(2684) */

Expected behavior is one of:

  1. error to declare a this parameter on an accessor field (get or set)
  2. OR accessing x.a gives same type error as calling x.b()

Actual behavior:
this parameter is syntactically allowed on accessor but has no impact outside the function so it is never checked. Accessing x.a throws an error at runtime.

Playground Link:
Playground Link

Related Issues:
none found, It's quite possible I'm the first one to actually try something like this.

@tadhgmister
Copy link
Author

For reference, what I was doing when I ran into this is that I wanted to write an abstract class with a static method that would work as a valid react component, something like this:

abstract class HookCls<P>{
    abstract useRender(props: P): React.ReactElement | null;
    public static JSX<P>(this: new()=>HookCls<P>, props: P){
        const instRef = React.useRef<HookCls<P>>();
        const inst = instRef.current ?? (instRef.current = new this());
        return inst.useRender(props);
    }
}

The constraint means that HookCls.JSX({}) is invalid because HookCls doesn't extend the concrete constructor, but a subclass like Example.JSX({}) would work, however when it's used in JSX like <Example.JSX a="test"/> means it doesn't keep the this value because it passes an unbound method to react, so I needed to replace it with a getter:

abstract class HookCls<P> {
    abstract useRender(props: P): React.ReactElement | null;
    public static get JSX<P>(this: new () => HookCls<P>) {
        return (props: P) => {
            const instRef = React.useRef<HookCls<P>>();
            const inst = instRef.current ?? (instRef.current = new this());
            return inst.useRender(props);
        };
    }
}

Right away typescript told me that generics on a getter is not valid, but it took me a while to find that it also doesn't support a this parameter correctly.

It would be amazing if typescript was capable of representing that HookCls.JSX has a different type on subclasses, (and it can be transformed based on a generic on the getter) but I recognize how much changes it would take to get something like this to be supported for such a very niche use case. So I expect typescript should just not allow declaring a this on accessors just like generics because it has no way to enforce it.

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels Feb 20, 2020
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 3.9.0 milestone Feb 20, 2020
@DanielRosenwasser
Copy link
Member

We should give an error on the this parameter like

'get' and 'set' accessors cannot declare 'this' parameters.

or

Accessors cannot declare 'this' parameters.

I prefer the first one.

@DanielRosenwasser DanielRosenwasser added Good First Issue Well scoped, documented and has the green light and removed Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". labels Feb 20, 2020
@DanielRosenwasser
Copy link
Member

Thanks @a-tarasyuk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants