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

Conversation

@ilevkivskyi
Copy link
Collaborator

commented Apr 16, 2019

This removes one TODO item that used to cause subtle bugs internally.

The test failed on Mac without the fix, I didn't test on Windows, but I think it should be good as well.

Ivan Levkivskyi

@ilevkivskyi ilevkivskyi requested a review from msullivan Apr 16, 2019

@msullivan

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

There is a danger with this approach: it checks case sensitivity for the entire path, not just parts of the path that correspond to a python module. This means that if we have something specified in the mypy search path that does not match the canonical casing, we won't be able to find anything in it.

I'm not sure if this is actually a problem? It could be, though. Worth documenting at least.

Fixing this means changing the API to include a root part that isn't checked, I guess.

@gvanrossum
Copy link
Member

left a comment

Oh, I hadn't thought of that. Then I disapprove. We should only check those path segments that correspond to packages -- users on Windows love to vary the case in pathnames.

Ivan Levkivskyi added some commits Apr 17, 2019

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

Fixing this means changing the API to include a root part that isn't checked, I guess.

Unfortunately this causes some code churn, because I need to "thread" this root part (prefix) through callers. Otherwise this looks a simple straightforward fix.


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

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum Apr 17, 2019

Member

Why prefix in head? That does a substring check and I can think of so many edge cases where this goes wrong (e.g. if prefix is '/' or ''). I would implement a more robust check that checks prefix is actually a filesystem prefix of head. Or you could remove prefix from head before you even enter the loop. Or you could use os.path.relpath(). Or the caller could be requested to pass the prefix and whatever follows separately (so you'd have pass os.path.join(prefix, head) to listdir(). Or...

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 17, 2019

Author Collaborator

Indeed.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 17, 2019

Author Collaborator

Actually after playing more with these, it looks like head.startswith(prefix) is the only one that behaves correctly, while other options I tried fail in some corner cases. Essentially, if the prefix is '' or '/' we should check the whole path in case sensitive manner.

Also could you please show a tricky test case, so that I can add it to the tests?

[[mypy]
mypy_path = tmp/funky_case

[case testPreferPackageOverFileCase]

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum Apr 17, 2019

Member

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.)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 17, 2019

Author Collaborator

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

@@ -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):

This comment has been minimized.

Copy link
@msullivan

msullivan Apr 17, 2019

Collaborator

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 17, 2019

Author Collaborator

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

Ivan Levkivskyi
@gvanrossum
Copy link
Member

left a comment

LGTM. The comment is optional.

@@ -209,7 +209,7 @@ def isfile_case(self, path: str, prefix: str) -> bool:

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

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum Apr 17, 2019

Member

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?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 17, 2019

Author Collaborator

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.

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

This PR is still labeled as "Changes required", but IIUC the last "LGTM" means it is good to go. So I am merging now.

@ilevkivskyi ilevkivskyi merged commit bb7dbd5 into python:master Apr 17, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:add-case-sense branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.