Skip to content

JS: introduce Expr:getUnderlyingX#345

Merged
semmle-qlci merged 7 commits into
masterfrom
unknown repository
Oct 30, 2018
Merged

JS: introduce Expr:getUnderlyingX#345
semmle-qlci merged 7 commits into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Oct 23, 2018

This PR introduces Expr::getUnderlyingValue and its sibling Expr::getUnderlyingReference as the semantic versions of what Expr::stripParens most often is used for.

I have two unopened PRs that make use of the features of this PR, so I would like it merged soon.

I have another stack of commits that replace most Expr::stripParens uses with Expr::getunderlyingValue and Expr:getUnderlyingReference throughout the code base. But I would like to wait with merging that since the performance evaluation of that is unstable, and since a timeout has started to consistently happen on the base commit as well.

@ghost ghost added the JS label Oct 23, 2018
@ghost ghost self-requested a review as a code owner October 23, 2018 06:38
Copy link
Copy Markdown

@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.

Basically fine, modulo some details. I'm not entirely sure about the "value" terminology, given that the predicate returns an expression, but I guess it makes sense in tandem with "reference".

Comment thread javascript/ql/src/semmle/javascript/Expr.qll
Comment thread javascript/ql/src/semmle/javascript/Expr.qll
Comment thread javascript/ql/src/semmle/javascript/TypeScript.qll
@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 23, 2018

Comments addressed.


/**
* Gets the innermost reference that this expression evaluates to, if any.
* For example, `o.m` of `(o.m)`, but not of `(0, o.m)`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd prefer a more complete listing of all covered cases (also for getUnderlyingValue).

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.

Done.

xiemaisi
xiemaisi previously approved these changes Oct 24, 2018
@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 26, 2018

Rebased to resolve expected output conflict.

xiemaisi
xiemaisi previously approved these changes Oct 26, 2018
@semmle-qlci semmle-qlci merged commit 1509752 into github:master Oct 30, 2018
smowton pushed a commit to smowton/codeql that referenced this pull request Apr 16, 2022
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