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

Support @abstract JSDoc tag #17227

Open
ghost opened this issue Jul 15, 2017 · 16 comments
Open

Support @abstract JSDoc tag #17227

ghost opened this issue Jul 15, 2017 · 16 comments
Labels
Domain: JavaScript The issue relates to JavaScript specifically Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Milestone

Comments

@ghost
Copy link

ghost commented Jul 15, 2017

TypeScript Version: nightly (2.5.0-dev.20170712)

Code

/** @abstract */
class C {}
new C();

Expected behavior:

Error.

Actual behavior:

No error.

@ghost ghost added the Salsa label Jul 15, 2017
@mhegazy mhegazy added the Suggestion An idea for TypeScript label Oct 11, 2017
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.2 milestone Aug 15, 2018
@RyanCavanaugh RyanCavanaugh added the Experience Enhancement Noncontroversial enhancements label Aug 15, 2018
@tschaub
Copy link
Contributor

tschaub commented Sep 1, 2018

Adding a bit more to this, it would be nice if @abstract on methods were supported.

For example, with this code:

/**
 * @abstract
 */
class Integer {
  /**
   * @abstract
   * @return {number} A number.
   */
  get() {}
}
 
class FortyTwo extends Integer {
  get() {
    return 42
  }
}


/**
 * @returns {number} The return.
 */
function getFortyTwo() {
  const number = new FortyTwo();
  return number.get();
}

tsc --pretty (at 3.1.0-dev.20180901) results in this error:

main.js:8:15 - error TS2355: A function whose declared type is neither 'void' nor 'any' must return a value.

8    * @return {number} A number.
                ~~~~~~

@kohlmannj
Copy link

kohlmannj commented May 3, 2019

Here’s an elaborate yet workable recipe for how I’ve worked around my desire for @abstract classes and methods support in one of my projects:

  • Declare an abstract class in a sidecar .d.ts file
  • Separately implement the abstract class in a .js file
  • // @ts-ignore any usages of an abstract method in the class’s implemented methods (sighhh)
  • Take advantage of TypeScript’s module definition merging to ensure that subclasses implement the abstract class methods

I don’t love this approach, but I do appreciate the added type-checking for my subclasses.

Example Code

SvgRenderer.d.ts

export declare abstract class SvgRenderer {
  public name: string;
  public data?: Record<string, any>;

  constructor(name: string, data?: Record<string, any>);
  public abstract render(): SVGSVGElement;
  public update(data: Record<string, any>): void;
  public serialize: () => string;
  public rasterize: () => Promise<Buffer>;
}

SvgRenderer.js

import { serialize, rasterize } from ./utils’;

/** @abstract */
export class SvgRenderer {
  /**
   * @param {string} name
   * @param {Record<string, any>} [data]
   */
  constructor(name, data) {
    this.name = name;
    this.data = data;
  }

  /**
   * @public
   * @param {Record<string, any>} data
   */
  update(data) {
    if (data) {
      this.data = data;
    } else {
      console.warn('update(): `data` is `undefined');
    }
  }

  // Notice that I’ve marked this method as `@abstract`,
  // but have _not_ added any JSDoc type annotations!
  // This is really just a human-readable reminder for now, however.
  /** @abstract */
  render() {
    throw new Error('Not implemented');
  }

  /**
   * @public
   * @returns {string}
   */
  serialize() {
    // TODO: Remove @ts-ignore after Microsoft/TypeScript#17227 is fixed
    /** @see https://github.com/Microsoft/TypeScript/issues/17227 */
    // @ts-ignore
    return serialize(this.render()); // error ts(2345)
  }

  /**
   * @public
   * @returns {Promise<Buffer>}
   */
  rasterize() {
    // TODO: Remove @ts-ignore after Microsoft/TypeScript#17227 is fixed
    /** @see https://github.com/Microsoft/TypeScript/issues/17227 */
    // @ts-ignore
    return rasterize(this.render());
  }
}

ExampleRenderer.js

import { SvgRenderer } from './SvgRenderer';

export class ExampleRenderer extends SvgRenderer {}

With both SvgRenderer.js and SvgRenderer.d.ts in place, I get the following error in the above ExampleRenderer.js, which is exactly what I want in this case:

