Skip to content

JS: Add data-flow from yield in generators.#4134

Merged
codeql-ci merged 8 commits intogithub:mainfrom
erik-krogh:genCalls
Sep 2, 2020
Merged

JS: Add data-flow from yield in generators.#4134
codeql-ci merged 8 commits intogithub:mainfrom
erik-krogh:genCalls

Conversation

@erik-krogh
Copy link
Contributor

This is a followup to the support I added for async functions.

@erik-krogh erik-krogh added the JS label Aug 25, 2020
@erik-krogh erik-krogh changed the title JS: Add support for return from yield in generators. JS: Add data-flow from yield in generators. Aug 26, 2020
@erik-krogh erik-krogh marked this pull request as ready for review August 26, 2020 11:04
@erik-krogh erik-krogh requested a review from a team as a code owner August 26, 2020 11:04
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Looking good! Could you run a quick evaluation?

predicate isAsync() { isAsync(this) }

/** Holds if this function is not asynchronous and also not a generator. */
predicate isOrdinary() { not isAsync() and not isGenerator() }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say there are too many kinds of Function instances to have a predicate named isOrdinary. Implicit class constructors, abstract methods, bodiless functions in ambient context, function signatures etc, all seem non-ordinary in their own way.

I'd suggest isAsyncOrGenerator and then replacing the calls with not f.isAsyncOrGenerator()

predicate localExceptionStepWithAsyncFlag(DataFlow::Node pred, DataFlow::Node succ, boolean async) {
exists(DataFlow::Node target | target = getThrowTarget(pred) |
async = false and
async = false and // this also covers generators - as the behavior of exceptions is close enough to the behavior of ordinary functions when it comes to exceptions (assuming that the iterator does not cross function boundaries).
Copy link
Contributor

Choose a reason for hiding this comment

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

Break the comment into multiple lines

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Sep 1, 2020
@erik-krogh
Copy link
Contributor Author

Looking good! Could you run a quick evaluation?

Evaluation looks fine

@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Sep 1, 2020
@codeql-ci codeql-ci merged commit c017308 into github:main Sep 2, 2020
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