Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jun 5, 2020

The unneeded defensive code query would sometimes flag the return value of a function as being a defensive code pattern, which doesn't seem like it qualifies as a defensive code pattern.

This "fixes" #3626 although in a somewhat indirect way.

@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Jun 5, 2020
@asgerf asgerf requested a review from a team as a code owner June 5, 2020 10:03
This was linked to issues Jun 5, 2020
@erik-krogh
Copy link
Contributor

LGTM.
Change note?

@asgerf asgerf force-pushed the js/unneeded-defensive-return branch 2 times, most recently from 3d14c8b to 6750cf7 Compare June 5, 2020 15:39
erik-krogh
erik-krogh previously approved these changes Jun 22, 2020
@asgerf asgerf force-pushed the js/unneeded-defensive-return branch from 6750cf7 to a109c1f Compare June 25, 2020 10:05
@asgerf
Copy link
Contributor Author

asgerf commented Jun 25, 2020

The initial evaluation looked bad. Apart from the performance (possibly noise), we lost two TPs due to ignoring returned expressions.

I've reverted the change to returned expressions and left in the special-cased check for document.all, and added regression tests for the TPs we lost. Smoke test looks fine.

@semmle-qlci semmle-qlci merged commit cf0cd00 into github:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LGTM.com - false positive LGTM.com - false positive
3 participants