ExampleRenderer.js:4:14 - error TS2515: Non-abstract class 'ExampleRenderer' does not implement inherited abstract member 'render' from class ‘SvgRenderer'.

4 export class ExampleRenderer extends SvgRenderer {}
               ~~~~~~~~~~~~~~~

(Note for posterity: this is with TypeScript 3.4.3.)

@RyanCavanaugh RyanCavanaugh removed the In Discussion Not yet reached consensus label Jul 29, 2019
@jonlepage
Copy link

Here’s an elaborate yet workable recipe for how I’ve worked around my desire for @abstract classes and methods support in one of my projects:

  • Declare an abstract class in a sidecar .d.ts file
  • Separately implement the abstract class in a .js file
  • // @ts-ignore any usages of an abstract method in the class’s implemented methods (sighhh)
  • Take advantage of TypeScript’s module definition merging to ensure that subclasses implement the abstract class methods

I don’t love this approach, but I do appreciate the added type-checking for my subclasses.

Example Code

SvgRenderer.d.ts

export declare abstract class SvgRenderer {
  public name: string;
  public data?: Record<string, any>;

  constructor(name: string, data?: Record<string, any>);
  public abstract render(): SVGSVGElement;
  public update(data: Record<string, any>): void;
  public serialize: () => string;
  public rasterize: () => Promise<Buffer>;
}

SvgRenderer.js

import { serialize, rasterize } from ./utils’;

/** @abstract */
export class SvgRenderer {
  /**
   * @param {string} name
   * @param {Record<string, any>} [data]
   */
  constructor(name, data) {
    this.name = name;
    this.data = data;
  }

  /**
   * @public
   * @param {Record<string, any>} data
   */
  update(data) {
    if (data) {
      this.data = data;
    } else {
      console.warn('update(): `data` is `undefined');
    }
  }

  // Notice that I’ve marked this method as `@abstract`,
  // but have _not_ added any JSDoc type annotations!
  // This is really just a human-readable reminder for now, however.
  /** @abstract */
  render() {
    throw new Error('Not implemented');
  }

  /**
   * @public
   * @returns {string}
   */
  serialize() {
    // TODO: Remove @ts-ignore after Microsoft/TypeScript#17227 is fixed
    /** @see https://github.com/Microsoft/TypeScript/issues/17227 */
    // @ts-ignore
    return serialize(this.render()); // error ts(2345)
  }

  /**
   * @public
   * @returns {Promise<Buffer>}
   */
  rasterize() {
    // TODO: Remove @ts-ignore after Microsoft/TypeScript#17227 is fixed
    /** @see https://github.com/Microsoft/TypeScript/issues/17227 */
    // @ts-ignore
    return rasterize(this.render());
  }
}

ExampleRenderer.js

import { SvgRenderer } from './SvgRenderer';

export class ExampleRenderer extends SvgRenderer {}

With both SvgRenderer.js and SvgRenderer.d.ts in place, I get the following error in the above ExampleRenderer.js, which is exactly what I want in this case:

ExampleRenderer.js:4:14 - error TS2515: Non-abstract class 'ExampleRenderer' does not implement inherited abstract member 'render' from class ‘SvgRenderer'.

4 export class ExampleRenderer extends SvgRenderer {}
               ~~~~~~~~~~~~~~~

(Note for posterity: this is with TypeScript 3.4.3.)

hey thanks, yes i also do this for now !

I have nothing against mixing .ts and .js, the two complement each other, jsdoc is more readable in a project i found, too bad it is so badly to support.
I use a lot jsdoc mixing with typescript for get a good types support.

@sandersn
Copy link
Member

Spurred by @a-tarasyuk 's recent PR, I did a survey of @abstract usage, trying to answer two question: (1) whether @abstract methods are usually contained inside @abstract classes, and (2) what is usually inside @abstract method bodies. It didn't take long; I only found 13 code bases using @abstract right now.

Here's what I found:

Abstract methods in abstract classes?

No.

@abstract methods are not usually contained inside @abstract classes. Unlike Typescript usage, @abstract usually shows up on a class (or constructor function) or its methods, but not both. There were 28 classes marked with @abstract, but only 22 methods contained inside them. The median class marked @abstract had zero abstract methods. I guess that the @abstract is intended only to prevent instantiation of the class.
(22 abstract methods were contained in abstract classes, whereas 67 were in non-abstract ones)

