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

Proposal: JSDoc tag to specify preferred type #49220

Open
sandersn opened this issue May 23, 2022 · 22 comments
Open

Proposal: JSDoc tag to specify preferred type #49220

sandersn opened this issue May 23, 2022 · 22 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@sandersn
Copy link
Member

sandersn commented May 23, 2022

Authors often want to specify that a parameter may be passed any string, but has a set of preferred values. Right now, Typescript has no concept of 'preferred type', even though people try to indicate by unioning it with the declared type: string | 'a' | 'b' | 'c'. However, the compiler reduces this to string early on. People have used workarounds to fool the subtype reduction machinery, like string & {} | 'a', but PRs like #49119 improve reduction and unintentionally break these workarounds from time to time.

Based on discussion from the May 20 2022 Design Meeting and suggestions from Discord [1] I propose an explicit jsdoc tag to specify the preferred type:

/**
 * @suggest {'foo' | 'bar'} e
 */
function f(e: string) {
}

This will instruct the language service to offer completions for e that wouldn't be possible given just its declared type. Documentation generators can also display the preferred type along with the declared type.

The syntax is similar to @param:
@suggest { type } identifier

But it can be used with any declaration:

/**
 * @suggest {'foo' | 'bar'} T
 */
function f<T>(p: T): T {
}
/** @suggest {'a' | 'b'} x */
declare var x: string
interface I {
  /** @suggest {'a' | 'b'} p */
  p: string
}
const o: I = {
  /**
   * @suggest {'a' | 'b' | 'c'} p Suggestions can conflict
   */
  p: 'a'
}
declare class C {
  /**
   * @suggest {HTMLAnchorElement | HTMLDivElement} p
   */
  p: HTMLElement
}

In Javascript, usage is more redundant:

/**
 * @param {string} p
 * @suggest {'a' | 'b'} p
 */
function f(p) {
}

In Typescript and checkJs files, the compiler should issue an error if the suggested type is not a subtype of the declared type:

/**
 * @suggest {'a' | 'b' | 0} p
//          ^^^^^^^^^^^^^ Suggested type must be a subtype of the declared type
 */
function f(p: string) {
}

And if the suggested type is never, then nothing should be added to the suggestion list. Maybe documentation generators should treat this as a signal that the property is deprecated (although -- why not use @deprecated in that case?).

Other options:

  1. Do nothing. Make sure at least one workaround remains to make the compiler leave reducible unions unreduced.
  2. Directly support string | 'a' | 'b' | 'c': have the compiler create a new string type for every string used in a literal union. Proposed in Support Intellisense for string/number literals in a widened union #33471.

Other design questions

  1. Should the tag specify values instead of types?
    This seems clunky for the common case, a list of values:
/**
 * @suggest {['a', 'b', 'c']} p
 */
  1. Could the tag be simplified by allowing it only after another tag for a declaration?

The syntax could be simplified to @suggest { type } in that case. That is,

/**
 * @param p @suggest {'a' | 'b'}
 */

But this is awkward in Typescript where people aren't used to writing @template for type parameters, for example. And few other JSDoc tags are context-sensitive like this -- @typedef/@property is the main one, and it's notoriously hard to use.

  1. Should the suggestion be integrated on the end of existing tags?

That is,

/**
 * @param {string} p {'a' | 'b'}
 */

I can't think of a good syntax for this.

  1. This is the first tag that TS users would be using to specify types in JSDoc. Is this OK?

Tag name

The tag is intended to specify a subtype of the declared type. Tools besides the compiler will use the type as an auxiliary to the declared type. Here are a few ideas for names:

  • @prefer
  • @suggest
  • @suggest-type
  • @suggesttype
  • @suggestType
  • @intended
  • @expected

[1] Thanks to cspotcode, @Gerrit0, Andrew Kay, d-fischer and Elijah from #language-design.

@sandersn sandersn added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels May 23, 2022
@Gerrit0
Copy link
Contributor

