bpo-28936: Detect lexically first syntax error first#4097
Conversation
…ld be detected first
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
There are doctests for misusing of the nonlocal and global statement at lines 400-438. It seems to me that the case "name '%U' is parameter and global" is not covered here. It is worth to add a doctest for it and for other cases if there are other missed cases.
| @@ -0,0 +1,2 @@ | |||
| Ensure that lexically first syntax error involving ``global`` or | |||
| ``nonlocal`` is detected first at a given scope. | |||
There was a problem hiding this comment.
Add "Patch by Ivan Levkivskyi."
| VISIT_QUIT(st, 0); | ||
| if (cur & (DEF_LOCAL | USE | DEF_ANNOT)) { | ||
| if (cur & (DEF_PARAM | DEF_LOCAL | USE | DEF_ANNOT)) { | ||
| char* msg; |
There was a problem hiding this comment.
While we are here, maybe add the "const" qualifier?
| @@ -467,8 +473,7 @@ analyze_name(PySTEntryObject *ste, PyObject *scopes, PyObject *name, long flags, | |||
| if (flags & DEF_GLOBAL) { | |||
| if (flags & DEF_PARAM) { | |||
There was a problem hiding this comment.
Is this check still needed?
There was a problem hiding this comment.
Hm, indeed this looks redundant. Initially I thought that it is used to prohibit:
def f(x):
def g():
global xBut this is not an error, and after thinking more it looks like it should not be an error, since global and nonlocal have unwanted interference with being parameter only in the same scope.
I therefore remove this check.
|
@serhiy-storchaka Thanks for review! I also added the doctests you mentioned. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you Ivan. Now I completely understand what this patch does, and it LGTM.
Added few minor comments.
|
|
||
| def test_global_err_then_warn(self): | ||
| # Bug #763201: The SyntaxError raised for one global statement | ||
| # shouldn't be clobbered by a SyntaxWarning issued for a later one. |
There was a problem hiding this comment.
I think it is better to name this test test_global_param_err_first since it is related to using global with a parameter.
| global a # SyntaxError | ||
| def warning(): | ||
| def error2(): | ||
| b = 1 |
There was a problem hiding this comment.
Maybe add # SyntaxError?
| @@ -0,0 +1,2 @@ | |||
| Ensure that lexically first syntax error involving ``global`` or | |||
| ``nonlocal`` is detected first at a given scope. Patch by Ivan Levkivskyi. | |||
There was a problem hiding this comment.
Mention that this is specific to parameters? Other errors already are detected first.
|
@serhiy-storchaka Thanks! I fixed the remaining things. |
4 participants
Attn: @serhiy-storchaka
https://bugs.python.org/issue28936