JS: introduce DataFlow::ClassNode #764

asger-semmle · 2019-01-14T15:42:39Z

Spin-off from #760

ghost

LGTM. Two minor documentation concerns.
This requires a change note and a thorough evaluation.
If the taint tracking experiments with this change are inconclusive, you could try porting js/mixed-static-instance-this-access to get an additional data point.

ghost · 2019-01-15T08:45:44Z

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

+ * A data flow node corresponding to a class definition or a function definition
+ * acting as a class.
+ *
+ * The following patterns are recognized as classes and methods:


Do we want a note here about how to extend the set of recognized patterns?

ghost · 2019-01-15T08:54:00Z

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

+    /**
+     * Gets an instance method with the given name, if any.
+     */
+    abstract FunctionNode getAnInstanceMethod(string name);


I think we should mention that the prototype chains are not traversed for these lookups.
I am not sure if the prototype traversal should be implicit or explicit one day.

asger-semmle · 2019-01-15T09:58:35Z

To clarify, this PR just contains the ClassNode as you requested it be split into its own PR. There is no code using it, so it can't be evaluated on its own.

And this is exactly why I included it in #760.

you could try porting js/mixed-static-instance-this-access to get an additional data point.

I don't see why I should first put this in its own PR and then bundle it something else? If we wanted it bundled with the use-case, we had #760.

xiemaisi

LGTM, with minor suggestion for simplification.

xiemaisi · 2019-01-15T11:54:57Z

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

+        result = method.getBody().flow()
+      )
+      or
+      result = getAPropertyWrite(name).getRhs().getALocalSource()


Suggested change

result = getAPropertyWrite(name).getRhs().getALocalSource()

result = getAPropertySource(name)

Ditto below.

ghost · 2019-01-15T12:42:42Z

I don't see why I should first put this in its own PR and then bundle it something else? If we wanted it bundled with the use-case, we had #760.

To clarify:
I agree that the performance for this change probably is fine, but:

The performance of #760 was characterized as:

Evaluation shows no result changes but a little bit of performance to investigate.

I suspect the performance hiccups are mainly caused by to the two other commits of #760 then. The suggested port of js/mixed-static-instance-this-access would have produced a cleaner data point.
(Furthermore, this is even a significant rewrite of the ClassNode compared to the version used in the evaluation of #760.)

asger-semmle · 2019-01-16T11:27:55Z

I tried integrating it with js/mixed-static-instance-this-access but we get a large number of false positives.
Edit: this commit was used for the evaluation.

One particular problem is that we can't tell if a static member is intended to be used as a "named constructor", like here.

ghost · 2019-01-16T11:34:34Z

Hmm. Thanks for trying. But at least it is now clear that the implementation scales.

asger-semmle · 2019-01-16T11:39:48Z

I've pushed a commit that adds getADirectSuperClass() since it was needed for the query so we might as well keep it, even if the query changes didn't work out.

ghost

Changes LGTM, but there is one test failure.

javascript/ql/test/library-tests/Classes/ClassNodeStaticMethod.ql

xiemaisi · 2019-01-18T09:00:07Z

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

+   *
+   * Does not include methods from superclasses.
+   */
+  FunctionNode getAnInstanceMethod(string name) { result = impl.getAnInstanceMethod(name) }


Here and below, I wonder whether it is really worth including "a"/"an" in the predicate name and doc comment. Of course, it is technically correct since we don't assume/enforce uniqueness, but in the majority of cases we probably would expect the result to be unique, so having an overly general name might lead to confusion and bad auto-complete experience.

What do you think?

(We should, of course, still mention the possibility of multiple results in the doc comment.)

I considered this as well, for the same reasons, but I think "a"/"an" should be used here.

Some arguments for:

the rest of the SourceNode getters use that convention

multiple prototypes are possible

the method definitions may be conditional

unlike ES6 classes, these class nodes are not always syntactic, the "a"/"an" prefix kind of reflects that

if we later add support for lookups in the prototype hierarchy, I think we want the "a/an" prefix, so it would be nice if we had some consistency between similar lookups

(this list is probably a rehash of what you already know)

I'm also against the a/an prefix. Even if it's technically possible, it's confusing and extremely rare in practice, and inconsistent with other APIs for things that are unique in practice.

Ok. Let us remove it then.

asger-semmle · 2019-01-18T15:51:42Z

I've reverted the a/an prefix from getInstanceMethod and getStaticMethod.

I've also excluded getters/setters from these predicates as that was the main situation where they had multiple returns, but it was in general unfortunate that you couldn't tell them apart using this API. I have instead introduced getInstanceMember and MemberKind to allow chainable access to any combination of methods/getter/setters. It's unfortunate that no commonly used name exists for "method or accessor" so I've just stuck with "member", even though constructors and fields aren't included.

Feel free to suggest another terminology, keeping in mind that DataFlow::MemberKind::method() is already quite a mouthful, so overly long names might not be the answer.

@esben-semmle @xiemaisi could you take another look at the latest commit?

ghost · 2019-01-20T20:58:47Z

This looks good to go, but have you sanity checked the performance of the latest commit?

ghost · 2019-01-21T06:47:46Z

Never mind. I keep forgetting that this is unevaluable..

asger-semmle requested a review from a team as a code owner January 14, 2019 15:42

ghost reviewed Jan 15, 2019

View reviewed changes

xiemaisi suggested changes Jan 15, 2019

View reviewed changes

ghost suggested changes Jan 17, 2019

View reviewed changes

javascript/ql/test/library-tests/Classes/ClassNodeStaticMethod.ql Show resolved Hide resolved

ghost previously approved these changes Jan 17, 2019

View reviewed changes

xiemaisi reviewed Jan 18, 2019

View reviewed changes

asger-semmle added 7 commits January 18, 2019 14:46

JS: introduce DataFlow::ClassNode

5b7675d

JS: split ClassNode into two classes

3cb2341

JS: make ClassNode::Range abstract

1eb0ca4

JS: remove isAmbient() check

cc1204a

JS: address comments

c82690f

JS: add getADirectSuperClass()

e18b635

JS: handle accessors separately

4b4daa6

asger-semmle dismissed ghost ’s stale review via 4b4daa6 January 18, 2019 15:42

asger-semmle force-pushed the dataflow-classnode branch from e66fe7e to 4b4daa6 Compare January 18, 2019 15:42

xiemaisi approved these changes Jan 18, 2019

View reviewed changes

ghost approved these changes Jan 20, 2019

View reviewed changes

ghost added the JS label Jan 20, 2019

semmle-qlci merged commit 0432b01 into github:master Jan 21, 2019

	result = getAPropertyWrite(name).getRhs().getALocalSource()
	result = getAPropertySource(name)

JS: introduce DataFlow::ClassNode #764

JS: introduce DataFlow::ClassNode #764

Uh oh!

Conversation

asger-semmle commented Jan 14, 2019

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asger-semmle commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 15, 2019

Uh oh!

asger-semmle commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 16, 2019

Uh oh!

asger-semmle commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asger-semmle commented Jan 18, 2019

Uh oh!

ghost commented Jan 20, 2019

Uh oh!

ghost commented Jan 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asger-semmle commented Jan 15, 2019 •

edited

Loading

asger-semmle commented Jan 16, 2019 •

edited

Loading

asger-semmle commented Jan 16, 2019 •

edited

Loading