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

findFile edges case with absolute paths on Windows #72

Closed
jmitchell opened this issue Feb 27, 2017 · 7 comments
Closed

findFile edges case with absolute paths on Windows #72

jmitchell opened this issue Feb 27, 2017 · 7 comments
Assignees
Labels
type: a-bug The described behavior is not working as intended.

Comments

@jmitchell
Copy link

The examples below where findFile returns Nothing are surprising, and if possible I think they should return the obvious path.

Prelude> import System.Directory
Prelude System.Directory> doesFileExist "C:\\tmp\\Test.hs"
True
Prelude System.Directory> findFile [] "C:\\tmp\\Test.hs"
Nothing
Prelude System.Directory> findFile ["C:\\"] "tmp\\Test.hs"
Just "C:\\tmp\\Test.hs"
Prelude System.Directory> findFile [] "\\tmp\\Test.hs"
Nothing
Prelude System.Directory> findFile ["\\"] "tmp\\Test.hs"
Just "\\tmp\\Test.hs"
Prelude System.Directory> setCurrentDirectory "C:\\tmp"
Prelude System.Directory> findFile ["."] "Test.hs"
Just ".\\Test.hs"
@Rufflewind
Copy link
Member

What do you consider the correct behavior to be? The documentation is rather underspecified currently.

I double-checked the results on my Linux system and there is at least one inconsistency between platforms that needs to be remedied:

findFile [] "C:\\tmp\\Test.hs"    -- consistent with Linux
findFile ["C:\\"] "tmp\\Test.hs"  -- (!) succeeds on Linux
findFile [] "\\tmp\\Test.hs"      -- no Linux equivalent
findFile ["\\"] "tmp\\Test.hs"    -- no Linux equivalent
findFile ["."] "Test.hs"          -- consistent with Linux

@jmitchell
Copy link
Author

jmitchell commented Feb 27, 2017

Proposed invariant: If doesFileExist fname returns True, I expect findFile [] fname to return Just fname. The invariant is platform-agnostic.

Currently these two cases from above violate the proposed invariant on Windows.

Prelude System.Directory> findFile [] "C:\\tmp\\Test.hs"
Nothing
Prelude System.Directory> findFile [] "\\tmp\\Test.hs"
Nothing

I expect the first to return Just "C:\\tmp\\Test.hs" and the second to return Just "\\tmp\\Test.hs".

@jmitchell
Copy link
Author

@Rufflewind, do you mind showing the ghci output on Linux corresponding to findFile ["C:\\"] "tmp\\Test.hs" -- (!) succeeds on Linux along with doesFileExist "C:\\tmp\\Test.hs"?

@jmitchell
Copy link
Author

The slash type is significant in Linux (tested on Ubuntu 16.04). May be affecting the results you're seeing.

Prelude System.Directory> doesFileExist "tmp\\Test.hs"
False
Prelude System.Directory> doesFileExist "tmp/Test.hs"
True

In any case, I recommend the same invariant.

@Rufflewind
Copy link
Member

> findFile ["/home/rf"] "downloads/somefile.pdf"
Just "/home/rf/downloads/somefile.pdf"

@jmitchell
Copy link
Author

jmitchell commented Feb 27, 2017

Have you found any other cases that violate the invariant?

@Rufflewind
Copy link
Member

Rufflewind commented Feb 28, 2017

Oh, sorry, it seems I have misread. I thought there was a discrepancy between Windows and Linux, but that was a mistake on my part.


It seems the behavior that you are proposing concerns solely with regard to cases where the first argument is []. There are two possibilities:

  • The given filename is absolute, in which case findFile appears to be almost equivalent to doesFileExist, except it fails if the first argument is [].
  • The given filename is relative, in which case if there are no directories to begin with, it fails since there are no directories to consult.

I feel like the first behavior may have been accidental and not part of the original API: it only worked because </> disregards the first path when the second path is absolute. It would be wrong to revert that now, but it seems a lot of people have implicitly assumed that findFile always worked for absolute paths even though it was accidental (and unnecessarily inefficient, since it still has to go through every directory even if the path is absolute and nonexistent).

In isolation, the first behavior is quite absurd, so I think a compromise would be:

isAbsolute p ==> findFile [] p == bool (Just p) Nothing <$> doesFileExist p

This should minimize the effect on most code except those that rely on findFile to handle absolute paths.

  • Tweak behavior to handle absolute paths explicitly.
  • Document the new behavior.

@Rufflewind Rufflewind added the type: a-bug The described behavior is not working as intended. label Feb 28, 2017
@Rufflewind Rufflewind added this to the 1.3.0.3 milestone Feb 28, 2017
@Rufflewind Rufflewind self-assigned this Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: a-bug The described behavior is not working as intended.
Projects
None yet
Development

No branches or pull requests

2 participants