Skip to content

Conversation

@asukaminato0721
Copy link
Contributor

fix #1358

@meta-cla meta-cla bot added the cla signed label Oct 22, 2025
@asukaminato0721 asukaminato0721 marked this pull request as ready for review October 22, 2025 11:59
@asukaminato0721 asukaminato0721 marked this pull request as draft October 23, 2025 04:46
@asukaminato0721 asukaminato0721 marked this pull request as ready for review October 23, 2025 07:33
@meta-codesync
Copy link

meta-codesync bot commented Oct 23, 2025

@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D85364483.

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

Mostly this looks good to me, but I'm a little concerned that
(a) the preexisting use of the word "stub" is misleading, and
(b) we need to double-check the behavior on actual stubs (as in, .pyi files)

.map(|(idx, range)| (self.get_idx(*idx).arc_clone_ty(), *range)),
);

if stub_or_impl == FunctionStubOrImpl::Stub {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: the name FunctionStubOrImpl::Stub is preexisting and goes against my request, but we can probably live with it for now - I may try to rename it later.

But related: @yangdanny97 do you know what happens if this is actually a "stub" in the sense of coming from a .pyi file?

In particular - will an actual stub from a .pyi file always have FunctionStubOrImpl::Impl rather than FunctionStubOrImpl::Stub?

It would be wrong for a stub to ever use Stub (that sentence explains exactly why the name Stub is probably the wrong name haha!), since for stubs we have to assume they imply an implementation exists, typically in native code.

We might want to add a test for that if we're not sure - this could throw a lot of false positives on actual .pyi files if we get it wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

FunctionStubOrImpl::Stub

This is where it's set, I don't think it considers the type of module, only whether it has a proper body or not.

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

Review automatically exported from Phabricator review in Meta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error when calling abstract method on super()

3 participants