Skip to content

JS: add DataFlow::getEnclosingExpr to get Expr for potentially reflective calls #2270

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 13, 2019
Merged

JS: add DataFlow::getEnclosingExpr to get Expr for potentially reflective calls #2270

merged 3 commits into from
Nov 13, 2019

Conversation

erik-krogh
Copy link
Contributor

Based on previous discussion.

Suggestions for the name of the new method are welcome.

I evaluated onUseOfReturnlessFunction, which is now the only query using this new method.
Evaluation shows no performance degradation, and removal of a whole bunch of FP's.

When I made the UseOfReturnlessFunction query I think I found the benchmarks I tested on using an initial query that had exists(call.asExpr()), and I therefore missed the benchmarks with lots of reflective calls.

@erik-krogh erik-krogh requested a review from a team as a code owner November 7, 2019 10:07
@max-schaefer
Copy link
Contributor

Apologies, I'm a bit late to this discussion, but I would actually favour just extending asExpr() to cover this as suggested by @asger-semmle here. I'm not convinced that it's a bigger problem for asExpr() to be non-injective than to have two subtly different predicates that do the same thing most of the time. Do we have an example where we rely on the current semantics of asExpr()?

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Nov 7, 2019 •
edited
Loading

I would actually favour just extending asExpr() to cover this.

It definitely seems better to have one method instead of two.

Doing it in asExpr() will definitely affect some results and requires a thorough evaluation.
I just started such an evaluation.

@erik-krogh erik-krogh mentioned this pull request Nov 11, 2019
Closed
@max-schaefer
Copy link
Contributor

Fixes #2288.

@erik-krogh
Copy link
Contributor Author

As pr. discussion: Lets land this, and then I'll do general changes to asExpr() in a followup PR once my evaluation of it finishes.

@erik-krogh
Copy link
Contributor Author

So I did a pretty big evaluation.

I compared a version where just UseOfReturnlessFunction used the new asExpr() method, and a version where the asExpr() is changed for everyone.

There is no change in results, but there is a nasty decrease in performance.

max-schaefer
max-schaefer approved these changes Nov 13, 2019
View reviewed changes
Copy link
Contributor

@max-schaefer max-schaefer left a comment

Choose a reason for hiding this comment

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

LGTM as a surgical fix.

@max-schaefer
Copy link
Contributor

but there is a nasty decrease in performance.

That's a shame, but hopefully something we can fix. It does seem like the right thing to do.

@max-schaefer max-schaefer added this to the 1.23 milestone Nov 13, 2019
@semmle-qlci semmle-qlci merged commit b11a742 into github:master Nov 13, 2019
@erik-krogh erik-krogh mentioned this pull request Nov 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-schaefer max-schaefer max-schaefer approved these changes

Assignees
No one assigned
Labels
JS
Projects
None yet
Milestone
1.23
Development

Successfully merging this pull request may close these issues.
3 participants
@erik-krogh @max-schaefer @semmle-qlci