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

B018 does not detect unused tuple expressions #14131

Open
LordAro opened this issue Nov 6, 2024 · 10 comments
Open

B018 does not detect unused tuple expressions #14131

LordAro opened this issue Nov 6, 2024 · 10 comments
Assignees
Labels
bug Something isn't working question Asking for support or clarification

Comments

@LordAro
Copy link

LordAro commented Nov 6, 2024

B018 (useless-expression) does not detect function calls with bare tuples:

def foo(): ...

foo(),

COM818 (trailing-comma-on-bare-tuple) detects this as it should, but I feel like B018 should flag this as well?

Ruff 0.7.2

@MichaReiser
Copy link
Member

MichaReiser commented Nov 6, 2024

Flagging this as B018 is only correct if foo has no side effects. For example, you could have

print("test"),

Removing the print would change the program behavior, which is why it's not useless.

Reasoning whether a function is only be possible in the most trivial cases because standard library functions in typeshed don't annotate if they have side effects.

That's why Ruff's current behavior is correct because it would be unsafe to assume the function has no side effects.

@MichaReiser MichaReiser added the question Asking for support or clarification label Nov 6, 2024
@LordAro
Copy link
Author

LordAro commented Nov 6, 2024

Indeed, and a print(), is originally how I found this behaviour.

Maybe the answer is just to enable flake8-commas linting...

@LordAro
Copy link
Author

LordAro commented Nov 6, 2024

Actually, hmm. It's definitely still a useless expression (unassigned tuple), just a weirdly formatted one where the "fix" is to remove the , rather than the entire expression

@MichaReiser
Copy link
Member

Yeah. I think detecting unnecessary tuples could be in the spirit of the rule, but it could also be its own rule, similar to useless-comparison.

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer and removed question Asking for support or clarification labels Nov 6, 2024
@MichaReiser MichaReiser changed the title B018 does not detect unused tuple expressions when they contain function calls B018 does not detect unused tuple expressions Nov 6, 2024
@dylwil3
Copy link
Collaborator

dylwil3 commented Nov 6, 2024

To me this seems to be working as I'd expect (but I'm happy to be overruled by the community).

On the one hand we have a lint rule detecting whether an expression is useless. I would think that if removing something useless affected the behavior of the program, then it must not have been useless to begin with. In other words, "useless" seems, to me, to be synonymous with "can be removed with no effect". The expression in question is a tuple expression, and removing it would necessarily remove all of its members.

There is no expression in the AST that's pointing to the "nonexistent second member":

~ echo "f()," | python -m ast
Module(
   body=[
      Expr(
         value=Tuple(
            elts=[
               Call(
                  func=Name(id='f', ctx=Load()),
                  args=[],
                  keywords=[])],
            ctx=Load()))],
   type_ignores=[])

So there's not really an expression you could point to and call "useless", right?

On the other hand, as you point out, we have a lint rule detecting trailing commas on bare tuples. So I'd think it would be the job of that lint rule to fix this expression.

@MichaReiser
Copy link
Member

I think the idea here is that print("hello"), would be fixed to just print("hello"), because the tuple expression is useless, not its content. But that might be difficult to convey in a diagnostic.

On the other hand, as you point out, we have a lint rule detecting trailing commas on bare tuples. So I'd think it would be the job of that lint rule to fix this expression.

I should have taken a closer look at that rule. I agree, we already have a rule that covers what you want.

@MichaReiser MichaReiser added question Asking for support or clarification and removed needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule labels Nov 6, 2024
@LordAro
Copy link
Author

LordAro commented Nov 6, 2024

Ah, ok, I've properly understood what I want: pylint W0106 which does warn about such things

def foo(): ...
[1, 2, foo()]
print("foo"),
test.py:1:0: W0106: Expression "[1, 2, foo()]" is assigned to nothing (expression-not-assigned)
test.py:2:0: W0106: Expression "(print('foo'), )" is assigned to nothing (expression-not-assigned)

obviously the fix is not to remove it entirely (as functions may have side effects) - it's one of

_ = [1, 2, foo()]  # a
foo()  # b

I don't think you'll need a type checker to do that (though pyright does detect the unused expression)

  /foo/test.py:1:1 - error: Expression value is unused (reportUnusedExpression)
  /foo/test.py:2:1 - error: Expression value is unused (reportUnusedExpression)

in #970 this lint is listed as implemented via B018, so...

@dylwil3
Copy link
Collaborator

dylwil3 commented Nov 6, 2024

Ok thanks this helps me understand better!

My takeaway is that W0106 is a slightly different rule that should just be implemented on its own, and would maybe suggest the fix "a".

I still feel like "useless" indicates "can be removed" not "should be modified", so I would vote to keep B018 from emitting a diagnostic for this case, but then you have COM818 and a newly implemented W0106 to fall back on. Does that seem ok? I'm still open to being convinced out of my pedantic interpretation of the word "useless"!

@LordAro
Copy link
Author

LordAro commented Nov 6, 2024

Actual flake8-bugbear flags up this specific example:

B018: Found useless expression. Either assign it to a variable or remove it. Note that dangling commas will cause things to be interpreted as useless tuples. For example, in the statement print(".."), is the same as (print(".."),) which is an unassigned tuple. Simply remove the comma to clear the error.

$ flake8 --select B test.py
test.py:1:1: B018 Found useless List expression. Consider either assigning it to a variable or removing it.
test.py:2:1: B018 Found useless Tuple expression. Consider either assigning it to a variable or removing it.

@dylwil3
Copy link
Collaborator

dylwil3 commented Nov 6, 2024

Alright, I stand corrected! We should certainly have parity with bugbear for this. Thanks for your persistence in the face of my pedantry!

@dylwil3 dylwil3 reopened this Nov 6, 2024
@dylwil3 dylwil3 added the bug Something isn't working label Nov 6, 2024
@dylwil3 dylwil3 self-assigned this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Asking for support or clarification
Projects
None yet
Development

No branches or pull requests

3 participants