-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
test_global_err_then_warn in test_syntax is no longer valid #73122
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
Comments
bpo-27999 invalidated test_global_err_then_warn in Lib/test/test_syntax.py:567. That test was added in bpo-763201 for testing that SyntaxWarning emitted in symtable_global() doesn't clobber a SyntaxError. In bpo-27999 a SyntaxWarning was converted to SyntaxError, and the test no longer serves its purpose. bpo-28512 makes tests more strong (it tests the position of SyntaxError), but it can't be applied in 3.6, because _check_error() catches different SyntaxError. |
Serhiy, here is a patch that might be helpful. It detects global-and-parameter errors sooner (when possible). This will cause the following: >>> if 1:
... def error(a):
... global a
... def error2():
... b = 1
... global b
...
File "<stdin>", line 3
SyntaxError: name 'a' is parameter and global However, in more complex (nested) cases, the global-after-assign will still be detected sooner: >>> def error(a):
... def inner():
... global a
... def inner2():
... b = 1
... global b
...
File "<stdin>", line 6
SyntaxError: name 'b' is assigned to before global declaration Maybe there is a way to delay the detection of this error until second pass in symtable.c, but don't see now how to do this. |
I don't know what should we do with this. Is there a bug that needs to be fixed? Or maybe the test can be just removed? |
I don't think this is a bug, but detecting first a SyntaxError that appears textually first might be seen as an improvement. I would say such behaviour seems more intuitive. A possible downside could be a (probably very minor) slow-down of compilation. I don't have any strong opinion here. |
Updated patch as proposed by Serhiy. |
Do you mind to create a pull request Ivan? |
OK, I made a PR with the fix and a test that checks the line number for syntax error (so that the original purpose test_global_err_then_warn is preserved). |
Am I needed here? |
As I understand, Serhiy is going to review the PR, so I think no. |
Sorry for disturbing you Guido. I had added you as the committer of bpo-27999. Do you have thoughts about this issue? |
I think this is very minor but if you two can agree that the code is right I think it's a nice little improvement, and I like that that particular test's usefulness is restored. PS. Long-term we should really build error recovery into our antiquated parser and report as many errors as we can, without cascading. But that's probably a Python 4 project. |
Is it correct that the parameter can be annotated in the function body? def f(x):
x: int Or that the local variable can be annotated after assignment? def f():
x = 1
x: int |
Those seem things that the type checker should complain about, but I don't think Python should. |
I agree with Guido, this is rather a task for type checkers. |
Thank you for your patch Ivan. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: