Dataflow: Support side-effects for callbacks in summaries.#6767
Conversation
hvitved
left a comment
There was a problem hiding this comment.
Two questions. Otherwise LGTM.
| private predicate isContentOfArgument(SummaryComponentStack s) { | ||
| s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail()) | ||
| or | ||
| s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_)) |
There was a problem hiding this comment.
Should this rather be
s.head() = TContentSummaryComponent(_) and
s.tail() = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_))?
There was a problem hiding this comment.
No, I also meant to include, say, Element of Element of Argument[...].
There was a problem hiding this comment.
It's basically mirroring the way that the shared dataflow lib is converting A --read--> B to post(B) --store--> post(A).
There was a problem hiding this comment.
What I meant was whether the second disjunct above should be replaced with my suggesting. I.e. whether to disallow simply Argument[...].
There was a problem hiding this comment.
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 | |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK, let's leave it in then.
|
Happy to approve once the failing test has been fixed. |
SummarizedCallables (only when the input spec isisContentOfArgument).input -> Parameter[0] of Argument[0]andParameter[0] of Argument[0] -> outputimplies a third implicitly added summaryinput -> output.