Proposed check: use suppress context manager about flake8-bugbear HOT 11 CLOSED

I did comment on the subtle bugs it might introduce, but there is another question: is bugbear supposed to be a tool to help detect bugs or to check coding style/conventions ? Is there anything inherently wrong about except: pass ? Does suppress fix anything ? In the end, using suppress is more of a style choice, and perhaps not so relevant for bugbear.

from flake8-bugbear.

Can you please explain more about what you feel all the benefits are here please? Generally most other things bugbear lints can actually be a bug and possible cause issues. I can’t see how the try/except usage can? flake8 already lints if you don’t use the exception via ‘as’.

Is there a performance benefit or any other benefit other than the syntax feeling cleaner? This also forced people to import contextlib, where the try/except does not.

from flake8-bugbear.

Hi.
Are you proposing as a code change in bugbear's code or as a warning to be reported when analyzing code ?

I personally think the suppress() context should be used sparingly, or not used at all.

A pitfall is the following case:

with contextlib.suppress(SomeError):
  value = some_function()
do_stuff(value) # NameError: value is not defined if some_function() threw an error

or

with contextlib.suppress(SomeError):
  some_function()
  return
assert 0 # This assert runs if some_function() threw an error.

Use of suppress() can introduce such subtle bugs. Another problem is that IDE's (I use PyCharm) do not recognize that the suppress() context silences exceptions, and as such will not flag that value might not be defined.

If any, I would actually suggest adding a new warning to bugbear about the use of contextlib.suppress(), instead of encouraging its use.

from flake8-bugbear.

Django had a ticket a while back to replace all the try ... except ...: pass blocks with contextlib.suppress(...): ..., but it was reverted due to performance concerns.

Even the people commenting on the issue that introduced it were indecisive about it. I think Ezio put it best:

This saves two lines, but makes the code less explicit, it can't be used for try/except/else or try/except/except, it requires an extra import, the implementation is simple enough that it doesn't necessary need to be in the stdlib, it might encourage bad style, and it's slower.

from flake8-bugbear.

Hello @cooperlees,

It's more like a design solution.

from flake8-bugbear.

Hi,

Thanks for suggesting this. Due to this lint idea not fixing a bug or Python 3 compatibility like most of the other 'B' errors I don't think this fits into flake8-bugbear. It could fit in the Opinionated warnings section, but unless another flake8-bugbear collaborator likes it I am not for it. I feel the more well known try/except will be more readable to most developers, and apart from saving lines I don't see any benefits. If you actually shared more benefits etc. I would be more open, but you did not.

I will leave this open for some other developers to see and comment on. If other people think this is a big win I'd love to hear why.

from flake8-bugbear.

@cooperlees thanks!

from flake8-bugbear.

Hello @joaoe,

This is the proposal for "opinionated warnings".

Use of suppress() can introduce such subtle bugs.

AFAIK It works the same as try ... except: pass pattern, so I don't see any subtle bugs here. Try this:

try:
    value = some_function()
except SomeError:
    pass
do_stuff(value) # NameError: value is not defined if some_function() threw an error

Another problem is that IDE's (I use PyCharm) do not recognize that the suppress() context silences exceptions, and as such will not flag that value might not be defined.

Yeah, that could be a problem.

from flake8-bugbear.

Hello @orlnub123,

Thanks for the links! Performance matters.

from flake8-bugbear.

Thanks for the feedback! You've convinced me.

from flake8-bugbear.

People who come here might be interested in SIM105 of flake8-simplify

from flake8-bugbear.