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

Type error due to repeated use of underscore as dummy assignment target #465

Closed
spkersten opened this issue Sep 26, 2014 · 25 comments
Closed

Comments

@spkersten
Copy link
Contributor

Consider the following code:

from typing import Tuple

class A: pass
class B: pass
class C: pass

def f() -> Tuple[A, B, C]:
    pass

a = A()
a, _, _ = f()

mypy reports: line 11: Incompatible types in assignment (expression has type "C", variable has type "B")

Seems like mypy treats _ as just a variable. That prevents it from being used as a dummy in assignments like the one above.

A solution might be to always give _ the Any type.

@refi64
Copy link
Contributor

refi64 commented Sep 26, 2014

Technically, Python itself treats it as a variable:

ryan@DevPC-LX:~$ python3
Python 3.3.5 |Continuum Analytics, Inc.| (default, Mar 10 2014, 11:19:31) 
[GCC 4.1.2 20080704 (Red Hat 4.1.2-54)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> def a(_, _): pass
... 
  File "<stdin>", line 1
SyntaxError: duplicate argument '_' in function definition
>>> 1
1
>>> _
1
>>> 

See here.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 3, 2014

Hmm, this is a bit tricky. A conservative (but ad-hoc) option would be to give variables named _ the type Any if and only if they are only assigned to but never read. Actually, we could give any local variable that is only assigned to in a multiple assignment but that is never read the type Any, I think.

So this would be accepted, since dummy is never read:

def foo() -> None:
    foobar, dummy, dummy = 1, 2, 'x'
    g(foobar)

Not sure if this generalization would be that useful, though.

We could also give a warning if a local variable is only assigned to, unless this happens in a multiple assignment (or a similar context) where it's okay to ignore the values of all variables except at least one.

@o11c
Copy link
Contributor

o11c commented Aug 24, 2015

I argue that only names that start with an underscore should be considered.

I have some code like a, _b, _c = foo() where I remove the underscore if I'm actually going to use the variable but keep a name attached for documentation reasons.

@o11c
Copy link
Contributor

o11c commented Aug 31, 2015

Further complication: what about people who use _ for gettext ? IMO that is unique enough that it's worth special-casing.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 1, 2015

But the rule for ignoring only things are only assigned to would work with gettext / _, as then you'd actually read the value of _. We probably want to give a warning if there's a normal-looking local variable such as dummy that is never read.

@ilevkivskyi
Copy link
Member

(Raised priority to high since this request keeps coming.)

@gvanrossum
Copy link
Member

I propose the following implementation. In assignment statements (which excludes other defining contexts like import, class or def), if a target variable doesn't yet have a type (i.e. it's the first assignment to the variable) and the variable name is exactly _, infer a type of Any instead of the type of the RHS.

This includes unpacking assignments like _, _, x = foo() as well as multiple assignments like x = y = _ = bar(). It should also include star-assignments like x, *_ = baz().

