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

Add case sensitive directory checks #6684

Merged
merged 7 commits into from Apr 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 22 additions & 5 deletions mypy/fscache.py
Expand Up @@ -181,14 +181,19 @@ def isfile(self, path: str) -> bool:
return False
return stat.S_ISREG(st.st_mode)

def isfile_case(self, path: str) -> bool:
def isfile_case(self, path: str, prefix: str) -> bool:
"""Return whether path exists and is a file.

On case-insensitive filesystems (like Mac or Windows) this returns
False if the case of the path's last component does not exactly
match the case found in the filesystem.
TODO: We should maybe check the case for some directory components also,
to avoid permitting wrongly-cased *packages*.
False if the case of path's last component does not exactly match
the case found in the filesystem.

We check also the case of other path components up to prefix.
For example, if path is 'user-stubs/pack/mod.pyi' and prefix is 'user-stubs',
we check that the case of 'pack' and 'mod.py' matches exactly, 'user-stubs' will be
case insensitive on case insensitive filesystems.

The caller must ensure that prefix is a valid file system prefix of path.
"""
if path in self.isfile_case_cache:
return self.isfile_case_cache[path]
Expand All @@ -198,9 +203,21 @@ def isfile_case(self, path: str) -> bool:
else:
try:
names = self.listdir(head)
# This allows to check file name case sensitively in
# case-insensitive filesystems.
res = tail in names and self.isfile(path)
except OSError:
res = False

# Also check the other path components in case sensitive way.
head, dir = os.path.split(head)
while res and head and dir and head.startswith(prefix):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good enough. Perhaps you could see if adding assert head.startswith(prefix) before line 211 always passes? OTOH perhaps that's risking too much.

I have a faint worry that e.g. head = '/foo/bar' and prefix = '/foo/b' but that's not possible given how prefix is constructed by the caller right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could see if adding assert head.startswith(prefix) before line 211 always passes? OTOH perhaps that's risking too much.

I added the assert, and all tests still passed. I think however it is better to not put it there, just in case.

I have a faint worry that e.g. head = '/foo/bar' and prefix = '/foo/b' but that's not possible given how prefix is constructed by the caller right?

Yes. I will probably just clarify this in the docstring.

try:
res = dir in self.listdir(head)
except OSError:
res = False
head, dir = os.path.split(head)

self.isfile_case_cache[path] = res
return res

Expand Down
38 changes: 22 additions & 16 deletions mypy/modulefinder.py
Expand Up @@ -215,28 +215,31 @@ def _find_module(self, id: str) -> Optional[str]:
near_misses = [] # Collect near misses for namespace mode (see below).
for base_dir, verify in candidate_base_dirs:
base_path = base_dir + seplast # so e.g. '/usr/lib/python3.4/foo/bar/baz'
dir_prefix = base_dir
for _ in range(len(components) - 1):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to strip off dir_chain directly with base_dir[:len(dir_chain)]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work (also it looks like you forgot minus there). I also tried with plus or minus one and some tests fail.

dir_prefix = os.path.dirname(dir_prefix)
# Prefer package over module, i.e. baz/__init__.py* over baz.py*.
for extension in PYTHON_EXTENSIONS:
path = base_path + sepinit + extension
path_stubs = base_path + '-stubs' + sepinit + extension
if fscache.isfile_case(path):
if verify and not verify_module(fscache, id, path):
near_misses.append(path)
if fscache.isfile_case(path, dir_prefix):
if verify and not verify_module(fscache, id, path, dir_prefix):
near_misses.append((path, dir_prefix))
continue
return path
elif fscache.isfile_case(path_stubs):
if verify and not verify_module(fscache, id, path_stubs):
near_misses.append(path_stubs)
elif fscache.isfile_case(path_stubs, dir_prefix):
if verify and not verify_module(fscache, id, path_stubs, dir_prefix):
near_misses.append((path_stubs, dir_prefix))
continue
return path_stubs
elif self.options and self.options.namespace_packages and fscache.isdir(base_path):
near_misses.append(base_path)
near_misses.append((base_path, dir_prefix))
# No package, look for module.
for extension in PYTHON_EXTENSIONS:
path = base_path + extension
if fscache.isfile_case(path):
if verify and not verify_module(fscache, id, path):
near_misses.append(path)
if fscache.isfile_case(path, dir_prefix):
if verify and not verify_module(fscache, id, path, dir_prefix):
near_misses.append((path, dir_prefix))
continue
return path

Expand All @@ -262,9 +265,10 @@ def _find_module(self, id: str) -> Optional[str]:
# foo/__init__.py it returns 2 (regardless of what's in
# foo/bar). It doesn't look higher than that.
if self.options and self.options.namespace_packages and near_misses:
levels = [highest_init_level(fscache, id, path) for path in near_misses]
levels = [highest_init_level(fscache, id, path, dir_prefix)
for path, dir_prefix in near_misses]
index = levels.index(max(levels))
return near_misses[index]
return near_misses[index][0]

