Skip to content

JS: Fix source locations for JSDoc type annotations#1528

Merged
semmle-qlci merged 12 commits intogithub:rc/1.21from
asger-semmle:jsdoc-source-location-fix
Jul 3, 2019
Merged

JS: Fix source locations for JSDoc type annotations#1528
semmle-qlci merged 12 commits intogithub:rc/1.21from
asger-semmle:jsdoc-source-location-fix

Conversation

@asger-semmle
Copy link
Contributor

In 1.20 we added support for JSDoc source locations, but the locations are relative to the surrounding tag, not absolute within the file.

The fix required some substantial changes to the doctrine-based parser, as the parsed string wasn't always a true substring of the source code. For instance, before parsing a type, all line breaks and indentation and leading *s were stripped out, which made it impractical to maintain source locations.

Fixes ODASA-8011

@asger-semmle asger-semmle added the JS label Jul 1, 2019
@asger-semmle asger-semmle added this to the 1.21.1 milestone Jul 1, 2019
@asger-semmle asger-semmle requested a review from a team as a code owner July 1, 2019 10:52
Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

Wow, kudos for fixing this, that can't have been fun! I only have one extremely minor niggle, otherwise I trust your testing.

/**
* Returns the absolute position of the end of the previous token.
*
* This can differ from the start of the current token case the two tokens
Copy link

Choose a reason for hiding this comment

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

Suggested change
* This can differ from the start of the current token case the two tokens
* This can differ from the start of the current token in case the two tokens

@semmle-qlci semmle-qlci merged commit 02bded3 into github:rc/1.21 Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants