Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 9, 2019
Merged

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

merged 1 commit into from
Jan 9, 2019

Conversation

xiemaisi
Copy link

@xiemaisi xiemaisi commented Jan 7, 2019

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.

@xiemaisi xiemaisi added the JS label Jan 7, 2019
@xiemaisi xiemaisi requested a review from a team as a code owner January 7, 2019 12:13
ghost
ghost reviewed Jan 7, 2019
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

xiemaisi commented Jan 7, 2019

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
Copy link
Contributor

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
Copy link

ghost commented Jan 7, 2019

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

JavaScript: Restructure implementation of DataFlow::SourceNode.
de42975 
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 as well.
@xiemaisi xiemaisi force-pushed the js/restructure-sourcenode branch from ef1d08d to de42975 Compare January 8, 2019 08:02
@xiemaisi
Copy link
Author

xiemaisi commented Jan 8, 2019

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
Copy link

ghost commented Jan 8, 2019

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

@semmle-qlci semmle-qlci merged commit 6886474 into github:master Jan 9, 2019
@xiemaisi xiemaisi deleted the js/restructure-sourcenode branch January 10, 2019 16:25
cklin pushed a commit that referenced this pull request May 20, 2022
@owen-mc
Merge pull request #727 from cklin/fix-ql-comments-syntax
ee94eb5 
Fix Beego references
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees
No one assigned
Labels
JS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xiemaisi @asger-semmle @semmle-qlci