-
Notifications
You must be signed in to change notification settings - Fork 767
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
False positive for possibly unbound variable #496
Comments
I agree that it should theoretically be possible to determine that a is not unbound in this instance, but I can't think of a practical way to determine this using the lazy evaluation technique employed in the pyright architecture. The TypeScript compiler (upon which the pyright architecture is based) does not handle this case either. Here are several workarounds:
|
This issue has been open for 30 days waiting for information. I'm going to close this issue but if the above guidance doesn't work for you, please let us know by reopening the issue. |
I don't think this is a waiting-for-info (if it is, let me know what more information I can give!); looks to me like Eric considers this a won't-fix given the limitations of pyright's architecture. |
We marked this as waiting-for-info to see if you would respond with any feedback for the workarounds, or if the original code is the only way to do it as intended. |
Here's another better workaround. I've seen this technique recommended in some mypy discussions. It involves the use of an def assert_never(a: NoReturn) -> NoReturn:
raise RuntimeError("Should not get here")
def f(x: Union[int, str, list]) -> None:
if isinstance(x, int):
a = 1
elif isinstance(x, str):
a = 2
elif isinstance(x, list):
a = 3
else:
assert_never(x)
print(a) |
Yeah, I used an There's also the question of the false negative for the unreachable code (which obviously doesn't need a workaround / isn't as big a deal). For what it's worth, |
Edit: I'm super wrong, sorry. Keeping what I wrote for posterity. Yeah, there's no way pylance could do this. def isinstance(x, type):
return False
from typing import Union
def f(x: Union[int, str, list]) -> None:
if isinstance(x, int):
a = 1
elif isinstance(x, str):
a = 2
elif isinstance(x, list):
a = 3
else:
print(type(x))
print(a)
f(3) This gives error: Exception has occurred: UnboundLocalError local variable 'a' referenced before assignment File "C:\code\blah.py", line 16, in f print(a) File "C:\code\blah.py", line 18, in f(3) pylance would have to be able to interpret and validate |
@mcrumiller, that's not a problem. Pylance knows the difference between builtins.isinstance and a locally-defined version. Narrowing logic is applied only for the builtins version. |
That's not really relevant — the same argument argument applies to any type narrowing involving isinstance. It's easy for static type checkers to detect the overloading you describe and all of them do. |
I am not sure about the first example but there is one other case that I encounter a lot, in many code bases, and it seems a bit more problematic in my opinion since it is a basic specificity of python: Here is an example: def best_unit(dt: float) -> Tuple[float, str]:
units = (
(1.0, "sec"),
(1e-3, "msec"),
(1e-6, "usec"),
(1e-9, "nsec"),
)
for scale, unit in units:
if dt >= scale:
break
return scale, unit # "unit" is possibly unbound Pylance(reportUnboundVariable) In this case the warning message is semantically incorrect because units is obviously not empty. |
A type checker has no way of knowing whether an iterable type is going to iterate zero or more times. It simply knows that it's iterable and what types it will return. In this case, A tuple is the only iterable type where a type checker sometimes (but not always) knows the length. All other iterable types (like lists, dictionaries, etc.) have lengths that are unknown to a static type checker. It's possible that in the special case of the tuple (as shown in your example), a type checker could avoid this false positive, but it would be complicated because the control flow graph would need to vary based on the type of the iterable. I'll give more thought to this and see if I can come up with a way to handle this case without introducing other false positives or greatly slowing down analysis. In the meantime, the workaround is straightforward. If you're going to use a variable after a loop, always initialize it before the loop. unit = ""
for scale, unit in units:
...
return scale, unit Or add a |
This issue has been fixed in version 2021.2.0, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#202120-3-february-2021 |
I can still see reproduce this in a try:
some_fn_that_can_throw_an_exception()
except Exception:
success = False
else:
success = True
finally:
print(success) # "success" is possibly unbound Or, am I missing something? |
The try/except/else/finally clause is different from this issue. Please refer to this issue instead. |
Thank you for the talk earlier today! I was reminded of this issue and thought it might be interesting to understand why lazy type evaluation means pyright can't determine that that branch is unreachable and "a" is unbound (although maybe I just need to start reading pyright code :-) Feel free to ignore this comment, it's basically just me rambling to myself. From the slides, it sounds like: pyright walks backward through the code flow graph. It sees Trying out some stuff, pyright doesn't complain about:
So clearly pyright is able to eliminate parts of the code flow graph as unreachable. But:
So seems like whatever reachability checking of the code flow graph pyright does happens before we get a chance to infer local types? mypy has some reachability analysis like Anyway, I think I see why it's not really feasible for pyright to determine branch reachability. There's just no good time to do it! So (idiomatic use of)
Here's an interesting example:
That sort of squares with "pyright determines reachability when building a code flow graph, at which point it doesn't have access to inferences made from code flow". Okay, but maybe pyright could still avoid the false positive for (I've already rambled longer than I thought I would, so I'm cutting myself off. Feel free to ignore, but I wouldn't mind code pointers or commentary) |
Like mypy, pyright is able to statically evaluate certain expression forms within the binder. This includes expressions like The other examples above involve expressions that cannot be evaluated by the binder. They involve the type evaluator, which depends on the code flow graph. All of the examples you provided could theoretically be handled by pyright in the type evaluator, but they would be expensive in terms of analysis performance. In response to this bug report, I added support for "implied else" narrowing. When I initially added this support, we saw a significant spike in analysis and completion times. I mitigated this by excluding the "falsy/truthy" type narrowing pattern, which is not typically used in cases where type exhaustion is the goal. The "implied else" support doesn't work in the case where the "else" is explicit, as in a couple of your cases above. I'm wondering whether you're raising this primarily because you're curious about a theoretical edge case, or is this a real case that you think is important to handle? Like I said, there are ways to handle this in pyright, but I question whether that would be the right tradeoff if it were to have a performance impact that affected the vast majority of pylance and pyright users. |
I don't think anything here is particularly important to handle and I trust you to make the correct tradeoffs. Oh neat, "implied else" is very similar to these examples. It looks like it might be implemented here: https://github.com/microsoft/pyright/blob/b9e33caee9823fc59a4ee53103ce5fcbdca1f71b/packages/pyright-internal/src/analyzer/binder.ts#L1125 I'll play around with it and see if I start to understand pyright's impl better. I also noticed this maybe-missing-feature?
|
This difference in behavior highlights another limitation of the "implied else narrowing" implementation. It requires that the reference expression for the condition (in this case, the reference expression is So your second example works if you annotate def func2():
x: Literal[True] = True
if x is True:
y = True
reveal_type(y) Here's the code that imposes that limitation: https://github.com/microsoft/pyright/blob/561ec5b093cc09c3b6216c51a161388447b551b4/packages/pyright-internal/src/analyzer/codeFlowEngine.ts#L721 My main motivation for adding "implied else narrowing" was to support type exhaustion checks like the sample at the top of this issue. And in those cases, the type of the reference expression is typically declared. |
Excelent workaround! |
Language Server version: Pylance language server 2020.10.0 (pyright d1f950ac)
Python version (& distribution if applicable, e.g. Anaconda): 3.8
Expected behaviour
Pylance should treat the else statement as being unreachable and shouldn't complain about the possibility of
a
being unbound.Actual behaviour
Pylance doesn't treat the else statement as being unreachable and does complain about the possibility of
a
being unbound.Code Snippet / Additional information
The text was updated successfully, but these errors were encountered: