Skip to content

@example breaks when example includes @ character (org package imports) #39371

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

Closed
mesqueeb opened this issue Jun 30, 2020 · 6 comments · Fixed by #42364
Closed

@example breaks when example includes @ character (org package imports) #39371

mesqueeb opened this issue Jun 30, 2020 · 6 comments · Fixed by #42364
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone

Comments

@mesqueeb
Copy link

mesqueeb commented Jun 30, 2020

TS Template added by @mjbvz

TypeScript Version: 4.0.0-dev.20200701

Search Terms

  • jsdoc
  • example
  • code block
  • quick info

  • VSCode Version:
Version: 1.46.1
Commit: cd9ea6488829f560dc949a8b2fb789f3cdc05f5d
Date: 2020-06-17T21:17:14.222Z (1 wk ago)
Electron: 7.3.1
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Darwin x64 19.4.0
  • OS Version:
macOS 10.15.4 (19E2269)

Steps to Reproduce:

  1. hover over a JSDoc with an @example with an @ in the example (eg. an organisation package import)
  2. see the error:

Screenshot 2020-06-30 12 51 17

As you can see from the screenshot, the whole code @example is broken because it's written require('@google-cloud/storage') inside the example.

Does this issue occur when all extensions are disabled?: Yes

@mjbvz mjbvz transferred this issue from microsoft/vscode Jul 2, 2020
@mjbvz
Copy link
Contributor

mjbvz commented Jul 2, 2020

Minimal example:

/**
 * @example
 * class Foo {
 *  @bla foo() { }
 * }
 */
function foo() { }

Results in:

[Trace  - 01:04:36.990] <semantic> Response received: quickinfo (513). Request took 2 ms. Success: true 
Result: {
    "kind": "function",
    "kindModifiers": "",
    "start": {
        "line": 7,
        "offset": 10
    },
    "end": {
        "line": 7,
        "offset": 13
    },
    "displayString": "function foo(): void",
    "documentation": "",
    "tags": [
        {
            "name": "example",
            "text": "class Foo {"
        },
        {
            "name": "bla",
            "text": "foo() { }\r}"
        }
    ]
}

@mjbvz mjbvz removed their assignment Jul 2, 2020
@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jul 7, 2020
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jul 7, 2020
@RyanCavanaugh
Copy link
Member

@mjbvz What exactly is the proposed behavior change here? This seems fundamentally unsolvable - we have no idea whether @bla is a decorator or a new JS Doc tag (it could easily be a decorator named @param), and parsing @example with a real parser is going to end up "swallowing" a lot of example snippets that aren't brace- or paren-balanced.

@mjbvz
Copy link
Contributor

mjbvz commented Jul 7, 2020

No proposal at the moment, just moving upstream so we can collect more feedback on the problem

@mesqueeb
Copy link
Author

mesqueeb commented Jul 7, 2020

@RyanCavanaugh I think if you can check if there there is any other characters other than * (space - star - space) in front of the @ + the previous tag is a @example one, you'd be able to differentiate them!

What do you think ? : )

It's especially annoying when we use organisation scopes in import statements:

/**
 * @example
 * import { something } from '@my-org/my-package'
 */

Thanks so much!!

@mesqueeb
Copy link
Author

@RyanCavanaugh I had another idea! Make it sure VSCode checks any @ tag if it's an existing JSDoc tag or not! If not, and it's behind an @example tag, you're pretty sure it's part of the example!! ;)

This seems fundamentally unsolvable

What do you think about my suggestions, still fundamentally unsolvable? : O

@sandersn
Copy link
Member

In the latest discussion with @mjbvz, we came up with two plausible fixes:

  1. Do not let @ start a tag after non-whitespace.
  2. Do not let @ start a tag anywhere after the beginning of a line. [1]

But we wanted to know how people have used non-initial @ in the past. That's where I come in.

The basic problem here is to survey how JSDoc tags are used not at the end beginning of a line, and estimate how often each usage occurs.
To do this, I scanned a fully npm installed Definitely Typed and Typescript user tests, looking both at Javascript and Typescript.
I used a simple regex looking for @ after * after non-whitespace, which is actually not too far off Typescript's existing parsing.
I got 50,000 hits with no deduping -- this is significant since common dependencies show up multiple times.
As a result, I'm not only going to use rough numbers in the rest of this analysis. I don't have time to do more right now, and I don't think it will change my recommendations significantly.

Of those 50,000 hits, about 38,000 were for tags Typescript already handles correctly: @link and email addresses in the @author tag. That left 12,000 other hits.

@ after non-whitespace

For fix (1), I looked at the 7,000+ hits where non-initial @ occurred after non-whitespace. I didn't see any JSDoc tags, although I was of course just reading through a text file and could have missed some.
I did see:

  • Backquoted tags, (which our parser should handle correctly, except for when it's buggy)
  • Backquoted scoped packages (same)
  • Backquoted email addresses (same)
  • Scoped packages used in code snippets
  • Unquoted email addresses
  • Usernames, quoted in various ways

So I think it's safe to say that the JSDoc parser should not treat @ after non-whitespace as starting a tag.

@ after the beginning of a line

For the remaining almost-5,000 hits, there was at least one non-initial @ preceded by whitespace on each line.
About 80% of these should not be treated tags:

  • Over half were scoped packages.
  • 10-20% (I didn't count, just eyeballed it) were usernames.
  • Another 10%-ish were meta mentions of language constructs, from such popular languages as:
    • CSS
    • JSDoc
    • ESNext
    • ES internal slots
    • Active Directory
    • SQL
    • GraphQL
    • some obscure Google language

The other 20% should be treated as tags. These usages fell into 3 categories:

  • @default and @since were used postfix in Typescript files in a scattering of places
  • @see was used inline as if it were @link, again only in Typescript files.
  • Double tags were used quite a bit, but only in big projects used with (1) Closure (2) checkJS (3) TSDoc:
    • api-extractor: /** @beta @override */
    • selenium-webdriver/lib/input.js: /** @private @const */
    • webpack/lib/library/UmdLibraryPlugin.js: /** @template T @typedef {import("./AbstractLibraryPlugin").LibraryContext<T>} LibraryContext<T> */
    • also @types/react for some reason: @see aria-pressed @see aria-selected

Recommendation

We should change the parser so that @ does not begin a tag when preceded by any whitespace.

The other usages support strictness at about 80%/20%. For a change with a big payoff, that's a reasonable ratio, especially considering how many of the projects in the 20% are customers that we have contact with. The fix is also simple: you just need to run a regex search/replace on your source, like sed. However, I don't see that big of a payoff for this change. The core of this bug is that unintended tags disrupt code examples. Unintended tags also interrupt text, but not as badly.

For now, we should not change the parser so that @ only begins a tag at the beginning of a line.

[1] "the beginning of a line" has a complex definition in the actual TS parser. Basically, it means "skip whitespace, an asterisk, and more whitespace", except all 3 of those are optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants