Skip to content

Ruby: Take overrides into account for singleton methods defined on modules#10705

Merged
hvitved merged 3 commits intogithub:mainfrom
hvitved:ruby/singleton-overrides
Oct 7, 2022
Merged

Ruby: Take overrides into account for singleton methods defined on modules#10705
hvitved merged 3 commits intogithub:mainfrom
hvitved:ruby/singleton-overrides

Conversation

@hvitved
Copy link
Copy Markdown
Contributor

@hvitved hvitved commented Oct 6, 2022

We were previously not handling cases like this:

class A
    def self.singleton; end
end

class B < A
end
B.singleton # should resolve to B#singleton

class C < A
    def self.singleton; end
end
C.singleton # should resolve to C#singleton

DCA shows 20k new call edges, and those I sampled looked legit.

@github-actions github-actions Bot added the Ruby label Oct 6, 2022
@hvitved hvitved force-pushed the ruby/singleton-overrides branch from 0cd40eb to b3b9550 Compare October 6, 2022 08:38
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Oct 6, 2022
@hvitved hvitved marked this pull request as ready for review October 6, 2022 09:52
@hvitved hvitved requested a review from a team as a code owner October 6, 2022 09:52
@hvitved hvitved force-pushed the ruby/singleton-overrides branch from b3b9550 to 48bdf13 Compare October 6, 2022 09:56
Copy link
Copy Markdown
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.

LGTM with a minor comment

Related to our previous discussion, I noticed some of the new call edges are calls to user-defined new methods.

}

pragma[nomagic]
private MethodBase lookupSingletonMethod(Module m, string name) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add some QLDoc indicating that this takes inheritance into account, and likewise on singletonMethodOnModule.

I'd probably have named these predicates in a way that clarifies their relationship, such as:

  • lookupOwnSingletonMethod
  • lookupSingletonMethod

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I simply used the naming after the existing lookupMethod predicate for instance methods. I'll add ql doc.

@hvitved hvitved merged commit b065d2d into github:main Oct 7, 2022
@hvitved hvitved deleted the ruby/singleton-overrides branch October 7, 2022 11:34
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