Skip to content

Add see tag support #39760

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

Merged
merged 36 commits into from
Sep 9, 2020
Merged

Add see tag support #39760

merged 36 commits into from
Sep 9, 2020

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Jul 27, 2020

Fixes a part of #35524

seetag

@Kingwl Kingwl marked this pull request as draft July 27, 2020 07:50
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 27, 2020
@Kingwl Kingwl changed the title Add see tag parser Add see tag support Jul 27, 2020
@Kingwl
Copy link
Contributor Author

Kingwl commented Jul 27, 2020

Friendly ping @sandersn .

I'm not sure what is the expected of unittests:: JSDocParsing

@Kingwl Kingwl marked this pull request as ready for review July 27, 2020 14:31
@sandersn
Copy link
Member

Couple of quick comments -- I've only glanced at the code, haven't really read it yet.

  1. The jsDocParsing unittests are pretty basic. I think the existing test for @see there is sufficient.
  2. The tests I'd like to see are goto-def tests, obviously.
  3. My understanding of jsdoc is that braces are not required here since these aren't type references, but we might as well parse them because people may still use them ... and they aren't ambiguous, so why not?

@Kingwl
Copy link
Contributor Author

Kingwl commented Jul 28, 2020

Hoops. Seems not my fault.

@Kingwl
Copy link
Contributor Author

Kingwl commented Aug 3, 2020

UP ↑

@Kingwl
Copy link
Contributor Author

Kingwl commented Sep 2, 2020

@sandersn Is that a valid syntax?... I'm not sure what are you expected...

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 2, 2020

Heya @Kingwl, I've started to run the tarball bundle task on this PR at 51b67d7. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 2, 2020

Hey @Kingwl, 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/83979/artifacts?artifactName=tgz&fileId=9A8F21D752ED3524E8C7785D7AE26622ABF7CCFFE3F5BCDE9FA311FFB577030F02&fileName=/typescript-4.1.0-insiders.20200902.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@sandersn
Copy link
Member

sandersn commented Sep 3, 2020

Based on my comment on the original bug, here are some more test cases that need to be added. They don't have to pass, but I want to make sure they don't crash:

  1. @see Class#method -- this should, ideally, resolve to Class at least.
  2. @see {@link M.f} -- doesn't need to work yet.
  3. @see M.f() -- should, ideally, resolve to M.f and ignore the trailing ().

I'm going to do a bit more looking to see how well we'll be able to resolve module references in things like lodash. I think a lot of uses just make up a name for the module they are inside, even though it's never declared inside the module itself.

@sandersn
Copy link
Member

sandersn commented Sep 9, 2020

The only question I have left before merging this is how to resolve x here:

/** @param x @see x */
function f(x) { }

Right now, @see x doesn't resolve in the scope of f.

But I don't actually think people write this. I'm going to figure out if that's the case.

Either way, I think the best thing is to resolve starting from the parameter list of a function declaration.

@Kingwl
Copy link
Contributor Author

Kingwl commented Sep 9, 2020

Maybe we could peek the parameters first if jsdoc attached on a function like declaration.
But that will point to two different symbol. Is that ok?

@sandersn
Copy link
Member

sandersn commented Sep 9, 2020

If we resolveName starting from the parameter list, I think you can get it to resolve to just the parameter. So @see on a function should start from the parameter list probably. (or it may be the function itself, although I'd rather not resolve to locals of a function.)

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

My suggestion fixes the parameter name resolution, still need a test like

const x = 1
/** @see x */
function f(x) { }

To make sure that x resolves to the parameter.

@postspectacular
Copy link

Could someone from the TS team please clarify if this current @see implementation is at all considering the new tsdoc syntax, and more generally, if there's any progress from the TS/VSCode team(s) in terms of better support for tsdoc, especially the {@link ...} syntax and its various link declaration variations? I can't find any information about if/when better tsdoc support is planned by the TS/VSC teams. The tsdoc website seems to indicate some interactions between the 3 MS teams, though... is that still ongoing?

@sandersn
Copy link
Member

sandersn commented Dec 4, 2020

Work on @link is ongoing at the editor-support-for-links branch. I'm just writing tests right now, so it shouldn't be long before I have a PR ready.
Both @link and @see initially parse and resolve entity names only (dotted names). Right now the branch can successfully parse an example like {@link A.B.C ...} where ... is any text you want to put there (it should work with a trailing |link text too -- but I need to add a test for that).

Can you add a deeper link to the tsdoc syntax you're talking about? I talked with @octogonz and concluded that TS should attempt to support a basic subset of entity names. @rbuckton pointed me to a grammar he made, but I'd have to look through my notes to find the link.

Edit: Here is what we're targetting in the near term: #35524 (comment)

@octogonz
Copy link

octogonz commented Dec 4, 2020

The TSDoc spec for @see gives these examples which might be useful test cases:

/**
 * Parses a string containing a Uniform Resource Locator (URL).
 * @see {@link ParsedUrl} for the returned data structure
 * @see {@link https://tools.ietf.org/html/rfc1738|RFC 1738}
 * for syntax
 * @see your developer SDK for code samples
 * @param url - the string to be parsed
 * @returns the parsed result
 */
function parseURL(url: string): ParsedUrl;

@octogonz
Copy link

octogonz commented Dec 4, 2020

and more generally, if there's any progress from the TS/VSCode team(s) in terms of better support for tsdoc,

TSDoc and the compiler seem to take somewhat different approaches today:

  • My impression is the compiler's priority is to correctly parse a subset of very common coding patterns, as determined by a forensic sample of real world code. This means its grammar is heuristic and relatively simplistic. It will necessarily misinterpret many TSDoc syntaxes. For example, you can't reliably parse `@see` or \@see or <tag title="@see" /> without full a Markdown-like engine.

  • By contrast, TSDoc's approach is prescriptive: Although the spec is incomplete today, the aim is to arrive at a rigorous spec. In strict mode, the parser actually issues error messages for malformed inputs (whereas CommonMark has no concept of an "incorrect" input, for example). It's also pretty good at handling comments that weren't authored for TSDoc, but that's a secondary aim.

As such TSDoc would be a somewhat heavyweight piece to incorporate into the compiler. (Not in terms of kilobytes, but in terms of complexity and upkeep.) And doing so might drag the compiler into the rabbit hole of enterprise documentation systems, which is a delightful but surprisingly complex space. This would benefit developers who write rich documentation and adopt TSDoc, but they are a minority. So the current situation is understandable, if not ideal.

An easy way out would be for the compiler to expose a plugin API enabling its comment parser to be replaced. This way, developers could opt-in to TSDoc, and there wouldn't need to be any close coordination between the two projects. This idea was brought up in my last chat with the TypeScript folks, and it seemed to align pretty well with their roadmap for extensibility. It seems like a feasible and pretty good solution.

@RomainMuller
Copy link

RomainMuller commented Feb 1, 2021

There's a weird thing going on right now where @see http://lmgtfy.com/ will actually result in symbol.getJsDocTags() to return an entry where the text is http ://lmgtfy.com (note the extra whitespace after http). This looks like a bug?

@dmrazzy
Copy link

dmrazzy commented Apr 3, 2025

🍾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.