FEAT: Func implementation must come after all @overload declarations

[iwakitakuma33]

#430

[facebook-github-bot]

Hi @iwakitakuma33!

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]

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]

@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]

@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]

@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]

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]

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
	}
	}

[iwakitakuma33]

@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.