Skip to content

Support a @nonnull/@nonnullable JSDoc assertion comment #23405

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

Open
DanielRosenwasser opened this issue Apr 13, 2018 · 15 comments · May be fixed by #57042
Open

Support a @nonnull/@nonnullable JSDoc assertion comment #23405

DanielRosenwasser opened this issue Apr 13, 2018 · 15 comments · May be fixed by #57042
Labels
Committed The team has roadmapped this issue Domain: JavaScript The issue relates to JavaScript specifically Domain: JSDoc Relates to JSDoc parsing and type generation Suggestion An idea for TypeScript
Milestone

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 13, 2018

This could be used in place of the non-null assertion operator, and solve #23403.

while (queue.length) {
    (/** @nonnull */ queue.pop())();
}

Related is #23217, which tracks definite assignment assertions.

@DanielRosenwasser DanielRosenwasser changed the title Support a @nonnullable JSDoc comment Support a @nonnull/@nonnullable JSDoc assertion comment Apr 13, 2018
@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript In Discussion Not yet reached consensus Salsa Domain: JSDoc Relates to JSDoc parsing and type generation labels Apr 13, 2018
@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Apr 17, 2018
@RyanCavanaugh
Copy link
Member

@sandersn please add /** @nonnull */expr as JS sugar for (expr as NonNullable<typeof expr>) 🍻

@weswigham
Copy link
Member

@RyanCavanaugh Or more appropriately as sugar for expr!, which is almost just sugar for (expr as NonNullable<typeof expr>), but also has fallback behavior if NonNullable doesn't exist.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 17, 2018

That's the easier and more correct way to write it, yes 😝

@mhegazy mhegazy added this to the TypeScript 2.9 milestone Apr 17, 2018
@sandersn
Copy link
Member

sandersn commented Apr 17, 2018

I'm not super excited about this suggestion because it doesn't improve the clunkiness of the existing solution. We are comparing

/** @type {() => void} */(expr)
/** @nonnull */(expr)

The two obstacles come from the cast syntax, not the contents of the comment: you need (1) a C-style comment that (2) is followed by a parenthesised expression. Note that the original example is wrong. You'd need /** @nonnull */(queue.pop())(). You're in for a bunch of punctuation and some confusing parentheses.

Given those two things, the difference between @nonnull and @type {*} is pretty small (1 character, to wit). Even @type {() => void} isn't that much of an improvement.

If sugar were free, I'd be happier about this proposal.

Edit: Updated with @weswigham's even smaller syntax.

@weswigham
Copy link
Member

@sandersn you can shave two characters - @type {*}

@weswigham
Copy link
Member

I think there's an important difference between a cast and a nonnull assertion, from the typechecking perspective, however - a nonnull doesn't destroy type information (ie, for quick info or inference), while an any cast does.

@brendankenny
Copy link
Contributor

any sort of DOM stuff can be pretty annoying without a simple nonnull assertion operator, especially as the DOM d.ts files get tightened from webidl generation (I mean, technically document.documentElement can be null, but in practice... :)

This is wandering way out of the usual jsdoc area, and it may be a terrible idea, but what about expr/**!*/ as sugar for expr!

@weswigham weswigham added Domain: JavaScript The issue relates to JavaScript specifically and removed Salsa labels Nov 29, 2018
@TheLarkInn
Copy link
Member

Bump for awesomness!

@sandersn sandersn removed their assignment Jan 7, 2020
@trusktr
Copy link
Contributor

trusktr commented Mar 17, 2020

what about expr/**!*/ as sugar for expr!

I like this.

In cases like this where comments do not specifically serve a documentation-for-humans purpose, but a documentation-for-ts-compiler purpose, I believe the rules should not specifically follow JSDoc rules.

Here's why:

JSDoc comments were designed to serve a documentation-for-humans purpose (for example the prose in a comment may end up on a documentation website or something similar).

If all type-oriented comments are limited to following JSDoc format, this interferes with existing JSDoc tooling:

  1. Some JSDoc tools do not take into account any code, and read nothing more than comments in a file.
    • Encountering an island /** @nonnull */ in the extracted comments would have no semantic meaning, for example.
  2. For JSDoc tools that do inspect code in order to infer more information from the code, having a /** @nonnull */ floating in front of an expression still doesn't make semantic sense.
    • A non-TypeScript doc tool would have no idea what documentation to generate from this if it didn't crash, and even in a TSDoc environment it isn't worth documentingsuch things; only TSDoc will explicitly know not to document it.

I've seen projects purposefully use JSDoc instead of TSDoc tooling for various reasons, a primary reason being that the developer wishes to be in precise control of the documentation output so that what one writes is exactly what one gets. This is in contrast to TSDoc which often documents too much that isn't semantically important and gives little (or difficult) control over the output.

We can get all the type information we need from an IDE with good intellisense, while otherwise leaving semantic documentation-for-humans to JSDoc comments (as well as JSDoc tools if you prefer, like I do).

Yes, maybe we can tell JSDoc tools to ignore the strange parts they don't understand (like random /** @nonull */ comments floating around, but that complicates the tools in unwated ways just to satisfy TypeScript (I've written my own JSDoc parser and documentation generator, so this is my first-hand sentiment).

