Skip to content

Handling @overload with a docstring declaration #368

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

Closed
wants to merge 9 commits into from

Conversation

arnav-jain1
Copy link
Contributor

@arnav-jain1 arnav-jain1 commented May 27, 2025

fixes #359. Added a check to make sure the body was not a docstring. Also added a test in the the third_party folder to show that the change is working. Let me know if anything needs to be changed.

@yangdanny97
Copy link
Contributor

yangdanny97 commented May 27, 2025

Thanks!

The third_party/conformance tests are pulled from the typing spec conformance test suite, and shouldn't be modified directly here: https://github.com/python/typing/tree/main/conformance

For this type of feature, could you please add an integration test here? https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/test/overload.rs

Also, I think we should only allow docstrings for function bodies if there is an @overload decorator

For example, a program like this should still be invalid:

def foo() -> int:
    """hello"""

Copy link
Contributor

@yangdanny97 yangdanny97 left a comment

Choose a reason for hiding this comment

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

Back to you with comments, lmk if you have any questions. Thanks!

@arnav-jain1
Copy link
Contributor Author

Should be fixed. Added 3 test cases, first one is the basic one. Second one was to make sure that stuff after a docstring would still be typechecked properly and if the body of the function had more info, it wouldn't deem it a stub. The last one is for the case you mentioned of the nonoverloaded function with a docstring.

@arnav-jain1
Copy link
Contributor Author

My commits are a bit messy because I am a little new to this but the code should be right

@arnav-jain1 arnav-jain1 requested a review from yangdanny97 June 9, 2025 03:31
@arnav-jain1
Copy link
Contributor Author

Also if you have others that you want me to fix, let me know! I am a bit swamped right now but I can try and get to it soon.

@facebook-github-bot
Copy link
Contributor

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

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.

@yangdanny97
Copy link
Contributor

Sorry for the delay @arnav-jain1 and thanks for your contribution

We'll get this merged soon!

My only feedback would be that it's more readable to just use a loop and set the is_overload/has_no_type_checking inside the loop v.s. using a reduce. I've applied that change to the patch that we're merging, so there's no action required from you.

@facebook-github-bot
Copy link
Contributor

@yangdanny97 merged this pull request in e888be0.

@arnav-jain1
Copy link
Contributor Author

All good on the delay. Been a bit busy so I forgot. But yeah I'll remember that for the future. Im tryna learn haskell or functional programming in general and kind of used it as a fun exercise.

@yangdanny97
Copy link
Contributor

Im tryna learn haskell or functional programming in general and kind of used it as a fun exercise.

Functional programming is fun! Pyre being written in OCaml was one of the reasons I joined this team in the first place haha.

The simple reduce ops like sum/any/all are already their own functions, so it's pretty rare to actually use reduce directly I think. These days I only really use reduce if 1) it's chained after a map or filter 2) I'm reducing to a single value 3) the reduce operation is simple.

facebook-github-bot pushed a commit that referenced this pull request Jul 14, 2025
Summary:
closes #515. Did the same thing I did for #368. I might have missed something let me know if I did.

Pull Request resolved: #667

Reviewed By: stroxler

Differential Revision: D78274982

Pulled By: yangdanny97

fbshipit-source-id: 2c529da96b8d71328e9493c229910ab291768863
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.

Don't check return type for @overload and docstring
4 participants