Skip to content

Detect and prevent creation of bad Identifier#21581

Merged
1 commit merged into
masterfrom
bad_identifier
Feb 5, 2018
Merged

Detect and prevent creation of bad Identifier#21581
1 commit merged into
masterfrom
bad_identifier

Conversation

@ghost
Copy link
Copy Markdown

@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
//// function f(o) { return o.nested.[|great|]; }

verify.rangesReferenceEachOther();

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

//// function f(o) { return o.nested.[|great|]; }

verify.rangesReferenceEachOther();

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.

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