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

Improve handling of assignment expressions #4253

Merged
merged 1 commit into from
Mar 26, 2021
Merged

Improve handling of assignment expressions #4253

merged 1 commit into from
Mar 26, 2021

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Mar 25, 2021

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

Fixed some small issues with assignment expressions.

Multiline strings

l1 = f'The number {(count1 := 4)} ' \
     f'is equal to {count1}'

This caused a used-before-assignment error in Python 3.8. The root cause is in the builtin ast module. Until 3.9, all nodes where parsed as being on the same line. I added some special case handling for it.
However this might cause some errors to remain unnoticed!

Assignment expression with Expr

foo if (foo := 3 - 2) > 0 else 0

Assignment expression with AnnAssign and AugAssign

x2: bool = b2 if (b2 := True) else False
x3 = 0
x3 += b3 if (b3 := 4) else 6

--

I believe this could be included with 2.7.3

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #3763
Closes #4238

@coveralls
Copy link

coveralls commented Mar 25, 2021

Coverage Status

Coverage increased (+0.001%) to 91.437% when pulling 6d6a326 on cdce8p:fix-used-before-assignment into cb3ea62 on PyCQA:master.

@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Mar 26, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.7.3 milestone Mar 26, 2021
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Looks reasonable. What kind of errors could go unnoticed ? Tests are still green which is a good sign 😄

pylint/constants.py Show resolved Hide resolved
tests/functional/a/assignment_expression.py Show resolved Hide resolved
@cdce8p
Copy link
Member Author

cdce8p commented Mar 26, 2021

What kind of errors could go unnoticed ? Tests are still green which is a good sign 😄

Could you explain what you mean? Without this change the newly added tests would create used-before-assignment errors.

@Pierre-Sassoulas
Copy link
Member

Maybe I misunderstood your description I was referring to However this might cause some errors to remain unnoticed! ?

@cdce8p
Copy link
Member Author

cdce8p commented Mar 26, 2021

Yeah, take the following example:

l1 = f'The number {(count1 := 4)} ' \
     f'is equal to {count1}'

l2 = f'is equal to {count2}' \
     f'The number {(count2 := 4)} '

count2 is defined after it's used, so it won't work.

Without this change, both l1 and l2 would throw used-before-assignment errors.
With the fix, none will (in py38 and below). That's because both statements appear to be on the same line (with the old ast module). So there is no way to check if it's a valid use case or not.

@cdce8p
Copy link
Member Author

cdce8p commented Mar 26, 2021

Just FWI: This MR is would be ready for merge from my side.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 5d5f657 into pylint-dev:master Mar 26, 2021
@cdce8p cdce8p deleted the fix-used-before-assignment branch March 26, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
3 participants