FEAT: Func implementation must come after all @overload declarations by iwakitakuma33 · Pull Request #443 · facebook/pyrefly

iwakitakuma33 · 2025-06-07T13:11:36Z

facebook-github-bot · 2025-06-07T13:11:42Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

kinto0 · 2025-06-09T14:34:12Z

Thank you!!! I'll let @rchen152 review this. Just FYI #430 included both this issue and another (likely more difficult to implement) one

@overload
def setBar(self, value: int, inline: Literal[False]) -> Self: ... # ERROR: Overloaded function must have an implementation

@overload
def setBar(self, value: int, inline: Literal[True]) -> None: ...

# the real problem here is this @overload decorator should not be here
@overload
def setBar(self, value, inline):
    if inline:
        return self
    else:
        return None

iwakitakuma33 · 2025-06-09T14:38:55Z

@kinto0
Thank you for your reply!
Yes, I will look into issues that seem more difficult problem.

This is my first time contributing to this repository, so I started small.

yangdanny97 · 2025-06-09T15:34:57Z

@iwakitakuma33

Thanks! Could you please add an integration test case in this file?

https://github.com/facebook/pyrefly/blob/fab3bdec8aa597a5e9c10436921d28f617e54f48/pyrefly/lib/test/overload.rs

iwakitakuma33 · 2025-06-09T19:01:56Z

@yangdanny97
I added test.

I did fix to show error if concrete function has overload.
If the overload function has ... Is it OK .
If the overload function has pass , is it ok that show error??

rchen152

Thank you for the PR! It would be great if the new error messages could be reported instead of the confusing "Overloaded function must have an implementation" messages, rather than both messages being shown. I left in-line comments with suggestions for how to do that.

rchen152 · 2025-06-09T21:37:06Z

pyrefly/lib/alt/function.rs

+                def.name.range,
+                ErrorKind::InvalidOverload,
+                None,
+                "@overload decorator should not be used on function implementations.".to_owned(),


Similar to my other comment - this is a good error message, but we should report either this error or "Overloaded function must have an implementation," not both.

If you add a new stub_or_impl field to DecoratedFunction (

pyrefly/pyrefly/lib/alt/types/decorated_function.rs

Line 29 in 4a08127

pub struct DecoratedFunction {

), then you can move this check to solve_function_binding and do something like:

if !skip_implementation { if def.stub_or_impl == FunctionStubOrImpl::Impl { // report "@overload decorator should not be used on function implementations." else { // report "Overloaded function must have an implementation" } }

rchen152 · 2025-06-09T21:49:30Z

pyrefly/lib/alt/function.rs

+                    implementation_range,
+                    ErrorKind::InvalidOverload,
+                    None,
+                    "Function implementation must come after all @overload declarations. ".to_owned(),


I like this error message, but I think it's confusing that this error and "Overloaded function must have an implementation" are both reported; we should report one or the other.

What I'd suggest doing is: when we encounter an @overload-decorated signature with no successors, we loop through the predecessors to see if there's an implementation among them. If there is, then we report this error rather than the "Overloaded function must have an implementation" error. Here's an example of how to loop through predecessors:

pyrefly/pyrefly/lib/alt/function.rs

Lines 485 to 502 in 4a08127

fn step_overload_pred(&self, pred: &mut Option<Idx<Key>>) -> Option<Arc<DecoratedFunction>> {

let pred_idx = (*pred)?;

let mut b = self.bindings().get(pred_idx);

while let Binding::Forward(k) = b {

b = self.bindings().get(*k);

}

if let Binding::Function(idx, pred_, _) = b {

let def = self.get_idx(*idx);

if def.metadata.flags.is_overload {

*pred = *pred_;

Some(def)

} else {

None

}

} else {

None

}

}

@rchen152
Thank you for your feedback. I have made revisions based on your comments.
I have also added tests.

When I revised the code to prevent the message “Overloaded function needs at least two signatures” from appearing unnecessarily, evaluating get_idx in the successor direction caused lazy evaluation to occur, causing tests other than overload to fail. Therefore, in this PR,I have limited the scope to cases where successor.is_none is true.

Sorry for the delay! I somehow missed a bunch of GitHub notifications this past week. I'll review this tomorrow.

facebook-github-bot · 2025-06-27T19:27:30Z

@rchen152 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Together with #443, fixes #430. Reviewed By: grievejia, ndmitchell Differential Revision: D77470882 fbshipit-source-id: 28c94150d7f44611fa35b1729acd14e74edcba3c

facebook-github-bot · 2025-06-30T04:57:43Z

@rchen152 merged this pull request in 663ff64.

rchen152 · 2025-06-30T04:59:40Z

Thanks @iwakitakuma33!

facebook-github-bot added the cla signed label Jun 7, 2025

kinto0 requested a review from rchen152 June 9, 2025 14:25

iwakitakuma33 force-pushed the iwakitakuma33/imp_before_overload branch 2 times, most recently from a61647f to 5c8b0d5 Compare June 9, 2025 19:33

rchen152 requested changes Jun 9, 2025

View reviewed changes

Takuma Iwaki added 4 commits June 21, 2025 06:04

FEAT: Func implementation must come after all @overload declarations

5c60101

CHORE: CLA check

f3a7cf2

FEAT: imp with overload

45c9059

REVIEW

a697a25

iwakitakuma33 force-pushed the iwakitakuma33/imp_before_overload branch from 23f2f45 to a697a25 Compare June 20, 2025 21:05

iwakitakuma33 requested a review from rchen152 June 20, 2025 21:08

facebook-github-bot closed this in 663ff64 Jun 30, 2025

facebook-github-bot added the Merged label Jun 30, 2025

	fn step_overload_pred(&self, pred: &mut Option<Idx<Key>>) -> Option<Arc<DecoratedFunction>> {
	let pred_idx = (*pred)?;
	let mut b = self.bindings().get(pred_idx);
	while let Binding::Forward(k) = b {
	b = self.bindings().get(*k);
	}
	if let Binding::Function(idx, pred_, _) = b {
	let def = self.get_idx(*idx);
	if def.metadata.flags.is_overload {
	pred = pred_;
	Some(def)
	} else {
	None
	}
	} else {
	None
	}
	}

Comments

Conversation

iwakitakuma33 commented Jun 7, 2025 • edited by yangdanny97 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 7, 2025

Action Required

Process

Uh oh!

kinto0 commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iwakitakuma33 commented Jun 9, 2025

Uh oh!

yangdanny97 commented Jun 9, 2025

Uh oh!

iwakitakuma33 commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchen152 left a comment

Choose a reason for hiding this comment

Uh oh!

rchen152 Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

rchen152 Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

iwakitakuma33 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

rchen152 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 27, 2025

Uh oh!

facebook-github-bot commented Jun 30, 2025

Uh oh!

rchen152 commented Jun 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iwakitakuma33 commented Jun 7, 2025 •

edited by yangdanny97

Loading

kinto0 commented Jun 9, 2025 •

edited

Loading

iwakitakuma33 commented Jun 9, 2025 •

edited

Loading