JavaScript: Model util.deprecate as a pre call-graph step.#3809
Conversation
| } | ||
|
|
||
| export const exportedConnection = getConnection(); | ||
| export const exportedConnection = require("util").deprecate(getConnection(), "don't use this"); |
There was a problem hiding this comment.
Hm, this test is a little misleading as the argument to deprecate should be a function - it just seems to return an empty object otherwise. And we currently don't type-track plain functions (my latest attempt didn't scale on gecko), so I'm not sure this change would have the intended effect.
So this probably wasn't the perfect use-case for PreCallGraphStep like I thought it was, but I still feel like it should be. I'm happy to merge like this if it works for flow-summaries, and I'll try and poke at the call graph builder again to make it work.
Could you add a test that actually passes a function to util.deprecate? (to show the FN)
There was a problem hiding this comment.
Ah, that was just me being lazy and trying to shoe-horn this onto an existing test instead of making a new one. Will fix.
It's a good point about util.deprecate discarding non-functions, though. That makes it a little suspect as a general-purpose data-flow step, though I can't immediately think of a scenario where it would cause false positives.
There was a problem hiding this comment.
Tests added; please take a look.
4a12edc to
b9d52b4
Compare
b9d52b4 to
640c194
Compare
|
@asgerf, could I get a re-approval? I had to slightly adjust the regex in the test since Java (and hence QL) doesn't support quantifiers in lookbehinds. No idea why it initially worked on my machine; maybe different Java versions. |
The motivation for this is the flow-summaries work, where we would be able to infer a summary for an (old) npm package if it weren't for a pesky
util.deprecateintervening. I have no evidence that it will move the needle on actual alerts, so I didn't include a change note, but I can certainly add one if desired.