-
Notifications
You must be signed in to change notification settings - Fork 269
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.
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
[dataflowengineoss] globalFromLit only when it's a standalone assignment #4559
Conversation
This patch looks coherent, I see the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this way of expressing the query, and the failing tests lose 1 flow but they are never 0, so I don't think this is an issue and rather a side effect of overtainting in the original query
@@ -16,14 +16,12 @@ class SingleAssignmentTests extends RubyCode2CpgFixture(withPostProcessing = tru | |||
val source = cpg.literal.l | |||
val sink = cpg.method.name("puts").callIn.argument.l | |||
val flows = sink.reachableByFlows(source).map(flowToResultPairs).distinct.sortBy(_.length).l | |||
flows.size shouldBe 6 | |||
val List(flow1, flow2, flow3, flow4, flow5, flow6) = flows | |||
val List(flow1, flow2, flow3, flow4, flow5) = flows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previously called flow3
is removed, but I reckon the new flow4
covers it better.
fd39e8f
to
183de17
Compare
Was actually expecting more tests to fail, as I don't have the required setup locally to test all frontends. Turns out there was only 7 of them and all on ruby. Took the opportunity to make those flow tests more explicit, instead of just looking at their sizes. Please kindly see if the changes committed after your approval still make sense, @DavidBakerEffendi |
183de17
to
56c4de8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ent (joernio#4559) * [dataflowengineoss] globalFromLit trigger only when it's a standalone assignment * fix unit-tests
In #4549, we noticed an extra flow in the following sample:
This was traced to
globalFromLiteral
, in which the statementx = foo(20)
would considerx
, adding it to the starting points of the query.In this patch we only trigger if the literal is the RHS of the assignment.
I may be missing some relevant context here that I didn't gather from the failing unit-tests. Before going through the trouble of fixing them all, is this patch coherent?
EDIT: Resolves #4549