I believe we should keep JSDoc comments pure to their purpose, as first-class citizens in documentation for humans, and leave anything else (documentation for the TypeScript compiler) in a different syntax, if possible.

Wdyt?

@Rich-MSR
Copy link

Rich-MSR commented Jul 3, 2021

Slightly off topic, but another possibility for dealing with issue #23403 is if Typescript provided an assertDefined() utility method:

type NonUndefined<T> = T extends undefined ? never : T;

/** 
 * Throws if the supplied value is _undefined_ (_null_ is allowed).\
 * Returns (via casting) the supplied value as a T with _undefined_ removed from its type space.
 * This informs the compiler that the value cannot be _undefined_.
 */
function assertDefined<T>(value: T, valueName?: string): NonUndefined<T>
{
    if (value === undefined)
    {
        throw new Error(`Encountered unexpected undefined value${valueName? ` for '${valueName}'` : ""}`);
    }
    return (value as NonUndefined<T>);
}

Which could then be used like this:

while (queue.length) {
    assertDefined(queue.pop())();
}

Some of the nice things about this approach are:

  1. TS design-time (programmer) mistakes - ie. incorrectly "asserting" that a value will never be undefined - are still explicitly caught at JS runtime.
  2. It's more targeted than using NonNullable<T> since it only excludes undefined, not undefined and null, so the compiler will still correctly catch cases like this:
let test: string | null | undefined = null;
let result: string = assertDefined(test);

There could be an assertNotNull() too.

  1. One could argue that the syntax feels more "natural" than using a JSDoc comment, and you can step through it with the debugger.
  2. Depending on tooling, like /**!*/, it's much easier to find uses of assertDefined in your code with a simple search than the non-null assertion operator !.

@rob-myers
Copy link

We can also write the function above using JSDoc types.

/** 
 * JSDoc types lack a non-undefined assertion.
 * https://github.com/Microsoft/TypeScript/issues/23405#issuecomment-873331031
 *
 * Throws if the supplied value is _undefined_ (_null_ is allowed).\
 * Returns (via casting) the supplied value as a T with _undefined_ removed from its type space.
 * This informs the compiler that the value cannot be _undefined_.
 * @template T
 * @param {T} value
 * @param {string} [valueName]
 * @returns {T extends undefined ? never : T}
 */
export function assertDefined(value, valueName) {
  if (value === undefined) {
    throw new Error(`Encountered unexpected undefined value${valueName? ` for '${valueName}'` : ""}`);
  }
  return /** @type {*} */ (value);
}

@jimmywarting
Copy link
Contributor

thanks for the inspiration @rob-myers

made some small changes as some stuff isn't always necessary

/** 
 * JSDoc types lack a non-null assertion.
 * https://github.com/Microsoft/TypeScript/issues/23405#issuecomment-873331031
 * 
 * @template T
 * @param {T} value
 */
function notNull(value) {
  // Use `==` to check for both null and undefined
  if (value == null) throw new Error(`did not expect value to be null or undefined`)
  return value
}

@ziadkh0
Copy link

ziadkh0 commented Sep 22, 2023

Here's my solution, I prefer it to notNull above because:

  1. It can be compiled away, making it have zero cost
  2. It's just one character 😝
/**
 * Non-null casting like TypeScript's [Non-null Assertion Operator][1].
 *
 * It removes `null` and `undefined` from a type without doing any explicit
 * checking. It's effectively a type assertion that `value` isn’t `null` or
 * `undefined`. Just like other type assertions, this doesn’t change the runtime
 * behavior of your code, so it’s important to only use it when you know that
 * the value can’t be `null` or `undefined`.
 *
 * @example
 * ```js
 * function liveDangerously(x?: number | null) {
 *   // No error
 *   console.log($(x).toFixed());
 * }
 * ```
 *
 * [1]: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#non-null-assertion-operator-postfix-
 *
 * @template T
 * @param {T} value
 * @returns {NonNullable<T>} `value` unchanged
 */
export function $(value) {
  return /** @type {NonNullable<T>} */ (value);
}

@trusktr
Copy link
Contributor

trusktr commented Sep 23, 2023

Based on #48650:

foo(nullableThing/*:!*/)

optional trailing when its the last thing on a line:

const foo: NonNullableThing = nullableThing //!
// The same with semi:
const foo: NonNullableThing = nullableThing; //!

Maybe with : if there's a hazard people are worried about, but I've never seen //! anywhere:

const foo: NonNullableThing = nullableThing //:!
// The same with semi:
const foo: NonNullableThing = nullableThing; //:!

Less is more, JSDoc so verbose!

@DanielRosenwasser DanielRosenwasser linked a pull request Jan 12, 2024 that will close this issue
@Neme12
Copy link

Neme12 commented Feb 24, 2024

This doesn't really solve #23403. The expected behavior there is:

That because the while loop is checking the length of an iterable is first > 0, that array methods are callable and valid with out errors.

not having to add a weird comment assertion (the compiler should know that pop won't return undefined because length was checked).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Domain: JavaScript The issue relates to JavaScript specifically Domain: JSDoc Relates to JSDoc parsing and type generation Suggestion An idea for TypeScript
Projects
None yet