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

Fixing davidhalter/parso#89 #90

Merged
merged 4 commits into from
Dec 14, 2019
Merged

Fixing davidhalter/parso#89 #90

merged 4 commits into from
Dec 14, 2019

Conversation

JarryShaw
Copy link
Contributor

@JarryShaw JarryShaw commented Dec 1, 2019

all changes are in parso/python/errors.py

  • add utility function (_get_namedexpr) to extract all assignment expression (namedexpr_test) nodes
  • add is_namedexpr parameter to _CheckAssignmentRule._check_assignment and special error message for assignment expression related assignment issues (cannot use named assignment with xxx)
  • add assignment expression check to _CompForRule (assignment expression cannot be used in a comprehension iterable expression)
  • add _NamedExprRule for special assignment expression checks
    • cannot use named assignment with lambda
    • cannot use named assignment with subscript
    • cannot use named assignment with attribute
    • and fallback general checks in _CheckAssignmentRule._check_assignment
  • add _ComprehensionRule for special checks on assignment expression in a comprehension
    • assignment expression within a comprehension cannot be used in a class body
    • assignment expression cannot rebind comprehension iteration variable 'xxx'

Sample testing code:

import parso

grammar = parso.load_grammar(version='3.8')

string = '''\
# Case 2
(lambda: x := 1)
# Case 3
(a[i] := x)
# Case 4
(a.b := c)
# Case 5
[i := 0 for i, j in range(5)]
[[(i := i) for j in range(5)] for i in range(5)]
[i for i, j in range(5) if True or (i := 1)]
[False and (i := 0) for i, j in range(5)]
# Case 6
[i+1 for i in (i := range(5))]
[i+1 for i in (j := range(5))]
[i+1 for i in (lambda: (j := range(5)))()]
# Case 7
class Example:
    [(j := i) for i in range(5)]
'''
module = grammar.parse(string, error_recovery=True)

errors = grammar.iter_errors(module)
print('\n'.join('[L%dC%d] %s' % (*error.start_pos, error.message) for error in errors))

Sample output:

[L2C1] SyntaxError: cannot use named assignment with lambda
[L4C1] SyntaxError: cannot use named assignment with subscript
[L6C1] SyntaxError: cannot use named assignment with attribute
[L8C1] SyntaxError: assignment expression cannot rebind comprehension iteration variable 'i'
[L9C3] SyntaxError: assignment expression cannot rebind comprehension iteration variable 'i'
[L10C36] SyntaxError: assignment expression cannot rebind comprehension iteration variable 'i'
[L11C12] SyntaxError: assignment expression cannot rebind comprehension iteration variable 'i'
[L13C15] SyntaxError: assignment expression cannot be used in a comprehension iterable expression
[L14C15] SyntaxError: assignment expression cannot be used in a comprehension iterable expression
[L15C24] SyntaxError: assignment expression cannot be used in a comprehension iterable expression
[L18C6] SyntaxError: assignment expression within a comprehension cannot be used in a class body

[all changes are in parso/python/errors.py]

* utility function (`_get_namedexpr`) extracting all assignment expression (`namedexpr_test`) nodes
* add `is_namedexpr` parameter to `_CheckAssignmentRule._check_assignment` and special error message for assignment expression related assignment issues (*cannot use named assignment with xxx*)
* add assignment expression check to `_CompForRule` (*assignment expression cannot be used in a comprehension iterable expression*)
* add `_NamedExprRule` for special assignment expression checks
  - *cannot use named assignment with lambda*
  - *cannot use named assignment with subscript*
  - *cannot use named assignment with attribute*
  - and fallback general checks in `_CheckAssignmentRule._check_assignment`
* add `_ComprehensionRule` for special checks on assignment expression in a comprehension
  - *assignment expression within a comprehension cannot be used in a class body*
  - *assignment expression cannot rebind comprehension iteration variable 'xxx'*
@JarryShaw
Copy link
Contributor Author

Wait... just realise there're some bugs when dealing with comprehensions... 🤦‍♂

e.g. `[i for i, j in range(5) for k in range (10) if True or (i := 1)]`
Copy link
Owner

@davidhalter davidhalter left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work. It looks really good.

Is there a way you can add a few tests? Typically we do this by adding tests to test/failing_examples.py. It would then probably also be good to add a few examples of named expressions that have no errors in them. Feel free to e.g. modify test.test_python_errors.test_valid_fstrings to take name expressions as well.

parso/python/errors.py Outdated Show resolved Hide resolved
 * search ancestors of namedexpr_test directly for comprehensions
 * added test samples for invalid namedexpr_test syntax
@JarryShaw
Copy link
Contributor Author

I have revised the implementation and added some failing & valid examples.

davidhalter added a commit that referenced this pull request Dec 14, 2019
- search_ancestor is now used instead of using node = node.parent
- Some lines were too long
@davidhalter davidhalter merged commit 89c4d95 into davidhalter:master Dec 14, 2019
@davidhalter
Copy link
Owner

davidhalter commented Dec 14, 2019

I refactored a few small things and then I identified some small issues.

I first wanted to let you do the changes, but quickly realized that it wasn't easy to even tell you what was wrong. At first I was just trying to change your implementation, then realized you did most things right and then realized that you didn't catch a few edge cases that I now implemented. There's probably still a few cases we haven't caught, but I think this is now at least much better than before (and some cases might be so exotic and stupid that nobody will ever actually catch them).

Thanks again for your work!

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.

2 participants