Skip to content

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Sep 7, 2022

This PR rewrites the call graph computation in the following ways:

  1. Remove temporary fix for require and require_relative methods.
  2. No longer resolve instance methods based on flow through self (closed-world assumption); instead rely on the type hierarchy, and allow dispatch to any overrides (open-world assumption), as long as the type is not known exactly (such as when constructed with new).
  3. Improve precision for singleton methods defined on objects, such as def x.foo; end, by taking overriding redefinitions into account:
def x.foo; end
def x.foo; end
x.foo # now only resolves to the latter definition
  1. Add missing call targets for singleton methods defined on parameters (including self):
def add_singleton x
  def x.singleton; end
end
add_singleton y
y.singleton # now has a call target
  1. Add missing call targets for instance methods on objects referenced by variables introduced via pattern matching:
case object
  in C => c then c.foo # resolves in the same was as a call to `foo` inside `C`
end
  1. Take type information into account for objects that are being type checked:
case object
  when C then object.foo # resolves in the same was as a call to `foo` inside `C`
end

Commit-by-commit review is encouraged.

I spot checked some of the missing results on opal/opal, and they look like false positives to me. We were previously generating a lot of flow through this call (included as a sub-module), which now has only 1 call target instead of 42, due to the change 2 above.

@github-actions github-actions bot added the Ruby label Sep 7, 2022
@hvitved hvitved force-pushed the ruby/call-graph-rework branch 2 times, most recently from be2e603 to cd96f9b Compare September 8, 2022 09:37
@hvitved hvitved changed the title Ruby/call graph rework Ruby: Rework call graph implementation Sep 8, 2022
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Sep 8, 2022
@hvitved hvitved marked this pull request as ready for review September 8, 2022 13:18
@hvitved hvitved requested a review from a team as a code owner September 8, 2022 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.

Could you do a DCA run with meta-queries turned on, so we can see the change in call graph edges?

@hvitved hvitved marked this pull request as draft September 13, 2022 07:06
@hvitved hvitved force-pushed the ruby/call-graph-rework branch 10 times, most recently from b66d1e8 to 50e13af Compare September 15, 2022 19:11
@hvitved hvitved force-pushed the ruby/call-graph-rework branch from 50e13af to ac4d4ff Compare September 16, 2022 08:26
@hvitved hvitved marked this pull request as ready for review September 16, 2022 08:27
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.

Overall I think it's worth devoting a bit more time to the investigation of the call graph changes from the affected projects. Obviously the changes are too large to review in full. but normally I'd pick a few samples from each project and triage those. It's not really clear to me how much triaging has been done here.

@hvitved
Copy link
Contributor Author

hvitved commented Sep 19, 2022

@asgerf: Thanks for the review. I have now spot checked two new call graph edges for each project, and here is what I found:

Project Call Target Conclusion
activeadmin__activeadmin call target Looks actually like a TP, but it is resolved for incorrect reasons. I have pushed a fix.
activeadmin__activeadmin call target Open-world
aws__aws-sdk-ruby call target Open-world
aws__aws-sdk-ruby call target Singleton class defined on self
backup__backup call target Singleton class defined on self
backup__backup call target Singleton class defined on self
diaspora__diaspora call target Open-world
diaspora__diaspora call target Open-world
discourse__discourse call target Singleton method on module
discourse__discourse call target Singleton method on module
errbit__errbit call target Open-world
errbit__errbit call target Singleton class defined on self
fastlane__fastlane call target Singleton class defined on self
fastlane__fastlane call target Singleton methods in same class
gitlabhq__gitlabhq call target Open-world
gitlabhq__gitlabhq call target Singleton class defined on self
googleapis__google-cloud-ruby call target Open-world
googleapis__google-cloud-ruby call target Open-world (mock override)
hahwul__mad-metasploit call target Open-world (overridden singleton method). I have added a test cases for this, and also made sure that even more cases are covered.
hahwul__mad-metasploit call target Open-world (overridden singleton method)
heartcombo__devise call target Open-world (override in test code)
heartcombo__devise call target Open-world (override in test code)
jeremyevans__sequel call target Open-world (yield call resolves, because the base call now resolves)
jeremyevans__sequel call target Open-world. Happens because the DataBaseMethods is included into the Sequel::IBMDB::Database module, which extends Sequel::Database

The general pattern is that we now find more due to the open-world assumption, which is not surprising, and it looks like tests code/mocks are a natural source of potential false positives, but I know we have the same issue in C#.

Triaging the results did reveal a couple of bugs around singleton methods, which I have now fixed.

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.

Thanks! This looks great 👍

@hvitved hvitved merged commit 6473977 into github:main Sep 20, 2022
@hvitved hvitved deleted the ruby/call-graph-rework branch September 20, 2022 13:29
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