Skip to content

Remove error on invalid jsdoc tokens#32769

Merged
sandersn merged 5 commits intomasterfrom
fix-error-on-invalid-jsdoc-tokens
Aug 9, 2019
Merged

Remove error on invalid jsdoc tokens#32769
sandersn merged 5 commits intomasterfrom
fix-error-on-invalid-jsdoc-tokens

Conversation

@sandersn
Copy link
Copy Markdown
Member

@sandersn sandersn commented Aug 8, 2019

In JSDoc:

  1. In the scanner, don't issue an error, even for invalid identifiers.
  2. In the binder, don't issue an error for reserved (but otherwise valid)
    identifiers.
/**
 * Example of 1: "\"
 * Example of 2: @private
 */

I'm not sure of the fix in the binder. It might be better to avoid setting originalKeywordKind in the parser for bad identifiers in jsdoc, but that seems wrong because the jsdoc code path really should behave just like the normal one so that it's easy to remove later.

Fixes #32756

In JSDoc:

1. In the scanner, don't issue an error, even for invalid identifiers.
2. In the binder, don't issue an error for reserved (but otherwise valid)
identifiers.

/**
 * Example of 1: "\"
 * Example of 2: @Private
 */
@sandersn sandersn requested a review from weswigham August 8, 2019 22:08
Comment thread src/compiler/scanner.ts
return token = getIdentifierToken();
}
error(Diagnostics.Invalid_character);
pos++;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we also be ignoring errors on invalid Unicode escapes, like \u{1010101010}? That'll require passing a flag into scanIdentifierParts and the other callers of scanExtendedUnicodeEscape.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can't get any error with the current code! I compared

/**
 * unicode-escape = \u{abcdefghi} -- should not have error for invalid unicode escape
 */
var hi = "\u{00fa}\u{abcdefghi}"

There's an error for the second abcdefghi but not the first.

I'll add a test to make sure it stays that way.

Copy link
Copy Markdown
Member

@weswigham weswigham Aug 9, 2019

Choose a reason for hiding this comment

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

You should also check, just in case, the scenario when the unicode escape is the second character in the identifier eg, q\u{abcdefghi} - The first character is handled inline in scanJsDocToken with a non-erroring peek (so it can fallback to the Invalid character error you just removed), while the second character (and beyond) is handled in scanIdentifierParts which, could always behave different.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

@sandersn sandersn merged commit 85b8d27 into master Aug 9, 2019
@sandersn sandersn deleted the fix-error-on-invalid-jsdoc-tokens branch August 9, 2019 19:53
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scanner change in #32716 reports errors on jsdoc

2 participants