# Finally, we may be asked to produce an ancestor for an
# installed package with a py.typed marker that is a
Expand Down Expand Up @@ -303,26 +307,28 @@ def find_modules_recursive(self, module: str) -> List[BuildSource]:
return result


def verify_module(fscache: FileSystemCache, id: str, path: str) -> bool:
def verify_module(fscache: FileSystemCache, id: str, path: str, prefix: str) -> bool:
"""Check that all packages containing id have a __init__ file."""
if path.endswith(('__init__.py', '__init__.pyi')):
path = os.path.dirname(path)
for i in range(id.count('.')):
path = os.path.dirname(path)
if not any(fscache.isfile_case(os.path.join(path, '__init__{}'.format(extension)))
if not any(fscache.isfile_case(os.path.join(path, '__init__{}'.format(extension)),
prefix)
for extension in PYTHON_EXTENSIONS):
return False
return True


def highest_init_level(fscache: FileSystemCache, id: str, path: str) -> int:
def highest_init_level(fscache: FileSystemCache, id: str, path: str, prefix: str) -> int:
"""Compute the highest level where an __init__ file is found."""
if path.endswith(('__init__.py', '__init__.pyi')):
path = os.path.dirname(path)
level = 0
for i in range(id.count('.')):
path = os.path.dirname(path)
if any(fscache.isfile_case(os.path.join(path, '__init__{}'.format(extension)))
if any(fscache.isfile_case(os.path.join(path, '__init__{}'.format(extension)),
prefix)
for extension in PYTHON_EXTENSIONS):
level = i + 1
return level
Expand Down
4 changes: 4 additions & 0 deletions mypy/test/testcheck.py
Expand Up @@ -89,6 +89,10 @@
if sys.version_info >= (3, 8):
typecheck_files.append('check-38.test')

# Special tests for platforms with case-insensitive filesystems.
if sys.platform in ('darwin', 'win32'):
typecheck_files.append('check-modules-case.test')


class TypeCheckSuite(DataSuite):
files = typecheck_files
Expand Down
69 changes: 69 additions & 0 deletions test-data/unit/check-modules-case.test
@@ -0,0 +1,69 @@
-- Type checker test cases dealing with modules and imports on case-insensitive filesystems.

[case testCaseSensitivityDir]
from a import B # E: Module 'a' has no attribute 'B'

[file a/__init__.py]
[file a/b/__init__.py]

[case testCaseInsensitivityDir]
# flags: --config-file tmp/mypy.ini

from a import B # E: Module 'a' has no attribute 'B'
from other import x
reveal_type(x) # E: Revealed type is 'builtins.int'

[file a/__init__.py]
[file a/b/__init__.py]
[file FuNkY_CaSe/other.py]
x = 1

[file mypy.ini]
[[mypy]
mypy_path = tmp/funky_case

[case testPreferPackageOverFileCase]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the test name is backwards. But don't we also need a test for the reverse? (Where the package has the right case and so is preferred.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a similar test case testPreferPackageOverFile in check-modules.test. I will add however another one here with mypy_path just in case.

# flags: --config-file tmp/mypy.ini
import a
[file funky/a.py]
/ # Deliberate syntax error, this file should not be parsed.
[file FuNkY/a/__init__.py]
pass

[file mypy.ini]
[[mypy]
mypy_path = tmp/funky

[case testNotPreferPackageOverFileCase]
import a
[file a.py]
'no'() # E: "str" not callable
[file A/__init__.py]
/ # Deliberate syntax error, this file should not be parsed.

[case testNamespacePackagePickFirstOnMypyPathCase]
# flags: --namespace-packages --config-file tmp/mypy.ini
from foo.bar import x
reveal_type(x) # E: Revealed type is 'builtins.int'
[file XX/foo/bar.py]
x = 0
[file yy/foo/bar.py]
x = ''
[file mypy.ini]
[[mypy]
mypy_path = tmp/xx, tmp/yy

[case testClassicPackageInsideNamespacePackageCase]
# flags: --namespace-packages --config-file tmp/mypy.ini
from foo.bar.baz.boo import x
reveal_type(x) # E: Revealed type is 'builtins.int'
[file xx/foo/bar/baz/boo.py]
x = ''
[file xx/foo/bar/baz/__init__.py]
[file yy/foo/bar/baz/boo.py]
x = 0
[file yy/foo/bar/__init__.py]

[file mypy.ini]
[[mypy]
mypy_path = TmP/xX, TmP/yY
2 changes: 1 addition & 1 deletion test-data/unit/check-modules.test
Expand Up @@ -1047,7 +1047,7 @@ from project.study.a import CustomType
[file project/study/a.py]
from project import root
# TODO (#4498): This test is basically testing the `all_are_submodules` logic
# in build, which skips generating a dependenecy to a module if
# in build, which skips generating a dependency to a module if
# everything in it is a submodule. But that is still all just a
# workaround for bugs in cycle handling. If we uncomment the next
# line, we'll still break:
Expand Down