Gerrit0 commented May 23, 2022

I'm a fan of @suggest since it most clearly matches the intended usage of this tag. (Even the proposed error message uses it!)

A note from the documentation side of things: @prefer-type is an invalid TSDoc tag. @preferType would be better.

On property declarations, should it be optional to provide the name, since that should be inferred?

What should happen if the tag is specified multiple times?

@xirzec
Copy link
Member

xirzec commented May 24, 2022

I love this feature idea, since today we end up having to export enums (of one kind or another) of 'known' values in the Azure SDKs when we have what is referred to internally as an "extensible enum" (the service may later add values) -- this seems much cleaner and is still something we can auto-generate

@maross3
Copy link

maross3 commented May 24, 2022

Directly support string | 'a' | 'b' | 'c': have the compiler create a new string type for every string used in a literal union.

I like a direct fix. Do any significant implications or difficulties come to mind with an approach like this?

As for the tag name, I like @suggest, @prefer, or @intended.

@Zamiell
Copy link
Contributor

Zamiell commented May 24, 2022

This seems like an extremely useful feature!

I'll also mention that this has value for code that deals with bit flags. Recently, in the typescript-eslint codebase, I've refactored enums to numbers, like this:

function getTypeFlags(type: ts.Type): ts.TypeFlag | number {}

Here, the compiler will immediately reduce the return type of the function to a number, so it would be much simpler to write the return value as number instead of ts.TypeFlag | number. However, I specify it in this way to indicate that it is not just any number; it is a set of 0 or more bit flags that match the TypeFlag enum.

In other words, I'm trying clumsily to annotate that it has a "preferred type" in a similar way to what Nathan describes in the proposal.

@orta
Copy link
Contributor

orta commented May 24, 2022

Linking to #33471 which is somewhat a TypeScript focused version of the same issue - JSDoc as a solution could be an interesting angle

@sandersn
Copy link
Member Author

@orta ah, that's the other option. I linked to it in the proposal.

Also, you can see @fatcerberus suggesting a JSDoc solution 3 years ago. Guess we're just slow on the uptake.

@bterlson
Copy link
Member

bterlson commented May 24, 2022

This feature seems great, to me having this be in a comment is good as it doesn't add any complexity to the already complex type semantics. It's just a hint for editors to help the user with some values they likely want to provide. I also like @suggest because it speaks directly to the intent ("editor, please suggest these values") without adding a value judgement about the suggested values being "intended" or "preferred" over other values (although one could choose to document that the suggested values are in fact preferred).

I am curious what the proposed editor experience is for this. It might be good to present these to the user in a way that is clear they are suggestions but other values are allowed. Displaying them as if the type were declared as a union of the suggested values runs the risk of causing confusion about whether other values are allowed.

@sandersn
Copy link
Member Author

The next step after the design meeting is to see whether this proposal, or the feature overall, can work with the patterns in CSSType. Here's a miniature model of what it looks like today:

export interface Properties<T = string & {}> {
  positionX?: Property.PositionX<T> | undefined
}
export type Globals = "inherit" | "initial"
export type PositionX<T = (string & {}) | 0> = Globals | T | "center" | "left" | (string & {})

export type Repeat = Globals | RepeatStyle | (string & {})
type RepeatStyle = "no-repeat" | "repeat" | (string & {})

Used like this:

// @filename: main.ts
import type { Properties } from "csstype"
const o: Properties = {
  positionX: "left" // completions should suggest "inherit", "initial", "center", "left", 0
}
const o2: Properties<"custom" | 1> = {
  positionX: "assignable to string" // completions should suggest "inherit", "initial", "center", "left", "custom", 0, 1
}

Translating the mini csstype into a jsdoc style would look like this:

export interface Properties<T = string> {
  positionX?: Property.PositionX<T> | undefined
}
export type Globals = "inherit" | "initial"
/** @suggest {Globals | T | "center" | "left"} PositionX */
export type PositionX<T = string | 0> = T | string

