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

3.13 support #80

Merged
merged 21 commits into from
Aug 26, 2024
Merged

3.13 support #80

merged 21 commits into from
Aug 26, 2024

Conversation

15r10nk
Copy link
Collaborator

@15r10nk 15r10nk commented Jan 27, 2024

First of all It was kind of an accident that I pushed directly into your repository, is it OK?

This is the start of the 3.13 support.

fixes #79

The existing tests work, but there are some open issues:

  • ("STORE_FAST_STORE_FAST", "STORE_FAST_LOAD_FAST", "LOAD_FAST_LOAD_FAST") are used to load/store two variables, but they map only to one position of one ast.Name. It is not possible for one bytecode to map to multiple locations. I think there is no other option for us than to accept the fact that we are not able to map this instructions to ast nodes.
  • I have only checked the existing tests so far and not searched for new samples.

@alexmojaki
Copy link
Owner

First of all It was kind of an accident that I pushed directly into your repository, is it OK?

Yes, using a branch instead of a fork is better.

@alexmojaki
Copy link
Owner

("STORE_FAST_STORE_FAST", "STORE_FAST_LOAD_FAST", "LOAD_FAST_LOAD_FAST") are used to load/store two variables, but they map only to one position of one ast.Name. It is not possible for one bytecode to map to multiple locations. I think there is no other option for us than to accept the fact that we are not able to map this instructions to ast nodes.

This is fine. Like you say, I don't even know what we'd do. But also, identifying ast.Name has barely any value. And I think it should be virtually impossible to actually get hold of a frame paused at that point. I can't see a way for one of those instructions to raise an exception - if they could, what would the Python traceback highlight?

@15r10nk
Copy link
Collaborator Author

15r10nk commented Jan 27, 2024

if they could, what would the Python traceback highlight

probably the first name.
I think I have a backtrace which could trigger something like this.

def a():
    def b():
        nonlocal e

        e += 1

    b()
    e = 5


a()

output (Python 3.13.0a3+):

Traceback (most recent call last):
  File "/home/frank/projects/executing/codi.py", line 11, in <module>
    a()
    ~^^
  File "/home/frank/projects/executing/codi.py", line 7, in a
    b()
    ~^^
  File "/home/frank/projects/executing/codi.py", line 5, in b
    e += 1
    ^
NameError: cannot access free variable 'e' where it is not associated with a value in enclosing scope

it is currently the same variable. so no issue here, but it is a backtrace which refers to an ast.Name.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Jan 27, 2024

I take it back, this is a LOAD_DEREF. The bytecode is only combined for LOAD_FAST.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Jan 28, 2024

found the first cpython 3.13 issue python/cpython#114671

@frenzymadness
Copy link
Contributor

What is the status, please? The second beta of 3.13 should be out today.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Jun 4, 2024

I fixed some tests for 3.13.0b1 and it is working for 3.13 now, but the ci is failing for 3.5.

@alexmojaki should we start to deprecate some python versions?

@alexmojaki
Copy link
Owner

Yes, we can drop <3.8

@frenzymadness
Copy link
Contributor

Thank you! With the patch from this PR, I can confirm that the RPM package in Fedora Linux with Python 3.13 builds fine.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Jun 5, 2024

I have still some problem with the tests (like python/cpython#120108) but I think that we are on a good way.

@frenzymadness
Copy link
Contributor

Python 3.13 beta 3 is out and beta 4 will be released soon.

@15r10nk 15r10nk marked this pull request as ready for review July 24, 2024 08:59
executing/_position_node_finder.py Show resolved Hide resolved
executing/_position_node_finder.py Outdated Show resolved Hide resolved
executing/_position_node_finder.py Outdated Show resolved Hide resolved
)
):
# closures
# TODO: better check that this is actualy a closure for a scope of a type variable
Copy link
Owner

Choose a reason for hiding this comment

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

Can this TODO be resolved? This condition looks very lenient

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The following code shows an example:
image

The code is not only generated for type variables (I fixed the comment).

Checking the variable name could be expensive to compute, because we would have to check large ast trees for variable names.

We could check for the LOAD_FAST,BUILD_TUPLE,LOAD_CONST,MAKE_FUNCTION,SET_FUNCTION_ATTRIBUTE pattern and allow the LOAD_FAST in these cases.

Copy link
Owner

Choose a reason for hiding this comment

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

Please just leave a more detailed comment, maybe a link to this discussion.

tests/test_main.py Outdated Show resolved Hide resolved
@alexmojaki
Copy link
Owner

Is this ready for review again?

@15r10nk
Copy link
Collaborator Author

15r10nk commented Aug 25, 2024

yes, you can review it.


if instruction.opname == "LOAD_FAST" and instruction.argval == "__class__":
raise KnownIssue(
f"loading of __class__ is accociated with a random node at the end of a class"
Copy link
Owner

Choose a reason for hiding this comment

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

Seems like this could also arise normally - what about checking if the node is a Name(id='__class__')?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The check would not work.

class T:
    def a():
        super()
    some_node

The node in this case is Name(id='some_node')

Copy link
Owner

Choose a reason for hiding this comment

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

is it a random node, or is it always class_node.body[-1]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not "random", but the last node cpython has worked on(this is my current assumption).

It is the assigned name in this case:

class T:
    def a():
        super()
    some_node=not_the_node()

I don't think that we can build any solid rule for these cases.

)
):
# closures
# TODO: better check that this is actualy a closure for a scope of a type variable
Copy link
Owner

Choose a reason for hiding this comment

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

Please just leave a more detailed comment, maybe a link to this discussion.

Copy link
Owner

@alexmojaki alexmojaki left a comment

Choose a reason for hiding this comment

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

Thank you!

@15r10nk
Copy link
Collaborator Author

15r10nk commented Aug 26, 2024

@alexmojaki, would prefer a normal merge-commit for this review.
The reason is that the different commits are useful to connect the small_samples with the code changes.

I would merge it if it is ok for you.

@alexmojaki
Copy link
Owner

OK, if you prefer that

@15r10nk 15r10nk merged commit ea6a695 into master Aug 26, 2024
8 checks passed
@15r10nk 15r10nk deleted the 3.13 branch August 31, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.13
3 participants