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

test_builtin.py test_compile unit test fix #5251

Merged
merged 7 commits into from
Apr 23, 2024

Conversation

hydrogen602
Copy link
Contributor

@hydrogen602 hydrogen602 commented Apr 21, 2024

Code changes to make the test BuiltinTest.test_compile in test_builtin.py work.

I've made a draft PR as this unit test has multiple different incompatibility issues that all need to be fixed for this to fully work, and so feedback can be given on the first issues while I work on the latter ones.

Issues:

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

If you are going to feel a PR is going too big, you don't need to finish it in a single PR. Keeping the test unchanged and submitting a partial fix is also a preferable way. Don't feel pressured by the unit of test. Thank you!

vm/src/stdlib/builtins.rs Outdated Show resolved Hide resolved
Lib/test/test_builtin.py Outdated Show resolved Hide resolved
@hydrogen602 hydrogen602 marked this pull request as ready for review April 23, 2024 01:09
@hydrogen602
Copy link
Contributor Author

If you are going to feel a PR is going too big, you don't need to finish it in a single PR. Keeping the test unchanged and submitting a partial fix is also a preferable way. Don't feel pressured by the unit of test. Thank you!

Ok I'll aim to get this merged as it is now, and then later look into the last issue with optimize=2 and docstrings.

Lib/test/test_builtin.py Show resolved Hide resolved
vm/src/stdlib/builtins.rs Show resolved Hide resolved
@youknowone youknowone force-pushed the test_builtin_test_compile_fix branch from 9adbe3b to b634a7b Compare April 23, 2024 03:00
@hydrogen602
Copy link
Contributor Author

hydrogen602 commented Apr 23, 2024

Looks like some more compile flags need to be added to the flag validator:

# The CO_xxx symbols are defined here under the same names defined in
# code.h and used by compile.h, so that an editor search will find them here.
# However, they're not exported in __all__, because they don't really belong to
# this module.
CO_NESTED = 0x0010                      # nested_scopes
CO_GENERATOR_ALLOWED = 0                # generators (obsolete, was 0x1000)
CO_FUTURE_DIVISION = 0x20000            # division
CO_FUTURE_ABSOLUTE_IMPORT = 0x40000     # perform absolute imports by default
CO_FUTURE_WITH_STATEMENT = 0x80000      # with statement
CO_FUTURE_PRINT_FUNCTION = 0x100000     # print function
CO_FUTURE_UNICODE_LITERALS = 0x200000   # unicode string literals
CO_FUTURE_BARRY_AS_BDFL = 0x400000
CO_FUTURE_GENERATOR_STOP = 0x800000     # StopIteration becomes RuntimeError in generators
CO_FUTURE_ANNOTATIONS = 0x1000000       # annotations become strings at runtime

I'll look into it

@hydrogen602
Copy link
Contributor Author

Looks like there is an error in macos - pylib/Lib/test/test_socket.py. I'm not sure why though - and the test file runs fine on my own mac. Do you have any insight?

@youknowone
Copy link
Member

That might be just flaky

@hydrogen602
Copy link
Contributor Author

One question I have is how __future__ works. It looks like from compiler/codegen/src/compile.rs compile_future_features() that __future__ settings like annotations are only collected from __future__ import statements, meaning the flags, especially CO_FUTURE_ANNOTATIONS, while allowed, are meaningless. Is there some plan to pass flags into compile.rs?

@youknowone
Copy link
Member

youknowone commented Apr 23, 2024

That's new to me. How about adding a function to collect __future__ from parsed ast and pass the flag value to compile? This way must be easy in current RustPython compile design.
On the other hand, RustPython is premature and we are on the moving target. Ignoring the future and always enabling CO_FUTURE_ANNOTATIONS will be another way to do it.

Copy link
Member

@youknowone youknowone 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 so much! The socket test failing looks irrelevant. Let me suppress it later.

@youknowone youknowone merged commit 3313fde into RustPython:main Apr 23, 2024
10 of 11 checks passed
@hydrogen602 hydrogen602 deleted the test_builtin_test_compile_fix branch April 23, 2024 05:13
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.

None yet

2 participants