Abstract method bodies?

Empty or a single throw.

The top two abstract method bodies were empty (40) and a single throw (17). Webpack had another 17 require/throw statement pairs that could be inlined to a single throw. The leftovers (8) were on methods that were mistakenly (or wishfully?) declared abstract, plus a single deprecated method in webpack that threw unless it could detect it was being used from a deprecated location, in which case it ran the deprecated code.

@sandersn
Copy link
Member

Based on discussion with @a-tarasyuk, after reading his PR #42186 and after surveying usage: we're not going to take PRs for this bug unless usage ticks up considerably and there is a bigger demand for it. I believe current usage is hugely depressed by the existence of Typescript, since people who want this feature likely want all the other OO decorations that Typescript adds, like public and private.

I'm going to leave this open because things may change in another decade or two.

@ShikChen
Copy link

When trying to migrate the type checker of a large JS codebase from Closure Compiler to Typescript, we found that lacking @abstract support is the major blocker. It would be great if the PR can be merged, and we are willing to test it for our codebase if needed :)

@ExE-Boss
Copy link
Contributor

It also helps with code like:

// base.d.ts
export interface SomeValue {
	foo: string;
	bar: number;
}

export abstract class Base {
	abstract doStuff(): string;
	abstract getSomeValue(): SomeValue;
}
// DoStuff.js
import { Base } from "./base.js";

/** @abstract */
export class DoStuff extends Base {
	doStuff(callback) {
		const someValue = this.getSomeValue();
		return `foo: ${JSON.stringify(someValue.foo)}, bar: ${JSON.stringify(someValue.bar)}`;
	}
}

Currently, TypeScript complains that the DoStuff abstract class doesn’t implement the getSomeValue abstract method.

@sandersn
Copy link
Member

@ShikChen

  1. Can you try out a build from the PR and report on your experience? Quoting the bot, add the following to your package.json:
{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/98652/artifacts?artifactName=tgz&fileId=606CC13BB0AC03ADD769068E6B431AFC3E8D76BC07CDFE3667DFE540A6500AC002&fileName=/typescript-4.3.0-insiders.20210316.tgz"
    }
}
  1. How do the semantics of @abstract in your Closure codebase compare to the semantics in Typescript? It is only on classes? Only on methods? What's in the body of abstract methods?
  2. What is your intended use of @abstract during the upgrade? For example, are you going to stop compiling with Closure, but continue compiling with JS files with Typescript for a long period of time? Unless that period is quite long, @abstract seems non-essential to me since it (1) prevents you from instantiating the abstract class and (2) forces you to propagate abstractness to subclasses. I think the latter, especially, should be rare in a transitional codebase. I know that chrome-devtools-frontend converted to TS without @abstract, but I'm not sure how they dealt with its lack. I think they may have converted abstract classes to TS quickly.

@peter50216
Copy link

@sandersn

(This is based on the same code base with @ShikChen)

  1. Can you try out a build from the PR and report on your experience? Quoting the bot, add the following to your package.json:
{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/98652/artifacts?artifactName=tgz&fileId=606CC13BB0AC03ADD769068E6B431AFC3E8D76BC07CDFE3667DFE540A6500AC002&fileName=/typescript-4.3.0-insiders.20210316.tgz"
    }
}

Tried the newer build in the last comment and it works well for our usage of @abstract.

  1. How do the semantics of @abstract in your Closure codebase compare to the semantics in Typescript? It is only on classes? Only on methods? What's in the body of abstract methods?

We're only using it to mark the pure interface class, which only has bunch of abstract methods and intended to be subclassed. All the body are empty, and we have @abstract marker on both the class and all the methods.

  1. What is your intended use of @abstract during the upgrade? For example, are you going to stop compiling with Closure, but continue compiling with JS files with Typescript for a long period of time?

Yes, we're planning to transition to TypeScript eventually.

Unless that period is quite long, @abstract seems non-essential to me since it (1) prevents you from instantiating the abstract class and (2) forces you to propagate abstractness to subclasses. I think the latter, especially, should be rare in a transitional codebase. I know that chrome-devtools-frontend converted to TS without @abstract, but I'm not sure how they dealt with its lack. I think they may have converted abstract classes to TS quickly.

