Skip to content

JS: ignore mocked callees for js/superfluous-trailing-arguments#1668

Merged
semmle-qlci merged 1 commit intomasterfrom
unknown repository
Aug 2, 2019
Merged

JS: ignore mocked callees for js/superfluous-trailing-arguments#1668
semmle-qlci merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Aug 1, 2019

This change whitelists calls with arguments to parameterless functions that trivially throw an exception for the js/superfluous-trailing-arguments query. Such functions are considered placeholders for something else.

Example:

const placeholder = () => {
  throw new Error("Remove this statement and implement this function");
};
placeholder(42, 39); // should not be flagged

The standard evaluations is uneventful. But a run on 200 LGTM projects shows that this change eliminates all of these alerts: https://lgtm.com/query/17012648499980566/?pageIndex=0.

(This change is inspired by the anomaly analysis, I did not open the PR before going on holiday)

@ghost ghost added the JS label Aug 1, 2019
@ghost ghost self-requested a review as a code owner August 1, 2019 09:27
@xiemaisi
Copy link

xiemaisi commented Aug 1, 2019

I assume you have seen cases where the placeholder function doesn't just have a single throw statement as its body? (That would seem to be a simpler criterion which I would have tried.)

@ghost
Copy link
Author

ghost commented Aug 1, 2019

Good question. The motivating anomalies are long gone from memory. But the tests are happy with the simpler not f.getBodyStmt(0) instanceof ThrowStmt (which deliberately does not check the statement count) - should we go with that instead?

@xiemaisi
Copy link

xiemaisi commented Aug 1, 2019

Yeah, let's do that instead.

@ghost
Copy link
Author

ghost commented Aug 1, 2019

Force-pushed the change.

@semmle-qlci semmle-qlci merged commit 1b30a25 into github:master Aug 2, 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.

2 participants

Comments