Opened 3 years ago

Closed 3 years ago

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

Reported by: Owned by: joeyhess thomie normal Core Libraries 7.9 filepath core-libraries-committee@… Windows Unknown/Multiple None/Unknown

### 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"


### comment:1 Changed 3 years ago by thomie

Owner: set to thomie 7.6.3 → 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 3 years ago by thomie (previous) (diff)

### comment:2 Changed 3 years 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 3 years 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
| 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
| 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 3 years ago by thomie (previous) (diff)

### comment:4 Changed 3 years ago by thoughtpolice

Component: libraries (other) → Core Libraries

Moving over to new owning component 'Core Libraries'.

### comment:5 Changed 3 years ago by NeilMitchell

Cc: core-libraries-committee@… added → duplicate new → 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.