JavaScript: Restructure implementation of DataFlow::SourceNode.#727

xiemaisi

It now uses a facade pattern similar to InvokeNode: the range of the class is defined by an abstract class DataFlow::SourceNode::Range, while the actual behaviour is defined by the (no longer abstract) SourceNode class itself.

Clients that want to add new source nodes need to extend DataFlow::SourceNode::Range, those that want to refine the behaviour of existing source nodes should extend DataFlow::SourceNode itself.

While this is technically a breaking API change, I think separating the two aspects in this way is cleaner and makes it easier to use, and improves performance (internal link) as well.

ghost

Implementation LGTM.

The improvements seem impressive. Well done.

Are you sure that the improvements aren't due to the change I have flagged as unrelated to the purpose of this PR?

For the record: the introduction of yet another style/naming scheme for working around the quirks of abstract classes is a bit unfortunate. We now have:

InvokeNode/InvokeNodeDef
ClientRequest/CustomClientRequest
SourceNode/SourceNode::Range
probably more

xiemaisi

the introduction of yet another naming scheme (...) is a bit unfortunate

I agree. Which of the other ones would you like me to switch to?

asger-semmle

I like this change a lot. I also like the ::Range better than the other conventions so far, so maybe the change others to use that.

Long ago I started a write-up on a canonical naming scheme here but I gave up on rolling it out after the miserable performance I saw in the string concatenation library.

ghost

I also prefer the ::Range scheme, let's use that from now on, and eventually migrate the other schemes.

xiemaisi

I've rebased to resolve conflicts with the autoformatting PR.

Two further minor API-related questions are perhaps worth discussing:

Should we introduce an empty abstract class CustomSourceNode extends SourceNode::Range and tell people to extend that instead of SourceNode::Range itself? The name might be slightly more intuitive, but of course it's functionally much the same as just extending SourceNode::Range itself.
Should we introduce a field SourceNode::Range impl in SourceNode like we have in InvokeNode? It wouldn't currently be useful for anything since SourceNode::Range doesn't implement any member predicates so we don't need impl outside the characteristic predicate, but it would be a step towards a more unified design pattern.

ghost

I think we should keep the API as is:

I think the ::Range design is the most intuitive for external parties, and we can always introduce a private ::InternalRange if needed
the field/instanceof implementation can always be changed, the current design is consistent with the rest of the code base

xiemaisi added the JS label Jan 7, 2019

xiemaisi requested a review from a team as a code owner January 7, 2019 12:13

ghost reviewed Jan 7, 2019

View reviewed changes

xiemaisi force-pushed the js/restructure-sourcenode branch from ef1d08d to de42975 Compare January 8, 2019 08:02

ghost approved these changes Jan 8, 2019

View reviewed changes

semmle-qlci merged commit 6886474 into github:master Jan 9, 2019

xiemaisi deleted the js/restructure-sourcenode branch January 10, 2019 16:25

cklin pushed a commit that referenced this pull request May 20, 2022

Merge pull request #727 from cklin/fix-ql-comments-syntax …

ee94eb5

Fix Beego references

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JavaScript: Restructure implementation of DataFlow::SourceNode.#727