Ruby: Adopt shared type tracking library #14709
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hvitved
force-pushed
the
ruby/shared-type-tracking
branch
3 times, most recently
from
8ccdce3 to
40e6a32
Compare
hvitved
force-pushed
the
ruby/shared-type-tracking
branch
13 times, most recently
from
9142613 to
582296d
Compare
hvitved
force-pushed
the
ruby/shared-type-tracking
branch
from
582296d to
6ce8e05
Compare
| */ | ||
| pragma[inline] | ||
| LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) } | ||
| LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t = t2.step(result, this) } |
There was a problem hiding this comment.
Is this change correct? Were the parameters of backtrack swapped? We might want to check that all the places where we use backtrack are still correct.
There was a problem hiding this comment.
Yes, it is because the shared library (which defines step) has the arguments reversed. I decided to keep the order as-is in backtrack.
| private import codeql.ruby.DataFlow | ||
| private import codeql.ruby.controlflow.CfgNodes | ||
| private import codeql.ruby.typetracking.TypeTracker | ||
| private import codeql.ruby.typetracking.internal.TypeTrackingImpl |
There was a problem hiding this comment.
Does this really need to import the Impl library? I think ideally the Impl module is only used when instantiating the library. The modules that are built on top should ideally only use the "public facing" bits.
There was a problem hiding this comment.
It does, because the code refers to StepSummary, which is considered internal in the new library.
There was a problem hiding this comment.
Ok, I guess we cannot avoid that, can we? Or should StepSummary perhaps be part of the public interface?
There was a problem hiding this comment.
I prefer to keep it internal, at least for now. It is not something I expect we need to use very often.
| * A description of a step on an inter-procedural data flow path. | ||
| */ | ||
| cached | ||
| newtype TStepSummary = |
There was a problem hiding this comment.
I see that most/all things in this module are deprecated. Shall we add a deprecation note to the file-level QL doc explaining what to use instead?
| | array_flow.rb:940:18:940:78 | # $ hasValueFlow=91.1 $ hasValueFlow=91.2 $ hasValueFlow=91.3 | Missing result:hasValueFlow=91.3 | | ||
| | array_flow.rb:957:28:957:46 | # $ hasValueFlow=93 | Missing result:hasValueFlow=93 | | ||
| | array_flow.rb:958:28:958:46 | # $ hasValueFlow=93 | Missing result:hasValueFlow=93 | | ||
| | array_flow.rb:1018:16:1018:36 | # $ hasValueFlow=99.2 | Missing result:hasValueFlow=99.2 | |
There was a problem hiding this comment.
I see that a lot of missing results are now gone. This is great, however, I'm curious why this is happening. I suppose the shared library is better than the old?
There was a problem hiding this comment.
This is because we now consider any target of a store (including with(out)-content checks) to be a local source node.
This PR switches out the existing type tracking library,
codeql.ruby.typetracking.TypeTracker.qll, with a new library based on the shared type tracking library, exposed ascodeql.ruby.typetracking.TypeTracking.qll. The old type tracking library has been deprecated.The consistency checks of the shared type tracking library revealed a lot of issues, which have been fixed on #14787, but also some here:
rescueclauses.