google / pytype

A static type analyzer for Python code
https://google.github.io/pytype
Other
4.71k stars 273 forks source link

Possible name collision for same-named source file in different directories #198

Open rchen152 opened 5 years ago

rchen152 commented 5 years ago

From https://github.com/googleapis/google-cloud-python/pull/6769#issuecomment-443837463. That particular issue hasn't been figured out yet, but it got me to think about whether it's generally possible for pytype to accidentally map two source files to the same pyi path, and it indeed is. With a structure like:

root
|----dir1
|    |----foo.py
|----dir2
     |----foo.py

when pytype is run from root, ninja errors out with: ninja: error: build.ninja:11: multiple rules generate /usr/local/google/home/rechen/root/pytype_output/pyi/foo.pyi [-w dupbuild=err]. Perhaps we need to include the relative path from the current directory to the pythonpath directory in pytype_output.

martindemello commented 5 years ago

note that this is asking pytype to analyse two unrelated projects (that happen to be sitting in the same directory on the user's filesystem). i think we should flag this as an error (possibly with a more informative error message) rather than try to put them in the same output tree.

rchen152 commented 5 years ago

True, we may want to forbid this rather than supporting it. Either way, the current behavior (cryptic failure) isn't good.

cmarqu commented 4 years ago

I'm trying to run pytype on a project (https://github.com/cocotb/cocotb) where the same filename exists in two different directory (e.g., https://github.com/cocotb/cocotb/blob/master/cocotb/monitors/avalon.py and https://github.com/cocotb/cocotb/blob/master/cocotb/drivers/avalon.py) - here, this is not unrelated projects. I would therefore be happy if this kind of file layout could be supported.

rchen152 commented 4 years ago

Hmm, in this case, since all of the packages and subpackages have __init__.py files, I'd have expected pytype to be able to use the full module names to distinguish between the two files. Which directory are you running pytype from in which there's a name collision?

cmarqu commented 4 years ago

I have tried both the very toplevel directory as well as the cocotb subdirectory. Let me boot up the machine where I tried this and see if I can figure out more.

cmarqu commented 4 years ago

For some reason, I cannot reproduce this anymore. As a workaround, I had added one of those "duplicating" directories to the exclude list, but now that I removed it again, it still works, even after removing the .pytype output directory. Please disregard, sorry for the confusion.

rchen152 commented 4 years ago

No worries, thanks for looking into it again!

mrahtz commented 4 years ago

An example of this error even within one project is

git clone https://github.com/hill-a/stable-baselines
cd stable-baselines
git checkout 3840ddf752143f7fc1a54bf3debc125855df4ca2
patch -p0 -i patch.diff
pytype -o /tmp/out
pytype -P /tmp/out

where patch.diff contains

diff --git stable_baselines/common/base_class.py stable_baselines/common/base_class.py
index 994a539..e26b591 100644
--- stable_baselines/common/base_class.py
+++ stable_baselines/common/base_class.py
@@ -940,7 +940,7 @@ class ActorCriticRLModel(BaseRLModel):
                              "Stored kwargs: {}, specified kwargs: {}".format(data['policy_kwargs'],
                                                                               kwargs['policy_kwargs']))

-        model = cls(policy=data["policy"], env=None, _init_setup_model=False)
+        model = cls(policy=data["policy"], env=None, _init_setup_model=False)  # pytype: disable=not-instantiable
         model.__dict__.update(data)
         model.__dict__.update(kwargs)
         model.set_env(env)
@@ -1048,7 +1048,7 @@ class OffPolicyRLModel(BaseRLModel):
                              "Stored kwargs: {}, specified kwargs: {}".format(data['policy_kwargs'],
                                                                               kwargs['policy_kwargs']))

-        model = cls(policy=data["policy"], env=None, _init_setup_model=False)
+        model = cls(policy=data["policy"], env=None, _init_setup_model=False)  # pytype: disable=not-instantiable
         model.__dict__.update(data)
         model.__dict__.update(kwargs)
         model.set_env(env)

This gives ninja: error: build.ninja:46: multiple rules generate /__init__.pyi [-w dupbuild=err].

rchen152 commented 4 years ago

Hmm, why are you running pytype -P /tmp/out? -P is for paths to source code directories, whereas /tmp/out looks like it contains just-generated .imports and .pyi files.

mrahtz commented 3 years ago

I might well have been doing something silly here. I was trying to run pytype in type-check mode, using previously-generated stubs.

Looking through pytype --help, I wasn't sure what -P is supposed to do (why would you specify source code directories with -P rather than just specifying them as the input argument?), but running pytype-single --help, I saw there was a fuller description of -P, and took it to mean that it could be used to specify a directory with previously-generated stubs - and that this also applied to regular pytype. I guess this is not the case?

rchen152 commented 3 years ago

Ah, the -P arguments to pytype and pytype-single are similar but not quite the same. They're both for telling pytype where the dependencies of your input files are, but for pytype, you specify source files whereas for pytype-single you specify type stubs. You wouldn't just pass these files as inputs because they would then be type-checked.

mrahtz commented 3 years ago

Ahh, gotcha :) Thanks!

nirradi commented 2 years ago

This reproduces every time for me if there's no __init__.py in the folder of one of the duplicates.

lucaswiman commented 1 year ago

Agree with @nirradi that adding a missing __init__.py fixed the problem for me. To future visitors to this page: you may be missing __init__.py files somewhere in your packages.

Maybe the multiple rules generate error should be extended to include a note that pytype has poor compatibility with PEP 420 implicit namespace packages and to try adding __init__.py files?

rchen152 commented 1 year ago

Aha, knowing that adding __init__.py files fixes the problem is helpful, thanks! It sounds like the issue is probably here: https://github.com/google/pytype/blob/173a7a416787a1b37f4a69ac6d00e18c69788b3e/pytype/tools/environment.py#L71-L78. We're relying on the presence of __init__.py to find subpackages, which indeed does not work for implicit namespace packages.