Skip to content

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Mar 3, 2022

Also added a new

predicate levelStep(Node pred, Node succ) { none() }

to the interface to allow for proper level steps.

@hvitved hvitved marked this pull request as ready for review March 3, 2022 11:33
@hvitved hvitved requested review from a team as code owners March 3, 2022 11:33
@hvitved hvitved added Python Ruby no-change-note-required This PR does not need a change note labels Mar 3, 2022
exists(string p | step = StoreStep(p) and content = "" and result = MkTypeTracker(hasCall, p))
or
step = JumpStep() and
result = MkTypeTracker(false, content)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the JavaScript implementation of TypeTracker.append I see that none of the steps ever clear the hasCall property and vice-versa for hasReturn in TypeBackTracker.prepend. @asgerf , what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

None of our type-tracking steps are jump-steps. No fundamental reason not to have them, we just haven't added them.

Note we handle captured variables differently; they are not modelled as jump steps (which is too imprecise for JS code). Also, when writing a type-tracking predicate, you may choose to manually reset the calling context using t.start() (I'm working on a doc describing the importance of this in more detail). Perhaps this is why it never seemed important for JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

@asgerf Thanks for the explanation. Just wanted to be sure that there wasn't a fundamental reason why resetting the hasCall/hasReturn flags would be bad.

Copy link
Contributor Author

@hvitved hvitved Mar 3, 2022

Choose a reason for hiding this comment

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

Also, when writing a type-tracking predicate, you may choose to manually reset the calling context using t.start()

Doesn't that also clear whether or not data is stored in a property?

Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Looks reasonable. The breadth of the DCA run for Python could be a bit bigger, though. We usually opt for nightly/nightly for the queries and sources.

@yoff
Copy link
Contributor

yoff commented Mar 4, 2022

Looks like the experiment has to be re-run. I am curious to see the results since the code change looks good :-)

@hvitved
Copy link
Contributor Author

hvitved commented Mar 7, 2022

The breadth of the DCA run for Python could be a bit bigger, though. We usually opt for nightly/nightly for the queries and sources.

I have rerun DCA. Does it look good to you?

@yoff
Copy link
Contributor

yoff commented Mar 7, 2022

DCA run looks good to me 👍

@hvitved hvitved merged commit c1db0a9 into github:main Mar 7, 2022
@hvitved hvitved deleted the typetracker/jump-step branch March 7, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Python Ruby

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants