Repository navigation
Improve handling of global scope for partially defined check #14213
Description
Currently, partially defined check (responsible for two error codes: partially-defined and use-before-def) doesn't handle variables defined in outer scope very consistently.
Here's an example that generates no error right now:
def foo() -> None:
global x
print(x) # No error here right now.
foo() # This will result in a runtime error.
x = 1However, in some cases, the check also generates use-before-def errors:
def foo() -> None:
global x
print(x) # E: Name "x" is used before definition[use-before-def]
x = 1
x = 1Note that the same applies to inner functions.
The question is: what should the behavior be here? Should we generate a may be defined error in both cases? Or, perhaps, we should generate no error in either case (because it's a pretty common pattern).
@JukkaL @ilevkivskyi do you have thoughts?
Activity
ilevkivskyi commentedon Nov 28, 2022
I think it should be "may be undefined" in both cases. IMO the general rule is roughly like this: if mypy can prove it will always fail, it is "use before definition", and if mypy cannot prove it will not fail, it is "may be undefined". So it is fine to have some false negatives for the first, and some false positives for the second. But not vice versa.
ilinum commentedon Nov 29, 2022
I agree with the thought behind this (when to use which error code).
However, I'm a bit worried that it will generate too many false positives and I'm thinking that maybe we should explicitly assume that everything from outer scope is always defined, even if definition comes after.
I recently stumbled upon an interesting example:
So for the example with classes above, we will definitely need some special handling (make class scope special). I'm not convinced that the noise is worth it.
ilevkivskyi commentedon Nov 29, 2022
The example with method is obviously safe, but attribute access is totally different story and will require separate handling independently of what we do here.
Coming back to variables, the
partially-definederror code is going to stay off by default, so people who will opt in to it, will likely prefer false positives over false negatives (especially if there is a simple workaround like put variables at top of module). As a random data point, I have fixed a bad bug caused by use of a forward global ~year ago.Note however, code like this should not generate any errors:
Because this is actually a very common pattern, and a totally fine one. If it is hard to implement the global variable error without also causing an error in this last example, it is fine to leave it as a follow up issue. (But we definitely should not give the
use-before-deferror as we currently do.)ilinum commentedon Nov 29, 2022
The check is a bit non-trivial to implement (since we need to handle var vs def declaration in scope differently). For now, #14222 will make it not generate an error.
[partially defined] fix false positive with global/nonlocal vars (#14222
ilinum commentedon Feb 8, 2023
After #14517 is merged, this issue will largely be resolved.
We still don't detect the following case:
But I don't see how we can handle this in a reasonable way. We either need to flag all such uses as possibly-undefined (which will cause a ton of errors -- any time you're using a global var). Or we'll need to track the order in which functions are called (which seems very difficult to implement).
ilevkivskyi commentedon Feb 11, 2023
Yes, I think this issue can be closed by #14517. You can either open a separate (low-priority) issue to track the remaining tricky case, or just wait for someone to complain about this :-)
[used before def] improve handling of global definitions in local sco…