Respect the semantics of EXPLICIT

[andras-kth]

Description

This PR removes the implicitly added current working directory when --explicit-package-bases is specified
and either MYPYPATH or mypy_path (or both) are non-empty. This follows the principle of least surprise,
as it respects the reasonable expectation that prescribing explicit package bases will not magically include
implicit bases. On the other hand, when explicit bases are not actually present, it does fall back to the current
practice of including the current working directory.

Rationale:

• With the --explicit-package-bases option, mypy has a tendency to erroneously treat folders
  in the current working directory that happen to match the name of an imported module in a source
  file being checked as "the" implementation of said module, producing errors like the following:

  user@host:~/Projects/test$ MYPYPATH=src mypy \
      --namespace-packages \
      --explicit-package-bases \
      --ignore-missing-imports \
      --exclude "$PWD/docker" \
      src/ns/mod.py
  src/ns/mod.py:1: error: Module "docker" has no attribute "api"
  Found 1 error in 1 file (checked 1 source file)
  user@host:~/Projects/test$

  Ideally, any such directory should be possible to --exclude, but I've yet to figure out how.
  The seemingly obvious --exclude "$PWD/docker" certainly doesn't work (as evidenced above).
  [NB#1: I've also tried endless variations of absolute and relative paths with or without a trailing slash.]
  [NB#2: Including --follow-imports=skip has no effect on the above.]

• Setting MYPYPATH explicitly to include the current working directory is simple,
  but telling mypy not to use the current working directory appears close to impossible.

• As noted in docs/source/running_mypy.rst:

  Setting :confval:mypy_path/MYPYPATH is mostly useful in the case
  where you want to try running mypy against multiple distinct
  sets of files that happen to share some common dependencies.

  In such a situation, the working directory may or may not hold relevant Python modules.
  This is not a problem when __init__.py markers can be used to decide what is a valid module.
  The --explicit-package-bases option, however, precludes that possibility.

• The following comment in mypy/modulefinder.py appears to display some confusion
  regarding why and when including the working directory is actually useful:

      # Do this even if running as a file, for sanity (mainly because with
      # multiple builds, there could be a mix of files/modules, so its easier
      # to just define the semantics that we always add the current director
      # to the lib_path
      # TODO: Don't do this in some cases; for motivation see see
      # https://github.com/python/mypy/issues/4195#issuecomment-341915031

  In my mind, this is a clear warning flag that control should be passed to the user.

Test Plan

Unfortunately, it's unclear how to write good test cases for this change, since the existing tests
for the relevant modules (essentially, mypy/test/{test_find_sources,testmodulefinder}.py)
appear to pass unaffected, which is not all too surprising, considering that the error reported
as part of this PR depends on the contents of the files and the tests only consider paths.

Here's a short script that recreates a minimal test case:

cd $(mktemp -d)
cleanup_mypy_test() {
    rm -frv "$PWD"
    cd -
}
mkdir -pv docker src/ns
echo FROM scratch > docker/Dockerfile
printf 'from docker import api\n\nhelp(api)\n' > src/ns/mod.py
MYPYPATH=src mypy --namespace-packages --explicit-package-bases --ignore-missing-imports --exclude docker src/ns/mod.py

The results with stock mypy are included above. With the changes in this PR, the output becomes:

Success: no issues found in 1 source file

The standard battery of test cases still succeeds, albeit I only tested on Python 3.8,
since I don't have any of the earlier versions installed. Tests were run using:

$ sed -i s/7/8/ tox.ini
$ tox
[... skip detailed output ...]
_____________________________________________ summary ______________________________________________
SKIPPED:  py35: InterpreterNotFound: python3.5                                                      
SKIPPED:  py36: InterpreterNotFound: python3.6                                                      
  py38: commands succeeded                                                                          
  lint: commands succeeded                                                                          
  type: commands succeeded                                                                          
  docs: commands succeeded                                                                          
  congratulations :) 
$

[hauntsaninja]

Thanks for your contribution and the clearly written up description of the problem and repro.

I agree that there's a problem here, but I don't think this is the right solution.

First, this isn't the right flag. --explicit-package-bases only affects the module name mypy associates with a given file, but it doesn't affect which files mypy will look at, nor should it. For what it's meant to do, not looking further up the file tree than the current directory is very much desirable (leaving breaking changes aside).

I'd maybe consider some other flag that lets you remove the current directory from the search paths, but that feels somewhat niche given there's a much clearer bug here: that follow_imports doesn't work. If you take your repro and add the following mypy.ini:

[mypy]
[mypy-docker.*]
follow_imports = skip

I'd expect MYPYPATH=src mypy --namespace-packages --explicit-package-bases src/ns/mod.py to pass, which it doesn't. But if you add touch docker/__init__.py, it does! Clearly something in the import following code predates namespace packages and doesn't handle this well. Fixing that will probably fix some other behaviours as well as providing a good solution here ("docker import isn't being followed to the right place? Use follow_imports" feels pretty natural).

I guess touch docker/__init__.py + follow imports also potentially gives you a shitty workaround for now.

[andras-kth]

Thanks for your contribution and the clearly written up description of the problem and repro.

I agree that there's a problem here, but I don't think this is the right solution.

I do expect you to know better; so, that's probably right. 😄

First, this isn't the right flag. --explicit-package-bases only affects the module name mypy associates with a given file, but it doesn't affect which files mypy will look at, nor should it. For what it's meant to do, not looking further up the file tree than the current directory is very much desirable (leaving breaking changes aside).

Interesting. I do have a different mental model, but I have only the actual documentation
(and, some of the source code I happened to stumble through trying to fix the bug) to base that on,
which leads me to believe that a clarifying update may be in order.

On another thought, is there anything wrong with also limiting where mypy looks for packages?
If --exlcude worked, I wouldn't have had to look for alternative fixes.

BTW, you mention breaking changes, which makes me curious about what you mean,
since your test suites passed without issues...

I'd maybe consider some other flag that lets you remove the current directory from the search paths,

Shouldn't --exclude be able to do that? Perhaps, not the current directory itself directly,
but specific sub-folders in any case...

[...] that feels somewhat niche given there's a much clearer bug here: that follow_imports doesn't work. If you take your repro and add the following mypy.ini:

[mypy]
[mypy-docker.*]
follow_imports = skip

I'd expect MYPYPATH=src mypy --namespace-packages --explicit-package-bases src/ns/mod.py to pass, which it doesn't. But if you add touch docker/__init__.py, it does! Clearly something in the import following code predates namespace packages and doesn't handle this well. Fixing that will probably fix some other behaviours as well as providing a good solution here ("docker import isn't being followed to the right place? Use follow_imports" feels pretty natural).

I guess touch docker/__init__.py + follow imports also potentially gives you a shitty workaround for now.

Wow... that's a really strange workaround. I'm guessing that it will also prevent mypy
from finding and using the actual docker module, which I believe it does with the "fix"
proposed here.

[nkakouros]

Any update on this? Is the use case covered now by some new flag or some other way?

[hauntsaninja]

It looks like the problem I identified in #10780 (review) I later fixed in #13758 , so closing this.

It's possible --exclude should imply a per-module follow_imports skip for matching modules, #10377 is the issue for that