Ensure variable is bound in FURB127

[henryiii]

This was happy, just reporting a cleanup:

pymalloc = None
try:
    pymalloc = bool(sysconfig.get_config_var("WITH_PYMALLOC"))
except AttributeError:
    pass

But after making the cleanup:

pymalloc = None
with contextlib.suppress(AttributeError):
    pymalloc = bool(sysconfig.get_config_var("WITH_PYMALLOC"))

skbuild/cmaker.py:436:17 [FURB127]: This variable is redeclared later, and can be removed here

In this case, it's not a huge deal (this is kind of ugly anyway so I'll probably rewrite it later), just thought I'd point it out if it's something you think is fixable and worth fixing.

[dosisod]

Thank you for mentioning this. From the user perspective it might be a bit annoying to fix a certain line, only to have it complain about something else when you run it again, but from a developer perspective, this is hard to maintain.

There are a lot of checks, and only more to come. If we start to have checks which depend on, or otherwise relate to another check, we risk having more fragile and harder to maintain checks. I like to think that each check is isolated, completely ignorant that any other check exists, except for itself. This reduces the amount of things you need to fix if something breaks.

In short, this is a minor side effect of having isolated checks which don't know about each other, and sometimes step on each other's toes. Hopefully that answers your question!

[henryiii]

My point is the second one is wrong (and the first check just happens to force you into the second situation and makes it more common). If sysconfig does not have the attribute, it errors out and skips the redefinition. So if you remove the first definition, this variable might never be defined.

[dosisod]

You're right, I did not pick up on that. Here is a MVP that shows it better (IMO):

from pathlib import Path
from contextlib import suppress, nullcontext

with suppress(FileNotFoundError):
    contents = Path("invalid filename").read_text()

print(contents)

# vs

with nullcontext():
    contents2 = Path("README.md").read_text()

print(contents2)

In this example, contents is unbound, since the exception prevents the assignment, whereas nullcontext does nothing, the exception isn't handled, and the print(contents2) line is never reached. This is a bug.

[dosisod]

Also, this made me realize that there is a different bug in the FURB107 check, in that suppress() with no args will not suppress anything, and I thought it actually suppressed everything.

[henryiii]

That's nice, actually; except: is a bug that I wish didn't exist. You can always be explicit and suppress BaseException - but that's probably never a good idea. Handling BaseException really, really should never just suppress it!

[dosisod]

I have fixed the bare exception suppress(), PR coming soon. I agree that bare exceptions shouldn't be used, but to make it have the same behavior you have to do suppress(BaseException).

The slightly harder issue is disabling this check if suppress() is used. There are instances where the following would be totally valid to warn against:

x = 1
with suppress(Exception):
    x = 2

Although contrived, there is no exception being raised here, though there is no good way of detecting that. This raises a different concern about other context managers which capture exceptions, leaving x unbound.

A better approach would be to emit a warning only if we know x will be bound. Version 1.0 of Mypy is slated to fix this. One of the items on the list is "Detect unbound variables", which is partially implemented here: python/mypy#13601 . It is slightly buggy, but will get better over time, hopefully in time for the v1 release.

I think our best option is to just skip emitting an error if we encounter suppress(), and when the unbound variable detector is released, we can switch to that instead.

[dosisod]

Didn't mean to close this