Skip to content
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

Dataflow: Support side-effects for callbacks in summaries. #6767

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

@aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Sep 28, 2021

  • Adds support for referring to the parameter of a callback as an input spec.
  • Adds post-update nodes for callback arguments to support callback side-effects being reflected back as side-effects on the arguments to SummarizedCallables (only when the input spec is isContentOfArgument).
  • Adds support for chaining summaries on callback parameters. E.g. the two summaries input -> Parameter[0] of Argument[0] and Parameter[0] of Argument[0] -> output implies a third implicitly added summary input -> output.
  • Adds implicit summaries for flow to the instance parameter of callbacks (optional feature).
  • Minor refactor to make the reference to a callback into an input state rather than an output state.
Copy link
Contributor

@hvitved hvitved left a comment

Two questions. Otherwise LGTM.

private predicate isContentOfArgument(SummaryComponentStack s) {
s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail())
or
s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_))
Copy link
Contributor

@hvitved hvitved Sep 30, 2021

Should this rather be

s.head() = TContentSummaryComponent(_) and
s.tail() = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_))

?

Copy link
Contributor Author

@aschackmull aschackmull Oct 1, 2021

No, I also meant to include, say, Element of Element of Argument[...].

Copy link
Contributor Author

@aschackmull aschackmull Oct 1, 2021

It's basically mirroring the way that the shared dataflow lib is converting A --read--> B to post(B) --store--> post(A).

Copy link
Contributor

@hvitved hvitved Oct 1, 2021

What I meant was whether the second disjunct above should be replaced with my suggesting. I.e. whether to disallow simply Argument[...].

Copy link
Contributor Author

@aschackmull aschackmull Oct 1, 2021

Ah, right. No, I most definitely want to include just Argument[...]. That's likely the most important case. I guess the predicate name could be better - I'm open to suggestions.

isContentOfArgument(output)
or
// flow from the receiver of a callback into the instance-parameter
exists(SummaryComponentStack s, SummaryComponentStack callbackRef |
Copy link
Contributor

@hvitved hvitved Sep 30, 2021

I am not convinced that this should go into the shared library; perhaps initially we should just have it in the Java library (and get rid of the thisParam predicate)?

Copy link
Contributor Author

@aschackmull aschackmull Oct 1, 2021

I considered this to be a general OO thing. Consider a call x.apply(y), then the x plays two roles: it is used to determine the runtime dispatch target and it is being passed into the call as the instance argument. And since most languages have the concept of this being a parameter of member functions, then this feature felt pretty universal.
This could even be relevant for languages with lambdas that have no explicit way to reference themselves through a this parameter, since in most such cases lambdas support closures, which means that captured variables are actually stored in a closure object at the lambda construction point, which will likely need to be modelled as implicit instance fields on the implicit this of the lambda in order to get accurate flow.

Copy link
Contributor Author

@aschackmull aschackmull Oct 1, 2021

So actually not just an OO thing. This is relevant for purely functional programming as well, since it's needed for proper support of closures.

Copy link
Contributor

@hvitved hvitved Oct 1, 2021

OK, let's leave it in then.

@hvitved
Copy link
Contributor

@hvitved hvitved commented Oct 1, 2021

Happy to approve once the failing test has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants