Skip to content

Java: Cache interpretElement. #15570

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.

Sign up for GitHub

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 1 commit into from
Feb 27, 2024
Merged

Java: Cache interpretElement. #15570

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ private Element interpretElement0(
}

/** Gets the source/sink/summary/neutral element corresponding to the supplied parameters. */
cached
Element interpretElement(
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that this is actually not the right thing to cache. I checked where it is used, and I think it ultimately boils down to the TDataFlowCallable newtype not being cached.

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably also need to cache viableCallableExt in DataFlowImplCommon.qll.

Copy link
Contributor

Choose a reason for hiding this comment

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

The latter is done here: #15582

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not so sure. There's a rather large dependency fan-out from interpretElement. And by not caching it, I can see that it would end up in at least two different DIL stages. It's used in at least ExternalFlow::sourceNode and ExternalFlow::sinkNode, which form their own stage, and in FlowSummaryImpl::summaryElement, which feeds into SummarizedCallableAdapter and its member predicates, which in turn feeds into the entire shared FlowSummaryImpl node synthesis. And the output of that is being used in at least two different cached stages as well (local steps are currently cached separately from the DataFlowImplCommon bulk caching).

string package, string type, boolean subtypes, string name, string signature, string ext
) {
Expand Down