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

.d.ts emitted from JS doesn't use this for method return types #53051

Closed
Methuselah96 opened this issue Mar 1, 2023 · 6 comments Β· Fixed by #54062
Closed

.d.ts emitted from JS doesn't use this for method return types #53051

Methuselah96 opened this issue Mar 1, 2023 · 6 comments Β· Fixed by #54062
Assignees
Labels
Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files Fix Available A PR has been opened for this issue

Comments

@Methuselah96
Copy link

Methuselah96 commented Mar 1, 2023

Bug Report

πŸ”Ž Search Terms

  • jsdoc
  • overload
  • return type
  • inheritance

πŸ•— Version & Regression Information

  • This changed between versions 4.5 and 4.6

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

NOTE: This is JS, not TS.

export class Controller {
	constructor() {
	}

	/**
	 * @returns {this}
	 */
	updateDisplay() {
		return this;
	}
}

export class BooleanControllerNoExtraMember extends Controller {
	constructor() {
		super();
	}

	updateDisplay() {
		console.log('test');
		return this;
	}
}

export class BooleanControllerWithExtraMember extends Controller {
	constructor() {
		super();
		this.test = 5;
	}

	updateDisplay() {
		console.log('test');
		return this;
	}
}

πŸ™ Actual behavior

The resulting declaration file includes updateDisplay() in BooleanControllerWithExtraMember with a return type of BooleanControllerWithExtraMember. This is problematic and causes type errors because the return type for BooleanControllerWithExtraMember.updateDisplay() needs to be assignable to this. Duplicating the JSDoc to specify that it returns this is undesirable since we don't want to have to duplicate the JSDoc for all derived classes.

I included BooleanControllerNoExtraMember as well to highlight the inconsistency that it sometimes doesn't specify the method overload due to seemingly unrelated changes to the class.

export class Controller {
    /**
     * @returns {this}
     */
    updateDisplay(): this;
}
export class BooleanControllerNoExtraMember extends Controller {
}
export class BooleanControllerWithExtraMember extends Controller {
    test: number;
    updateDisplay(): BooleanControllerWithExtraMember;
}

πŸ™‚ Expected behavior

BooleanControllerWithExtraMember doesn't specify updateDisplay() because it doesn't need to. This is how TypeScript 4.5 behaves.

export class Controller {
    /**
     * @returns {this}
     */
    updateDisplay(): this;
}
export class BooleanControllerNoExtraMember extends Controller {
}
export class BooleanControllerWithExtraMember extends Controller {
    test: number;
}
@Methuselah96 Methuselah96 changed the title Method overload in derived class does not rely on base class JSDoc types Unncessary method overload in derived class (from JS) Mar 2, 2023
@Methuselah96 Methuselah96 changed the title Unncessary method overload in derived class (from JS) Problematic and inconsistent method overload in derived class (from JS) Mar 2, 2023
@Methuselah96 Methuselah96 changed the title Problematic and inconsistent method overload in derived class (from JS) Problematic method overload in derived class (from JS) Mar 2, 2023
@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files labels Mar 2, 2023
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 5.1.0 milestone Mar 2, 2023
@a-tarasyuk
Copy link
Contributor

a-tarasyuk commented Mar 3, 2023

This is how TypeScript 4.5 behaves.

The TypeScript example includes updateDisplay() for each class. Or am I wrong?

export declare class Controller {
    constructor();
    updateDisplay(): this;
}
export declare class BooleanControllerNoExtraMember extends Controller {
    constructor();
    updateDisplay(): this;
}
export declare class BooleanControllerWithExtraMember extends Controller {
    test: number;
    constructor();
    updateDisplay(): this;
}

@Methuselah96
Copy link
Author

Methuselah96 commented Mar 3, 2023

This is when creating type declarations from JavaScript (you can change language in the TSConfig > Lang option).

@sandersn
Copy link
Member

I think the output should match whatever TS does; the current state is clearly inconsistent.

@sandersn
Copy link
Member

sandersn commented Apr 27, 2023

Here's a repro that's the same code in JS and TS, but generates different .d.ts files:

export class C {
    updateDisplay() {
        return this;
    }
}

In C, updateDisplay: () => this but the generated d.ts file for JS has updateDisplay(): C whereas for TS it has updateDisplay(): this

@sandersn sandersn changed the title Problematic method overload in derived class (from JS) .dts emitted from JS doesn't use this for methods Apr 27, 2023
@sandersn sandersn changed the title .dts emitted from JS doesn't use this for methods .d.ts emitted from JS doesn't use this for methods Apr 27, 2023
@sandersn sandersn changed the title .d.ts emitted from JS doesn't use this for methods .d.ts emitted from JS doesn't use this for method return types Apr 27, 2023
@sandersn
Copy link
Member

Here's a repro for the overrride part of the bug (though this should perhaps be a separate bug):

export class C {
    updateDisplay() {
    }
}

export class D extends C {
    updateDisplay() {
    }
}

The generated d.ts for JS has export class D extends C {} whereas for TS, D includes updateDisplay(): void.

@sandersn
Copy link
Member

Filed #54061 to track the missing-identical-override bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants