Skip to content
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

gh-104683: Argument Clinic: Extract parse_function_names() helper #107964

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 15, 2023

Reduce duplicate code in state_modulename_name()

Reduce duplicate code in state_modulename_name()
@erlend-aasland
Copy link
Contributor Author

This refactor helped me discover a regression of gh-107885; Argument Clinic does not fail() for these cases:

/*[clinic input]
class C "void *" ""
C.meth
  a: int
[clinic start generated code]*/
/*[clinic input]
# This should not be allowed!
C.__new__ = C.meth
[clinic start generated code]*/
/*[clinic input]
# Neither should this
@classmethod
C.__init__ = C.meth
[clinic start generated code]*/

We should bake those assertions into the parse_names helper.

Tools/clinic/clinic.py Show resolved Hide resolved
Tools/clinic/clinic.py Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

Post this, I want to further extract helpers from this stage of the state machine; with this refactor, the remaining code is clearer (simply, since it is fewer lines of code), and it is now obvious we need to do something with the _module_and_class() calls :)

@AlexWaygood
Copy link
Member

it is now obvious we need to do something with the _module_and_class() calls :)

Off-topic, but I'm pretty sure this block of code is unreachable FYI:

cpython/Tools/clinic/clinic.py

Lines 2440 to 2441 in 13c36dc

if not hasattr(parent, 'classes'):
return module, cls

It's not currently covered by any of our tests. If our type annotations are correct, moreover (and I believe they are), parent should only ever be an instance of Clinic, Module or Class. Instances of Clinic, Module and Class all have a .classes attribute. (Mypy likely doesn't catch this because it doesn't have a great understanding of hasattr(), which is a bit too dynamic for it.)

@erlend-aasland
Copy link
Contributor Author

There are some corner cases which I think should be fixed:

I suggest we create an issue for this, and retarget this PR as a fix for these issues.

@AlexWaygood
Copy link
Member

The knot of code in #107964 (comment) is such a complex conditional (and, it appears, apparently buggy as well) that I think I'd prefer to see a dedicated PR untangling just that bit, and fixing the bug, before we go ahead with a change that factors out the common logic into a helper function. Would that be okay?

@erlend-aasland erlend-aasland marked this pull request as ready for review August 15, 2023 21:04
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
erlend-aasland and others added 3 commits August 16, 2023 13:19
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Oh, one optional other suggestion:

Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@erlend-aasland erlend-aasland enabled auto-merge (squash) August 16, 2023 13:01
@erlend-aasland
Copy link
Contributor Author

Thanks for the review!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

🎉

@AlexWaygood AlexWaygood changed the title gh-104683: Argument Clinic: Extract parse_names() helper gh-104683: Argument Clinic: Extract parse_function_names() helper Aug 16, 2023
@erlend-aasland erlend-aasland merged commit 42429d3 into python:main Aug 16, 2023
16 checks passed
@erlend-aasland erlend-aasland deleted the clinic/extract-parse-names-helper branch August 16, 2023 13:50
@erlend-aasland
Copy link
Contributor Author

I'll follow-up with further refactorings in this part of the source code; there's still a lot of duplicated code there.

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.

3 participants