Note that this doesn't cover other similar cases like _1, _2, x = foo(), but that's all right (such code is probably just a work-around for #465 :-).

It also doesn't cover more general variable reuse -- that's #1174 and it's more complex.

@ilevkivskyi
Copy link
Member

@gvanrossum I think it would still be safer if _ is never read. This is quite easy to implement, since we can already see this in semantic analysis before inferring the type during type checking. For example, if the name is visited not as lvalue, then it is stripped of its magic properties and will be given a normal inferred type. This will save the use case for i18n some people are worried about.

@gvanrossum
Copy link
Member

The use case for i18n is already taken care of since it is not an assignment -- the idiom is from gettext import gettext as _.

I have to think about how easy it is to detect whether _ is ever read -- it seems that would require a new attribute somewhere but I don't know where yet.

@ilevkivskyi
Copy link
Member

The use case for i18n is already taken care of since it is not an assignment -- the idiom is from gettext import gettext as _.

I think I have seen some cases where this was implemented as an assignment (in situations with custom gettext). Although this is rare, this will lead to surprising false negatives.

it seems that would require a new attribute somewhere but I don't know where yet

I think this will require a flag on Var.

@gvanrossum
Copy link
Member

OK, I will try it. Though I also found some code like this:

for foo, _ = bar():
    <stuff>
_ = _  # Trying to defeat some other static analysis tool

gvanrossum pushed a commit to gvanrossum/mypy that referenced this issue Mar 30, 2018
@gvanrossum
Copy link
Member

I ran out of time but here's the simpler version: https://github.com/gvanrossum/mypy/tree/underscore. This does not check that the variable is never used.

Note that the version you propose would also suffer from action at a distance:

_, x = foo()  # Error here if the print() way below is uncommented
.
.
.
# print(_)

@ilevkivskyi
Copy link
Member

Note that the version you propose would also suffer from action at a distance

Yes, this can be even in another module if I import _ from somewhere where it is used as _, _, x = foo(). We can wait for input from @JukkaL on safety vs action at a distance. But I already see that such action at a distance is problematic in incremental mode because it goes "upstream" to module deps.

@gvanrossum
Copy link
Member

@ilevkivskyi Perhaps if you have an idea you can leave a hint on where I could conveniently add a flag to Var for all uses of a variable but not assignments to it, in one of the semanal*.py passes?

I also worry that this might be complicated if you have e.g.

class C:
    _ = 0
    def foo(self) -> None:
        print(self._)

since IIRC the getattr operation isn't determined to reference class C until the type checking phase.

@ilevkivskyi
Copy link
Member

My idea was second pass of semantic analysis. The idea is that visit_name_expr etc. are never called on lvalues from visit_assignment_stmt. Then you can set the flag in visit_name if isinstance(n.node, Var), and the same in two places in visit_member_expr (one covers module attributes another covers Cls.x and self.x).

@ilevkivskyi
Copy link
Member

Anyway, as I said before I am now not so sure this is a good idea because of probable problems with incremental mode. To illustrate this:

[case testCheckErrorForUnderscoreIncremental]
import a
[file a.py]
from b import _
[file b.py]
_ = int()
_ = str()
[file a.py.2]
from b import _
x = _

The problem is that b will not be re-checked, and the error will not be shown on the second run.

@JelleZijlstra
Copy link
Member

Maybe we can apply the special casing only within functions. Most use cases are going to be within functions anyway.

@gvanrossum
Copy link
Member

gvanrossum commented Mar 31, 2018

Most use cases are going to be within functions anyway.

I'm not so sure -- the weird case I found (_ = _) was actually using _ at the global level.

Perhaps another more conservative approach would be to simply not infer a type if the variable name is _. Then accidental uses would be flagged with Name '_' is not defined. However this would still not help for

def gettext(): ...
_ = gettext
x = _("")

since the _("") call would become a false positive.

@gvanrossum
Copy link
Member

I have a further question of clarification (unrelated to the usage issue). Clearly for _, _ in ... should count as an assignment, since it's a common use case. (And my implementation supports it.) But should with cm() as _ count? And except E as _?

@ilevkivskyi
Copy link
Member

I would say with and except are overkill, because a variable name is not syntactically required in these cases.

@gvanrossum
Copy link
Member

a variable name is not syntactically required in these cases.

I'm sorry, apparently I am having a hard time parsing people's sentences today... Just to confirm, you mean that the entire as BLAH clause can be omitted when it's not needed? While that's true, at least with ... as BLAH can take a tuple unpacking, where _ could be handy. (This is not the case for except ... as BLAH though.)

I do think these two are rather marginal.

@gvanrossum
Copy link
Member

OTOH my current code actually does work for with ... as _ and except ... as _ and that's fine too.

I'm making it into a PR but we'll wait for Jukka.

@ilevkivskyi
Copy link
Member

Just to confirm, you mean that the entire as BLAH clause can be omitted when it's not needed?

Yes, this is what I mean.

OTOH my current code actually does work for with ... as _ and except ... as _ and that's fine too.

If this is easy to implement, I am fine with this.

@ilevkivskyi
Copy link
Member

Maybe we can apply the special casing only within functions. Most use cases are going to be within functions anyway.

Doing this only at the local scope is indeed very safe, since we will not have any problems with external visibility of _. So this is another possible compromise. I am however not sure how dominant is this situation (doing _, _, x = y at a local scope) as compared to other scopes.

@gvanrossum
Copy link
Member

I did a grep on some of our large codebases and this usage is indeed (nearly) only used in function scopes. So I am now okay with allowing this only at function scope. I need to figure out how to test for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants