JS: Add ModuleImportNode::Range#945
Conversation
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
LGTM modulo the missing evaluation, and some very minor docstring comments.
I assume the big diff is just due to moving code around.
|
|
||
| module ModuleImportNode { | ||
| /** | ||
| * Data flow node that refers to an imported module. |
There was a problem hiding this comment.
Missing A?
A data flow node that refers to an imported module.
| * Gets a (default) import of the module with the given path, such as `require("fs")` | ||
| * or `import * as fs from "fs"`. | ||
| * | ||
| * This predicate can be customized by subclassing `ModuleImportNode::Range`. |
There was a problem hiding this comment.
Do we use "customized" like this anywhere else?
You can only add ModuleImportNodes to this predicate when subclassing ModuleImportNode::Range, so "customize" seems like a stretch.
There was a problem hiding this comment.
Changed to "extended"
ghost
added
the
| /** | ||
| * Data flow node that refers to an imported module. | ||
| */ | ||
| abstract class Range extends DataFlow::SourceNode { |
There was a problem hiding this comment.
Do we need this to extend DataFlow::SourceNode itself, or could it extend DataFlow::SourceNode::Range instead?
There was a problem hiding this comment.
Extending SourceNode::Range will add more dependencies, which could lead to negative recursion or performance issues. I believe there was only a one-way dependency between ModuleImportNode and SourceNode previously so it seems safer to keep it that way.
|
LGTM, modulo minor comments. I don't think this needs an evaluation, but if you want to do a quick sanity check I won't stand in your way. |
asger-semmle
force-pushed
the
extensible-module-import
branch
from
7dd02c1 to
707886f
Compare
|
Evaluation with just Xss.ql looks fine. I also checked the profile viewer and they look identical. |
3 participants
Will conflict with #934 which is undergoing a long evaluation. Will evaluate this after rebasing on that.
Would very much appreciate it if this could make it into 1.20, which is why I'm opening this ASAP.