Opened 5 months ago

Closed 4 weeks ago

Last modified 3 weeks ago

#5844 closed change (fixed)

[flake8-eyeo] Detect (more) redundant parentheses

Reported by: sebastian Assignee: rhowell
Priority: P3 Milestone:
Module: Automation Keywords:
Cc: kvas, rhowell Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29602819

Description (last modified by sebastian)

Background

Currently, flake8-eyeo (our own plugin for the flake8 Python code checker) already detects parentheses used in statements (A111), which are redundant in Python, e.g:

if (x == y):
    pass

We currently go to quite some length, to consider all legit uses of parentheses in statements (e.g. newlines, sub-expressions and tuples), while we don't address any redundant usage of parentheses outside of if and while statements at all.

In a recent code review one particular error-prone case (which isn't automatically detected yet) was identified. That is that in order to create a tuple with a single element, a trailing comma is required, while otherwise the parentheses indicate a sub-expression:

(1, 2)      # -> tuple
(1 + 2) * 3 # -> int (parentheses semantically required)
(1)         # -> int (parentheses redundant)
(1,)        # -> tuple

However, these are just two examples where redundant usage of parentheses cause inconsistent code or even potential bugs. While on the other hand there are also cases where usage of parentheses, that aren't syntactically or semantically required, facilitate the understanding of the code. So how could we distinguish those two in an as abstract/generic as possible way, rather than explicitly targeting each individual pattern, which isn't a realistic approach (in particular on the parser/tokenizer level)?

In order to eliminate any case in which removal of parentheses would cause invalid code or different behavior, we can just recompile the code for each pair of parentheses with this pair removed, and ignore any case where the compilation fails or the generated AST differs from the original. This is much simpler and more reliable than considering all syntactical requirements and semantical implications ourselves.

What we are left with at this point are occurrences of parentheses, in the token stream, that are neither syntactically nor semantically required. If we map these to their corresponding AST nodes we can fairly easily implement any exception we want to grant for readability.

What to change

Make flake8-eyeo log an A111 error for each usage of parentheses that isn't syntactically or semantically required, with following exceptions (where parentheses can facilitate readability):

  • Tuple literals
  • Nested arithmetic, bitwise or boolean operations
  • Expressions, assigned to variables or used in keyword arguments, if they include comparisons

Change History (12)

comment:1 Changed 5 months ago by sebastian

  • Description modified (diff)

comment:2 Changed 4 months ago by rhowell

  • Owner set to rhowell

comment:3 Changed 4 months ago by rhowell

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed 4 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5844 - Remove redundant parentheses

comment:5 Changed 4 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5844 - Remove redundant parentheses in buildtools

comment:6 Changed 4 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5844 - Remove redundant parentheses

comment:7 Changed 4 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5844 - Remove redundant parentheses

comment:8 Changed 4 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5844 - Remove redundant parentheses

comment:9 Changed 4 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5844 - Detect (more) redundant parentheses

comment:10 Changed 4 weeks ago by sebastian

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:11 Changed 4 weeks ago by sebastian

  • Component changed from Sitescripts to Automation

comment:12 Changed 3 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5844 - Remove redundant parentheses in buildtools

Note: See TracTickets for help on using tickets.