#8752 closed bug (duplicate)

System.FilePath.Windows.combine does not really check isAbsolute

Reported by: joeyhess Owned by: thomie
Priority: normal Milestone:
Component: Core Libraries Version: 7.9
Keywords: filepath Cc: core-libraries-committee@…
Operating System: Windows Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

"Combine two paths, if the second path isAbsolute, then it returns the second.¨

But, the implementation of combine checks if the first character of a path is a path separator, which on Windows is not the same as checking if isAbsolute.

This can have counterintuitive results. For example:

    import System.FilePath.Windows

    prop_windows_is_sane :: Bool
    prop_windows_is_sane = isAbsolute p || ("C:\\STUFF" </> p /= p)
      where p = "\\foo\\bar"

Change History (5)

comment:1 Changed 11 months ago by thomie

  • Owner set to thomie
  • Version changed from 7.6.3 to 7.9

Are you proposing a change to the behavior of combine, or can we just change its docstring to:

"Combine two paths, if the second path 'hasDrive' or starts with a path separator, then it returns the second."

Edit: s/isAbsolute/hasDrive/

Last edited 11 months ago by thomie (previous) (diff)

comment:2 Changed 11 months ago by joeyhess

Changing the doc string in only System.FilePath.Windows.combine would be confusing because System.FilePath.Posix.combine has the same doc string. And I assume most users just use System.FilePath and assume it works for any OS, so probably won't notice if the windows version has a caveat added.

Would changing the implementation to really check isAbsolute be likely to break things?

comment:3 Changed 11 months ago by thomie

Can you explain why the docstring I posed is confusing? It is valid for both Posix as Windows, so it's not really a caveat.

Correct me if I'm wrong, but the way I understand it is that on Windows, if a filepath starts with a single slash, it is considered to be relative to the root of the current drive. The system keeps track of the current drive along with the current directory of that drive.

Currently, if the second argument starts with a slash, combine returns it as is. The implementation is essentially:

combine :: FilePath -> FilePath -> FilePath 
combine a b | hasDrive b || hasLeadingPathSeparator b = b
            | otherwise = combineAlways a b

I am guessing you are looking for the following behavior:

combine "C:\\" "\\foo\\bar" == "C:\\foo\\bar"
combine "C:\\STUFF" "\\foo\\bar" == "C:\\foo\\bar"
combine "\\\\shared" "\\foo\\bar" == "\\\\shared\\foo\\bar"
combine "\\\\shared\\stuff" "\\foo\\bar" == "\\\\shared\\foo\\bar"

But, since anything else doesn't make any sense:

combine "home" "\\bob" == "\\bob"

So just changing the check to isAbsolute (or hasDrive, since combining with "C:foo", which isRelative, is not implemented at the moment, but that is another issue) doesn't work.

An implementation that has this behavior would be:

combine :: FilePath -> FilePath -> FilePath 
combine a b | hasDrive b || hasLeadingPathSeparator b && not (hasDrive a) = b
            | hasLeadingPathSeparator b = combineAlways (takeDrive a) (dropLeadingPathSeparator b)
            | otherwise = combineAlways a b

Or, an option with the same behavior as above, except:

combine "C:\\STUFF" "\\foo\\bar" == "\\foo\\bar"
combine ""\\\\shared\\stuff" "\\foo\\bar" == "\\foo\\bar"

, would be:

combine :: FilePath -> FilePath -> FilePath 
combine a b | hasDrive b || hasLeadingPathSeparator b && not (isDrive a) = b
            | hasLeadingPathSeparator b = combineAlways a (dropLeadingPathSeparator b)
            | otherwise = combineAlways a b

I'm not sure which of the three is better. We have to change the docstring either way.

Edit: formatting

Last edited 10 months ago by thomie (previous) (diff)

comment:4 Changed 10 months ago by thoughtpolice

  • Component changed from libraries (other) to Core Libraries

Moving over to new owning component 'Core Libraries'.

comment:5 Changed 10 months ago by NeilMitchell

  • Cc core-libraries-committee@… added
  • Resolution set to duplicate
  • Status changed from new to closed

I've migrated this ticket to https://github.com/haskell/filepath/issues/8, where it will be dealt with outside GHC, and then picked up by GHC when next merged.

Note: See TracTickets for help on using tickets.