Since the codebase is ~18k loc, it'd take some time to fully migrate to TypeScript. We're currently trying to fix as many errors from tsc as possible before fully transition to TypeScript syntax.

The current major blocker we have is if the abstract method also has @return, tsc complains about the empty body must return a value (TS2355). We'll be happy as long as TypeScript doesn't complain about that as long as the method is also marked as @abstract.

@DanielRosenwasser
Copy link
Member

My suggestion to workaround a method with both abstract and return is to define a method called abstract whose return type is never and invoke that. TypeScript understands that

/**
 * @returns {never}
 */
function abstract() {
  throw new Error("This method was declared as abstract and cannot be called directly.")
}

/**
 * @abstract
 */
class Foo {
  /**
   * @abstract
   * @returns {string}
   */
  method() {
    abstract();
  }
}

TypeScript won't enforce the abstract-ness, but it's possible that Closure will and TypeScript won't get in your way during the transition.

@peter50216
Copy link

@DanielRosenwasser Sadly closure compiler doesn't like the workaround, and reports error:

[JSC_MISPLACED_ANNOTATION] Misplaced @abstract annotation. function with a non-empty body cannot be abstract

@a-tarasyuk
Copy link
Contributor

a-tarasyuk commented Aug 13, 2021

It was an interesting experiment, however, it seems there are no reasons to continue working on it. I agree with @sandersn's points about this feature. Therefore, I closed PR in order not to mislead users who use PR directly.

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 6, 2021
Migrate the CCA build from closure compiler to TypeScript. Also fix all
remaining TypeScript errors that are hard to be fixed while satisfying
both closure compiler and TypeScript.

The one issue that contributes to most of the code change is that
TypeScript doesn't properly support @abstract jsdoc tag [1], and
complains that methods with return types should return something. We
manually added a runtime assertNotReached() to those functions to get
rid of the warning. We can refactor most usage to proper TypeScript
interface later when doing per-file migrations.

Also updates cca.py {tsc,deploy} to encompass the extra tsc pass.

There are four files that due to jsdoc typing limitation can't be
correctly typed in jsdoc. Those files are migrated to .ts. And since
TypeScript doesn't recognize the jsdoc types in .ts files, all types are
inlined with help from ts language server refactor support.

[1]: microsoft/TypeScript#17227

Bug: b:172340451
Test: tast run <ip> camera.CCAUISmoke* camera.CCAUIStress*

Change-Id: Ic6047744fb85bee2d5519cff88a861df75263dc0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3090445
Commit-Queue: Pi-Hsun Shih <pihsun@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Shik Chen <shik@chromium.org>
Cr-Commit-Position: refs/heads/main@{#948458}
@trusktr
Copy link

trusktr commented Jan 11, 2022

@sandersn Did you count the plain JS code bases that have abstract classes and methods, but that do not use JSDoc syntax? I've written and seen a slew of code bases like this.

I'm working on https://github.com/nasa/openmct, which has a number of abstract classes with abstract methods, not marked with comments. There is currently no intent to move the project to TypeScript, but adding some type support for IDEs as we develop, little by little, would be really nice.

I am willing to test the feature in the PR, to provide the "buyin" you mentioned.

@sandersn
Copy link
Member

@trusktr I didn't think to see how many people were creating abstract classes/methods without using @abstract. Can you describe how your code indicates abstractness?

@dec0dOS
Copy link

dec0dOS commented Sep 21, 2023

Any updates on this? Is there a way to retrieve the content from #42186?

@trusktr
Copy link

trusktr commented Sep 22, 2023

@trusktr I didn't think to see how many people were creating abstract classes/methods without using @abstract. Can you describe how your code indicates abstractness?

Nothing crazy, probably what you might imagine:

class Base {
  foo() {
    throw new Error('implement in subclass')
  }
}

class Sub extends Base {
  foo() { return something }
}

Having a working @abstract would solve that (and I'd use the comment). But these days, I really wish type+doc comments were more terse and moving away from JSDoc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: JavaScript The issue relates to JavaScript specifically Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.