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

Add unqualified JSDoc member references #44202

Merged
merged 2 commits into from
May 26, 2021

Conversation

sandersn
Copy link
Member

This allows unqualified references like:

class Zero {
 /** @param buddy Must be {@link D_HORSE} or {@link D_DOG}. */
 deploy(buddy: number) { }
 static D_HORSE = 1
 static D_DOG = 2
}

I surveyed @see and @link again to estimate how common this is. I found a little over 200 uses, which is around 2%. [1] Sorted by frequency, this
is the next feature on the list, along with the module: prefix. So I think this is about the right point to stop adding features.

In this case, however, I liked most of the uses -- there were a lot of deprecated functions that referenced a function just below, where it would be wordy to qualify the name, but the reader would benefit from a link.

Note that unqualified references do not work inside type or object literals. The code I ended up with is quite complicated and I didn't observe any uses in the wild.

Fixes #43595

[1] The survey code was a hand-written approximation of member resolution, so it probably missed some uses.

This allows unqualified references like:

```ts
class Zero {
 /** @param buddy Must be {@link D_HORSE} or {@link D_DOG}. */
 deploy(buddy: number) { }
 static D_HORSE = 1
 static D_DOG = 2
}
```

I surveyed @see and @link again to estimate how common this is. I found
a little over 200 uses, which is around 2%. Sorted by frequency, this
*is* the next feature on the list, along with the `module:` prefix.
So I think this is about the right point to stop adding code.

In this case, however, I liked most of the uses -- there were a lot
of deprecated functions that referenced a function just below, where it
would be wordy to qualify the name, but the reader would benefit from a
link.

Note that unqualified references do *not* work inside type or object
literals. The code I ended up with is quite complicated and I didn't
observe any uses in the wild.

Fixes #43595
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 21, 2021
Since they don't work anyway
@amcasey
Copy link
Member

amcasey commented May 21, 2021

Is my understanding correct?

class C {
    m() {}
    n() {
        type T = {
            m: number;
            /** {@link m} <- this refers to C.m? */
            n: number;
        }
    }
}

@andrewbranch
Copy link
Member

I could have sworn there was a getObjectLikeDeclarationContainer or something that covered classes, interfaces, object type literals, and object literals, but I’m not finding what I’m looking for.

@sandersn
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 25, 2021

Heya @sandersn, I've started to run the tarball bundle task on this PR at 8d25eea. You can monitor the build here.

@sandersn
Copy link
Member Author

sandersn commented May 25, 2021

@amcasey Your interpretation is correct, because an unresolved identifier in jsdoc looks for an ancestor like so: findAncestor(name, or(isClassLike, isInterfaceDeclaration)), which will then resolve m as C.prototype.m.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 25, 2021

Hey @sandersn, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/103883/artifacts?artifactName=tgz&fileId=0B6828028A1454AA926FD9AEAFC4ED039C84E5E6FB34278891D4633F6A47EAF302&fileName=/typescript-4.4.0-insiders.20210525.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.4.0-pr-44202-5".;

@andrewbranch
Copy link
Member

@sandersn I can’t tell if your response means to imply that it’s already working how you think it should work or not, but I think my intuition is that if we will look up unqualified member names in classes and interfaces, we should also do so in type literals and object literals, especially since the latter can be difficult to name otherwise:

(function() {
  return {
    /** {@link b} If this doesn't resolve, what's the alternative? */
    a() {},
    b() {}
})();

I’m not claiming that it is important that this works, but wanted to understand your thinking on it. I guess the tradeoff with resolving more unqualified things is that you could unexpectedly shadow something in a higher scope, and become unable to link to it.

@sandersn
Copy link
Member Author

Right, I wasn't clear about my intentions:

As the PR stands today, I intended for classes and interfaces to support unqualified references and type and object literals not to support them. That's because

  1. This whole feature is a 2% feature, not a 10% or 40% one.
  2. The code required to resolve a member of an anonymous type is fairly different from resolving a named one, as far as I pushed my experiment. I could be missing something obvious that makes it cheap to implement.
  3. The penalty for not resolving references in anonymous types is inconsistent semantics for an interactive feature.
  4. Existing uses of @link are a lot more common on classes and interfaces.

Note that (1) and (4) apply to existing code, but we're going to change future code by shipping this feature. So it might become more important later. (3) might also change since @link is part of the API and people may start using it to generate documentation.

I'll take another look at (2) when I have the chance, although it's going to be a little slow since I'm on DT this week and got started on another JS experiment.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured you probably had data to suggest that it’s not important to resolve in other objecty syntaxes. Sounds good.

@amcasey
Copy link
Member

amcasey commented May 25, 2021

You've addressed my concerns.

@sandersn sandersn merged commit 459bd19 into master May 26, 2021
@sandersn sandersn deleted the unqualified-jsdoc-member-references branch May 26, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@link and @see should lookup sibling methods/properties without a class prefix
4 participants