Skip to content

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

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

Merged
merged 3 commits into from
Oct 7, 2022

Conversation

hvitved
Copy link
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
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.

or
exists(Module other |
extendCallModule(m, other) and
method = lookupMethod(other, name)
)
}

pragma[nomagic]
private MethodBase lookupSingletonMethod(Module m, string name) {
Copy link
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
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