Skip to content

Detect and prevent creation of bad Identifier #21581

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
1 commit merged into from
Feb 5, 2018
Merged

Detect and prevent creation of bad Identifier #21581

1 commit merged into from
Feb 5, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 2, 2018

Fixes #20792
In Node.getChildren() we create synthetic nodes for all of the tokens that weren't parsed. This can cause a problem if we scan an Identifier because we don't give it any text here (and besides, an Identifier shouldn't be trivia). After adding an assertion and running all tests, I found that there was one kind of jsdoc node which we weren't avoiding scanning trivia for -- we need to avoid scanning trivia for these because arbitrary tokens could appear in a comment.

@ghost ghost requested a review from sandersn February 2, 2018 19:48
@@ -14,9 +14,3 @@
//// function f(o) { return o.nested.[|great|]; }

verify.rangesReferenceEachOther();

Copy link
Author

Choose a reason for hiding this comment

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

This was the test that broke under the new assertion. I'm just removing a comment here that does nothing (it only has two slashes).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I must have left it from earlier testing and checked it in by mistake. But it would probably be good to retain this as a parser test.

Copy link
Author

@ghost ghost Feb 5, 2018

Choose a reason for hiding this comment

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

By "this" I meant the above code. This below is a regular comment that doesn't test anything.

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.

Good as-is, but it would be nice to add a parsing test too.

@@ -14,9 +14,3 @@
//// function f(o) { return o.nested.[|great|]; }

verify.rangesReferenceEachOther();

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I must have left it from earlier testing and checked it in by mistake. But it would probably be good to retain this as a parser test.

@ghost ghost merged commit 14bd0a2 into master Feb 5, 2018
@ghost ghost deleted the bad_identifier branch February 5, 2018 17:13
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This pull request was closed.
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.

1 participant