Skip to content

Ruby: Reimplement flow through captured variables using field flow #11725

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

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Dec 16, 2022

This PR adopts the shared variable capture flow library in Ruby.

The new results reported by DCA are a result of the fact that we are now also able to track side-effects on captured variables, such as writes to instance fields inside blocks. The lost results (those that I have checked) are FPs resulting from the old jump-step based modeling, where data could flow in through one callable and out via another.

The implementation is similar to Java (see PR above), except I chose to distinguish "lambda self" from "normal self". This is because self can actually be referenced from within lambdas (as a captured variable):

class C
    def foo
        self.@x = taint # the `self.` is actually implicit in Ruby
    end

    def bar
        yield
    end

    def baz
        self.bar do
            sink self.@x # captured access to `self`
        end
    end
end

c = C.new
c.foo
c.baz

@github-actions github-actions bot added the Ruby label Dec 16, 2022
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Dec 16, 2022
@hvitved hvitved force-pushed the ruby/capture-field-flow branch from d4f1208 to 5e11ea7 Compare December 16, 2022 14:57
@hvitved hvitved force-pushed the ruby/capture-field-flow branch from b7d1941 to eaf1b5c Compare March 14, 2023 08:37
@hvitved hvitved force-pushed the ruby/capture-field-flow branch from eaf1b5c to abce224 Compare March 14, 2023 08:59
@hvitved hvitved force-pushed the ruby/capture-field-flow branch from abce224 to b04fd1c Compare March 14, 2023 10:45
@hvitved hvitved force-pushed the ruby/capture-field-flow branch 4 times, most recently from a3f3886 to 3480fb9 Compare August 23, 2023 08:07
@hvitved hvitved force-pushed the ruby/capture-field-flow branch 2 times, most recently from 7afc01d to d018b80 Compare August 24, 2023 09:56
@hvitved hvitved force-pushed the ruby/capture-field-flow branch from d018b80 to b68b332 Compare August 25, 2023 10:19
@hvitved hvitved marked this pull request as ready for review August 25, 2023 13:08
@hvitved hvitved requested a review from a team as a code owner August 25, 2023 13:08
@calumgrant calumgrant requested a review from asgerf August 29, 2023 13:18
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Haven't done a full review, just posting my comments so far

@asgerf
Copy link
Contributor

asgerf commented Aug 31, 2023

#14048 has been merged, which will conflict with this PR. Apologies for the disruption.

@hvitved
Copy link
Contributor Author

hvitved commented Aug 31, 2023

#14048 has been merged, which will conflict with this PR. Apologies for the disruption.

No worries; I'll update.

@hvitved hvitved force-pushed the ruby/capture-field-flow branch from fe52878 to d259c89 Compare August 31, 2023 09:11
@hvitved hvitved force-pushed the ruby/capture-field-flow branch from d259c89 to a06a9ff Compare September 6, 2023 09:02
@@ -331,6 +443,7 @@ private module Cached {
p instanceof SplatParameter
} or
TSelfParameterNode(MethodBase m) or
TLambdaSelfParameterNode(Callable c) { lambdaCreationExpr(_, _, c) } or
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rename this to TLambdaSelfReferenceNode. As it stands it sounds too much like it's related to the self parameter somehow, which it isn't.

Blocks can actually receive a self argument that overrides its captured view of self. When and if we add support for that, we'll need an actual self parameter for lambdas, and then this name will just get even more confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean for blocks passed to module_eval and similar methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aibaars Yes, module_eval, class_eval, instance_eval, or any method that that passes its block to one of those methods, such as RSpec.describe in a test or rescue_from in an action controller.

@hvitved hvitved merged commit bb85f87 into github:main Sep 13, 2023
@hvitved hvitved deleted the ruby/capture-field-flow branch September 13, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants