Skip to content

Conversation

key262yek
Copy link
Contributor

No description provided.

Copy link
Contributor

@fanninpm fanninpm left a comment

Choose a reason for hiding this comment

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

Welcome to RustPython, and thanks for volunteering your time and effort. There is an issue with the CPython test suite that you may or may not have noticed by now.

Comment on lines +799 to +801
"end_lineno" => ctx.none(),
"offset" => ctx.none(),
"end_offset" => ctx.none(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that the positioning of these is currently incompatible with some of the exceptions that are derived from SyntaxError. On your local machine, can you run the following command and figure out why the test suite is hanging?

cargo run --release --features ssl,jit -- -m test -u all --slowest --fail-env-changed -v

Copy link
Contributor Author

@key262yek key262yek Jul 11, 2022

Choose a reason for hiding this comment

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

Thank you for your help in solving the problem.
In fact, it was not confirmed whether the test file was runable because it was a pull request only until the commit where the test file was just copied.
I added the expectedFailure tag and skip tag to make test runable, and re-commit

The biggest problem is that the test_no_hang_on_context_chain_cycle1,2,3 test (which are all new) in Lib/test/test_exception.py is stuck in an infinite loop, so the test suite have not ended.
(Still don't know why they are stuck, so I added skip tags)

I'm reviewing the entire test to check if there are any other problems, so I'll leave a comment again when the results come out.

I didn't know that the order of attributes could be a problem. Could you explain a little bit more about the mechanism that could be a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know that the order of attributes could be a problem. Could you explain a little bit more about the mechanism that could be a problem?

My mistake. I was caught off guard by the Unicode-related errors, as they have similarly-named attributes.


I'd recommend taking a look at the definition of fn new_syntax_error() (in vm/src/vm/vm_new.rs) as well as where that method is invoked. (You may need to reach into RustPython's compiler crate to implement those new attributes properly.)

Copy link
Contributor Author

@key262yek key262yek Jul 11, 2022

Choose a reason for hiding this comment

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

First, I checked other test on current branch and compare it to the results of current main branch and of the case when I switched order of attributes.
There are no difference of results. Hence, until now, the order of attributes seems not a big problem.

And I have concerned because there are a lot of things to change for the SyntaxError update. But thanks to you, now I know where to start. Thank you for the recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

the easiest way to diagnosing hanging problem is checking callstack when it hanged. I usually use debugger but there will be other ways too.
tough you may not need it right now

@youknowone youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 13, 2022
@key262yek key262yek closed this Jul 13, 2022
@key262yek key262yek reopened this Jul 13, 2022
@key262yek key262yek force-pushed the update-syntaxerror branch from c4631a6 to bf2e2db Compare July 13, 2022 20:44
add 'end_lineno' and 'end_offset'
@key262yek key262yek force-pushed the update-syntaxerror branch from bf2e2db to 4c94439 Compare July 15, 2022 05:07
@youknowone youknowone marked this pull request as ready for review July 15, 2022 17:21
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.

Though the actual functions are not implemented yet, this patch is enough to resolve blocking issue for test_traceback.py

@youknowone youknowone merged commit 5983a5a into RustPython:main Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
z-ca-2022 Tag to track contrubution-academy 2022
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants