Fix name `path` not defined #5212

pslacerda · 2018-06-13T03:52:50Z

In #1422 __path__ implicit module attribute is not defined. This fix is not perfect because typing.List[str] would be better.

pslacerda · 2018-06-13T07:41:29Z

The last commit was to fix the tests.

ilevkivskyi

This fix is not perfect because typing.List[str] would be better.

The previous attempt #3527 didn't land exactly because of this. You could read discussions in that PR and improve your solution.

ilevkivskyi · 2018-06-13T11:18:30Z

test-data/unit/check-incomplete-fixture.test

@@ -12,7 +12,7 @@ import m
 m.x # E: "object" has no attribute "x"
 [file m.py]

-[case testListMissingFromStubs]
+[case testListMissingFromStubs-skip]


Why do you skip this test?

Because I thought that was not that important. I think that it can be fixed adding some .pyi stub that define list.

I tried del list but in mypy del deletes the value and not the variable itself. I would keep as is and mark this test to skip.

ilevkivskyi · 2018-06-13T11:19:33Z

test-data/unit/check-basic.test

+[case testModule__path__]
+import typing
+x = __path__ # type: list
+a = __path__ # type: A  # E: Incompatible types in assignment (expression has type "list", variable has type "A")


How about just using a reveal_type on __path__?

I agree, I was only going with the flow of other similar tests. Should them also use reveal_type on __name__ and etc?

As builtins.list is partially typed, reveal_type reveals builtins.list[Any] instead of builtins.list.

ilevkivskyi · 2018-06-13T11:20:24Z

mypy/nodes.py

@@ -101,7 +101,8 @@ def get_column(self) -> int:
 implicit_module_attrs = {'__name__': '__builtins__.str',
                         '__doc__': None,  # depends on Python version, see semanal.py
                         '__file__': '__builtins__.str',
-                         '__package__': '__builtins__.str'}
+                         '__package__': '__builtins__.str',
+                         '__path__': '__builtins__.list'}


IIRC __path__ is only defined in __init__.py modules, not in every module.

I thought that __path__ was defined everywhere. I will fix that.

pslacerda · 2018-06-13T15:34:25Z

#3527 was typing __path__ as __builtins__.str, because of this __path__.append() was failing.
In this PR the variable is correctly typed, even if only partially.

I didn't knew that __path__ was defined only in __init__.pyi? files.

The test was skipped because list was forced to be defined in every module if not previously defined, very much like True.

ilevkivskyi

Here are few more comments. Also I still think it should be List[str], not just list.

ilevkivskyi · 2018-06-14T10:52:55Z

mypy/semanal_pass1.py

+                # We are running tests without 'list' in builtins.
+                # TODO: Maybe optionally choose an default fixture stub per test file?
+                if 'list' not in self.sem.globals:
+                    literal_types.append(('list', AnyType(TypeOfAny.special_form)))


This is very dangerous. With this addition many tests will pass (of fail in ultra-weird way) if someone forgets to add a fixture with list. It is OK with literals, but not with a class.

Isn't list a literal?

No it is not, this construct (literal_types) is for things like None, True, False etc.

ilevkivskyi · 2018-06-14T10:54:46Z

test-data/unit/fixtures/primitives.pyi

@@ -20,4 +20,5 @@ class str:
 class bytes: pass
 class bytearray: pass
 class tuple(Generic[T]): pass
+class list(Generic[T]): pass


The header of this file says # builtins stub with non-generic primitive types, so I would avoid adding generic classes here. We have already enough fixtures with list, why do you need more?

Because it is a bultin, but I agree that there are enough fixtures.

ilevkivskyi · 2018-06-14T10:56:43Z

mypy/nodes.py

@@ -101,7 +101,8 @@ def get_column(self) -> int:
 implicit_module_attrs = {'__name__': '__builtins__.str',
                         '__doc__': None,  # depends on Python version, see semanal.py
                         '__file__': '__builtins__.str',
-                         '__package__': '__builtins__.str'}
+                         '__package__': '__builtins__.str',
+                         '__path__': '__builtins__.list'}


If you will fix this, you can then just take care to add list.pyi fixture to all tests that include __init__.py[i] files. I think there are not many of those.

Are you saying that __path__ shoun't be here because it exists only in __init__? So __init__.py[i] that uses __path__ must include an fixture?

Yes, please see the previous comments to this code. You already promised to fix it in #5212 (comment) two months ago.

It is OK to just close this if you don't have time/desire to work on this. If you still want to do this, then please go through all previous comments and fix mentioned issues.

gvanrossum · 2018-08-02T00:54:27Z

This seems stalled. @pslacerda are you planning to address the issues Ivan mentioned in the code review?

gvanrossum · 2018-09-29T04:15:40Z

Let's close this, it's stalled.

pslacerda added 2 commits June 13, 2018 00:47

Fix name __path__ not defined

3f478b4

Fix tests

1eb836a

pslacerda added 2 commits June 13, 2018 05:13

Better commentary

278f104

New test case

2e95389

ilevkivskyi reviewed Jun 13, 2018

View reviewed changes

ilevkivskyi reviewed Jun 14, 2018

View reviewed changes

gvanrossum closed this Sep 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix name `path` not defined #5212

Fix name `path` not defined #5212

pslacerda commented Jun 13, 2018 •

edited by ilevkivskyi

Loading

pslacerda commented Jun 13, 2018

ilevkivskyi left a comment

ilevkivskyi Jun 13, 2018

pslacerda Jun 13, 2018 •

edited

Loading

pslacerda Jun 13, 2018

ilevkivskyi Jun 13, 2018

pslacerda Jun 13, 2018

pslacerda Jun 13, 2018

ilevkivskyi Jun 13, 2018

pslacerda Jun 13, 2018

pslacerda commented Jun 13, 2018

ilevkivskyi left a comment

ilevkivskyi Jun 14, 2018

pslacerda Aug 3, 2018

ilevkivskyi Aug 7, 2018

ilevkivskyi Jun 14, 2018

pslacerda Aug 3, 2018

ilevkivskyi Jun 14, 2018

pslacerda Aug 3, 2018

ilevkivskyi Aug 7, 2018

gvanrossum commented Aug 2, 2018

gvanrossum commented Sep 29, 2018

Fix name __path__ not defined #5212

Fix name __path__ not defined #5212

Conversation

pslacerda commented Jun 13, 2018 • edited by ilevkivskyi Loading

pslacerda commented Jun 13, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pslacerda Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pslacerda commented Jun 13, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Aug 2, 2018

gvanrossum commented Sep 29, 2018

Fix name `path` not defined #5212

Fix name `path` not defined #5212

pslacerda commented Jun 13, 2018 •

edited by ilevkivskyi

Loading

pslacerda Jun 13, 2018 •

edited

Loading