JS: additional ClientRequest::getADataNodes#310
Conversation
ghost
added
the
ghost
self-requested a review
as a 
xiemaisi
left a comment
xiemaisi
left a comment
There was a problem hiding this comment.
LGTM on the whole, two minor suggestions.
|
|
||
| override DataFlow::Node getUrl() { | ||
| result = url | ||
| result = getArgument(0) and not result instanceof DataFlow::ObjectLiteralNode or |
There was a problem hiding this comment.
I'm not sure about the not result instanceof (...) bit. I see what you're trying to accomplish, but in practice this seems a little too syntactic, so I'd prefer dropping the special-casing.
| none() | ||
| exists (DataFlow::MethodCallNode chained, string name | | ||
| name = "set" or name = "send" or name = "query" | | ||
| chained.getReceiver*() = this and |
There was a problem hiding this comment.
Perhaps we could introduce SourceNode.getAMemberCall() (implemented as getAMemberCall(_)) and then phrase this as chained = getAMemberCall*().getAMethodCall(name)? Alternatively, introducing a new predicate to model the chaining also seems justified.
There was a problem hiding this comment.
Done, but based on getAMethodCall instead of getAMemberCall.
I looked through the other mentions of chained in the code base for refactoring opportunities, but none of the cases were trivial to rewrite to use the new getAChainedMethodCall.
| /** | ||
| * Gets a chained method call that invokes `methodName` last. | ||
| * | ||
| * The chain steps includes only calls that have the syntactic shape of a method call, |
|
Fixed and squashed. |
More uses of `instanceof` in the external/internal AST layer
Extract property references with only backing field
2 participants
Implements the empty
getADataNodes from #300.The changes are isolated, and they can easily be reviewed as a single diff.
The queries in #300 are the only ones that actually use
getADataNode, and they are not enabled by default, so I will evaluate this later when some other #300 holes have been plugged.