/** @suggest {Globals | RepeatStyle} Repeat */
export type Repeat = string
/** @suggest {"no-repeat" | "repeat"} RepeatStyle */
type RepeatStyle = string

To make this work, completions for positionX would have to look for a jsdoc @suggest not only on positionX in csstype, but the declaration(s) of its type. And all those type aliases to string would have to survive until completions runs. That makes me think that the biggest difference between this proposal and the one that makes string | 'a' | 'b' | 'c' work is in the syntax, not the implementation in the checker, as @rbuckton has pointed out.

@sandersn
Copy link
Member Author

A simpler test than the mini csstype would be:

/** @suggest {'a' | 'b'} AB */
type AB = string
/** @suggest {'c' | 'd'} CD */
type CD = string
/** @suggest {AB | CD} */
type ABCD = string
var x: ABCD = ''

@sandersn
Copy link
Member Author

I did some quick usage analysis of string unions, looking at Definitely Typed and its dependencies. Counting by occurrences, 90% of usage is of string unions on value declarations, like

interface I {
  from?: string | "onLoad" | undefined
}

But the usage of csstype and react means that type aliases are actually more relied on — about 150 million downloads per month between the two. Still, csstype's graph of aliases that depend on each other are relatively rare: only 3 other packages used them.

@sandersn
Copy link
Member Author

Here an extremely quick addition to getStringLiteralCompleitionsFromSignature showing that the language services can do a plausible job of following type aliases back to their definitions:

            if (checker.getStringType() === type) {
                // TODO: Should be any union containing string
                // 1. Look directly at the annotation on the parameter
                // 2. Try to resolve it.
                // 3. See whether
                //   a. its rhs is `string` (or a union with `string`)
                //   b. it has a suggest jsdoc
                //   c. TODO: Do this recursively! (for csstype)
                // 4. If yes, add the types from getTypeFromTypeNode(suggestTag.typeExpression)
                const annotation = (candidate.parameters[argumentInfo.argumentIndex].valueDeclaration as ParameterDeclaration).type
                if (annotation && isTypeReferenceNode(annotation)) {
                    const s = checker.getSymbolAtLocation(annotation.typeName)
                    if (s?.declarations?.some(d => isTypeAliasDeclaration(d) && d.type.kind === SyntaxKind.StringKeyword)) {
                        // ... create fake types t,u for now...
                        return [t, u]
                    }
                }
            }

@joheredi
Copy link
Member

export interface Properties<T = string> {
  positionX?: Property.PositionX<T> | undefined
}
export type Globals = "inherit" | "initial"
/** @suggest {Globals | T | "center" | "left"} PositionX */
export type PositionX<T = string | 0> = T | string

/** @suggest {Globals | RepeatStyle} Repeat */
export type Repeat = string
/** @suggest {"no-repeat" | "repeat"} RepeatStyle */
type RepeatStyle = string

I love this way of adding suggested values for a string! This feels like a natural syntax and as @bterlson mentioned, it is clear on the intent.

For reference, in @autorest/typescript we generate similar TSDocs so this feature will be super helpful and straightforward for us to adopt.

https://github.com/Azure/autorest.typescript/blob/4c1a61ed7366e85846c8e5fb93b19020b42ad5d3/test/smoke/generated/keyvault-resource-manager/src/models/index.ts#L1093
image

@thw0rted
Copy link

thw0rted commented Jun 2, 2022

First: how does it work for function params? You write

/**
 * @suggest {'foo' | 'bar'} e
 */
function f(e: string) {
}

but how do I offer suggestions for a function with three parameters, or worse, for just the third parameter?

Second: does the actual type have to be string for this? Would it allow

/**
 * @suggest {1 | 2 | 3 | true | 'default'}
 */
function f(n: any)

