Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Sep 23, 2023

Now that we track types in data flow, implementing localMustFlowStep means that we will also get type-based call-target pruning, as implemented in the shared library.

@hvitved hvitved changed the title Ruby: Implement mustFlow Ruby: Implement localMustFlowStep Jan 10, 2025
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Jan 10, 2025
@hvitved hvitved marked this pull request as ready for review January 10, 2025 08:12
@hvitved hvitved requested a review from a team as a code owner January 10, 2025 08:12
@hvitved hvitved requested a review from aibaars January 16, 2025 10:03
node2.asExpr() = SsaImpl::getARead(def)
)
or
node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs()
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me, I was just wondering if there was overlap between this line and def.(Ssa::WriteDefinition).assigns(node1.asExpr()) above. Both seem to be related to assignments. Similar question about SsaFlow::toParameterNodeImpl and the special handling for (ImplicitBlockArgumentNode).getParameterNode(_).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks good to me, I was just wondering if there was overlap between this line and def.(Ssa::WriteDefinition).assigns(node1.asExpr()) above. Both seem to be related to assignments.

The first line is about flow from e to SSA def for x in x = e, and the second line is about flow from e to the whole assignment expression x = e.

Similar question about SsaFlow::toParameterNodeImpl and the special handling for (ImplicitBlockArgumentNode).getParameterNode(_).

This is because implicit block parameters and arguments are not covered by our SSA analysis, otherwise you are right it would be an instance of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying.

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.

LGTM, just had a question.

@hvitved hvitved merged commit 253ccd1 into github:main Jan 27, 2025
23 checks passed
@hvitved hvitved deleted the ruby/must-flow branch January 27, 2025 11:51
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 Ruby

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants