Data flow: Account for hidden subpath wrappers
#15734
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hvitved
force-pushed
the
dataflow/hidden-subpath
branch
2 times, most recently
from
126e550 to
ad21ea7
Compare
hvitved
force-pushed
the
dataflow/hidden-subpath
branch
from
ad21ea7 to
593ccf2
Compare
hvitved
force-pushed
the
dataflow/hidden-subpath
branch
from
593ccf2 to
543cbfc
Compare
![github-advanced-security[bot]](https://web.archive.org/web/20260116000528im_/https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| private module CaptureInput implements Shared::InputSig<Location> { | ||
| private import csharp as Cs |
Check warning
Code scanning / CodeQL
Names only differing by case
hvitved
force-pushed
the
dataflow/hidden-subpath
branch
from
543cbfc to
f533ae9
Compare
hvitved
force-pushed
the
dataflow/hidden-subpath
branch
from
f533ae9 to
2aa0cda
Compare
aschackmull
reviewed
Mar 18, 2024
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Path explanations now include flow that goes through callbacks passed into library functions. For example, if `map` is library function, then in `result = map(xs, x => x + 1)` we will now include the step from `x` to `x + 1` in the path explanation, instead og going directly from `xs` to `result`. Note that this change does not affect actual query results, but only how path explanations are computed. |
Contributor
There was a problem hiding this comment.
Suggested change
| * Path explanations now include flow that goes through callbacks passed into library functions. For example, if `map` is library function, then in `result = map(xs, x => x + 1)` we will now include the step from `x` to `x + 1` in the path explanation, instead og going directly from `xs` to `result`. Note that this change does not affect actual query results, but only how path explanations are computed. | |
| * Path explanations now include flow that goes through callbacks passed into library functions. For example, if `map` is a library function, then in `result = map(xs, x => x + 1)` we will now include the step from `x` to `x + 1` in the path explanation, instead of going directly from `xs` to `result`. Note that this change does not affect actual query results, but only how path explanations are computed. |
aschackmull
reviewed
Mar 18, 2024
Comment on lines
4243
to
4245
| private PathNodeImpl localStepFromHidden(PathNodeImpl n) { | ||
| n = localStep(result) and | ||
| result.isHidden() |
Contributor
There was a problem hiding this comment.
From the name I'd expect that n was hidden, not that the order was swapped.
Suggested change
| private PathNodeImpl localStepFromHidden(PathNodeImpl n) { | |
| n = localStep(result) and | |
| result.isHidden() | |
| private PathNodeImpl localStepFromHidden(PathNodeImpl n) { | |
| result = localStep(n) and | |
| n.isHidden() |
Contributor
There was a problem hiding this comment.
Or maybe just change the predicate name:
Suggested change
| private PathNodeImpl localStepFromHidden(PathNodeImpl n) { | |
| n = localStep(result) and | |
| result.isHidden() | |
| private PathNodeImpl revLocalStepToHidden(PathNodeImpl n) { | |
| n = localStep(result) and | |
| result.isHidden() |
Contributor
There was a problem hiding this comment.
Or maybe that should be revLocalStepFromHidden. I guess that depends on whether you read it as rev applying to all of the rest or just to localStep.
Contributor
There was a problem hiding this comment.
I think the code would be clearer if both of these relations were using named columns instead of the result column and were both in the forward direction, i.e. localStepFromHidden(n1, n2) and localStepToHidden(n1, n2)`. This would add a few lines and name a few more intermediate nodes in the usages below, but I think that's a readability win.
aschackmull
reviewed
Mar 18, 2024
Contributor
aschackmull
left a comment
aschackmull
left a comment
There was a problem hiding this comment.
I think readability can be improved a bit, but otherwise LGTM.
aschackmull
reviewed
Mar 18, 2024
hvitved
force-pushed
the
dataflow/hidden-subpath
branch
from
c8d3e64 to
7a3b8eb
Compare
Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we would not get
subpathscomputed for hidden wrappers, such as when flow passes through a callback passed into a library method with a flow summary:This PR changes that to allow for an arbitrary number of hidden intermediate wrappers, and also takes into account that argument/out/parameter nodes may be hidden (we previously only took into account that return nodes could be hidden).