(Obviously I wouldn't recommend writing an API like that, but sometimes you need to write types for a library you inherited from someone, uh, creative.)

@HolgerJeromin
Copy link
Contributor

Rest parameters or other Tuple Types could be a challenge with @suggest, too:

function testFunction(...args: [string, 'suggestMe' | string, number]) {
    let foo0 = args[0];
    let foo1 = args[1];
}

@sandersn
Copy link
Member Author

sandersn commented Jun 2, 2022

@thw0rted

  1. You don't have to provide suggestions for all parameters, so you can just specify the third one:
/**
 * @suggest {'a' | 'b'} third
 */
function ordinal(first: string, second: string, third: string) { }
  1. You should be able to write any subtype in @suggest, but it's likely that editor support will (1) show @suggest for its associated parameter (2) add to the completion list -- but the latter should only apply in cases where we can be sure. I'd say the way you should write it is:
/**
 * @suggest {1 | 2 | 3 | 'default' | true}
 */
function various(varying: number | string | boolean | Anything | Else | You | Want) { }

@HolgerJeromin Yes, that is one downside of using JSDoc: it doesn't have a way to apply parameter names to rest parameters. You'd have to declare a type alias:

/** @suggest {'suggestMe'} */
type SuggestMe = string
function testFunction(...args: [string, SuggestMe, number]) {
    let foo0 = args[0];
    let foo1 = args[1];
}

@thw0rted
Copy link

thw0rted commented Jun 3, 2022

Ah, I had to look three times to notice the little "e" after the curly-braces in the snippet I pasted, but I get it now. So when it appears on a function it includes a param-name, and when it appears on a type it just applies to the type. Totally makes sense.

@gabritto
Copy link
Member

I've explored what this proposal would look like in practice, and found some complications already mentioned above:

Complications

Type parameter instantiation:

As pointed out above, csstype relies on type parameter instantiation affecting the suggestions:

// some dependent of csstype
import * as CSS from "csstype"
interface MyProperties extends CSS.Properties<0 | 1 | 2> {} // `Properties` is instantiated here...

const obj: MyProperties = {
    backgroundSize: _, // And you get 0, 1 and 2 here for completions
}

Using type parameters in the suggestions can look weird with the JSDoc annotation:

class Foo<T extends string> {
  /**
   * @suggest {T} prop
   */
  prop!: string;
}

const foo = new Foo<"a" | "b">();
foo.prop = _ // We want "a" and "b" to be suggested here

It's unclear how we would define the semantics of that and more unclear how we would implement it.

Annotating types:

With current usage, the suggestion behavior is captured directly in the types:

type SuggestSomething = "hey" | "hello" | (string & {});

const x: SuggestSomething = _ // Suggests "hey", "hello"
const y: SuggestSomething = _ // Also suggests "hey", "hello"

With the proposed JSDoc tag, that would look like this:

/**
 * @suggest {"hey" | "hello"} SuggestSomething
 */
type SuggestSomething = string;

const x: SuggestSomething = _ // Suggests "hey", "hello"
const y: SuggestSomething = _ // Also suggests "hey", "hello"

But notice that, to the type system, SuggestSomething is simply string.
So currently the suggestion annotation is attached to the SuggestSomething type declaration, but it is not attached to the type (which is just string). This means that, to implement the JSDoc tag for annotating types, we would have to do extra work to keep track of this suggestion information throughout the type checker, because the type checker currently doesn't care about how the string type was declared for this particular case. As pointed out in #49220 (comment), by this point, we ask ourselves whether it's even worth it having this alternative be a JSDoc annotation, since the type checker is already going to have to deal with the suggestion-related information if we want to support the scenario above.

A possible alternative would be to restrict the JSDoc annotation to value declarations only, like this:

type SuggestSomething = string;

/**
 * @suggest {"hey" | "hello"} x
 */
const x: SuggestSomething = _ // Suggests "hey", "hello"
/**
 * @suggest {"hey" | "hello"} x
 */
const y: SuggestSomething = _ // Also suggests "hey", "hello"

But now you have to repeat the annotation everywhere, instead of just relying on the type.
This greatly simplifies the implementation, but now we have to ask users to do things differently and to possibly repeat some (or a lot) of code.

Conclusion?

In light of the above and the previous comments by @sandersn, it seems to me that people want or are indeed using types to track this suggestion behavior. So if we want to meet people where they are, we would need a solution where the types keep track of this suggestion behavior. And, once the types (and the type checker) are already keeping track of this behavior, it is unclear to me that there is a benefit in exposing a feature like this through JSDoc syntax, as opposed to type syntax.

Other questions

Another question with a JSDoc-based solution is whether we would type check the annotation to check if the suggestions are assignable to the annotated type. We currently don't produce errors on JSDoc in TS files, but we (or a linter) might want to error when the suggestions don't match the expected type.
This is a type of mistake that can already happen with the current approach, depending on how you write the types:

type StringLike = "a" | "b" | 0 | (string & {}); // oops, now it has a number
const x: StringLike = 0;

@xirzec
Copy link
Member

xirzec commented Aug 3, 2023

Assuming suggestions are always a subset of the "real" type, would another way to approach this be to create a new way to union the super type without collapsing to it?

That is to say if we had something like

type ExtensibleEnum = "a" | "b" | Allow<string>

The language service could suggest the literals, but passing "c" would still be considered valid?

I guess this would be more a form of #33471 than providing suggestions, though.

@jakebailey
Copy link
Member

That's effectively just codifying the & {} trick as an intrinsic, which has all of the downsides of keeping the hack, which is that we have to preserve this info everywhere, can't optimize the type down to just string, etc, for a feature that's designed to not affect type checking and can only be consumed in the editor.

@Andarist
Copy link
Contributor

Here is another library that relies on the completions being dynamically auto-generated based on the supplied argument:
https://www.typescriptlang.org/play?ts=5.2.2#code/JYWwDg9gTgLgBAbzgI2AOwCYEEA2O4C+cAZlBCHAESqYC0ApgG71oy07ADOML9UlAbjgB6YXBgBPMPU4AuOAGYAdAAZVAKHU1seABQIiAQ05wAEgBUAsgBkAkmjABXGAFEc9ECxgAaOAG11OERAoPEpenlKAGMOKIBrSm8QoI5uXih5XSYvAEo4AF4APmDQ0OzWAWTQ0TgAPQB+KoIkoOaQhCrJaUiAK0duAH1DNAkYAAt0AHNEqtSeND5M8pg8opLSuGXKjZExBqaWwiSAXRzKoA

A non-dynamic solution for this problem could be a real deal-breaker for a lot of neat use cases that are possible with the current workaround of string & {}. Would support for that workaround go away completely if the JSDoc-based solution would get in?

@gabritto
Copy link
Member

Thanks for the example. I think we're inclined to go with a non-JSDoc approach, one of the main reasons being that we would need to support type parameter instantiation. So if/when we come up with an alternative, we'd probably want to stop supporting string & {} at some point, but we'd make sure the existing use cases are supported.

@nmay231
Copy link

nmay231 commented Oct 31, 2023

Not certain if this comparison has been made in other threads, but there is already a way to get 'suggested' values on the inside of functions using generics. Autocomplete doesn't work outside of the function, unfortunately. And it's a little quirky as to the types you get inside.

function funky<K extends "a" | "b">(arg: K | string) {
	if (arg === "a") { // arg autocompletes to "a" | "b"
		arg;
		//^?(parameter) arg: "a" | K
		onlyA(arg); // Still works though
	} else if (arg === "b") {
		arg;
		//^?(parameter) arg: "b" | K
	} else {
		arg;
		//^?(parameter) arg: string | K
	}
}

function onlyA(arg: "a") {console.log("AAAAAAAAAAAAAA")}

funky("") // No intellisense here though... :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests