Fix W.splitFileName "/\\?/a:" #220

Bodigrim · 2024-01-21T20:29:17Z

Fixes #219.

hasufell · 2024-01-23T09:27:56Z

System/FilePath/Internal.hs

+      , null bs''
+      , Just (drive, rest) <- readDriveLetter file
+      -> (dirSlash <> drive, rest)
+    _ -> (dirSlash, file)


I think this line is redundant? I'tll fall through to | otherwise, no?

Unfortunately it does not fall through, the line is required.

Oops, I misread the code.

hasufell · 2024-01-23T09:47:39Z

We have equivalence tests that compare this function with the old implementation. It's depressing that it wasn't caught. The quickcheck generators are not good enough and don't produce enough "odd data".

I think one way forward is to implement the ABNF that I have here: https://github.com/hasufell/filepath/blob/16e5374620189b27eca1eed09642ec02b2222fc8/System/FilePath/Internal/Parser.hs#L157-L165

...at least for the test suite and use that to generate windows filepaths.

hasufell · 2024-01-23T09:59:03Z

System/FilePath/Internal.hs

+      -- Otherwise, since s1 is a path separator, we might be in the middle of UNC path.
+      , null bs' || maybe False isIncompleteUNC (readDriveUNC dirSlash)
+      -> (fp, mempty)
+      -- This handles inputs like "//?/A:" and "//?/A:foo"


Trivia: //?/A:foo is actually invalid in some sense... it's hard to interpret it. UNC paths are absolute, but A:foo on its own is a relative directory (relative to current working dir on drive A). The windows API probably won't care, but no such file can ever exist, I think.

True, but filepath-1.4.2.2 does not mind to process //?/A:foo even if it's complete nonsense.

Bodigrim · 2024-01-23T21:12:17Z

I started this PR with thinking about properties of splitFileName. One of them is that takeDrive x isPrefixOf dropFileName x. Testing such property indeed reveals the originally reported issue with \\?\a: and also \\?\a:foo.

hasufell · 2024-01-24T09:51:15Z

I pushed updated equivalence tests that now make use of the ABNF to generate more odd cases. Lots of new failures:

$ cabal run --enable-tests filepath-equivalent-tests -- --quickcheck-tests 50_000 -p 'windows'
Running 1 test suites...
Test suite filepath-equivalent-tests: RUNNING...
equivalence
  pathSeparator (windows):             OK
    +++ OK, passed 1 test.
  pathSeparators (windows):            OK
    +++ OK, passed 1 test.
  isPathSeparator (windows):           OK (0.13s)
    +++ OK, passed 50000 tests.
  searchPathSeparator (windows):       OK
    +++ OK, passed 1 test.
  isSearchPathSeparator (windows):     OK (0.13s)
    +++ OK, passed 50000 tests.
  extSeparator (windows):              OK
    +++ OK, passed 1 test.
  isExtSeparator (windows):            OK (0.17s)
    +++ OK, passed 50000 tests.
  splitSearchPath (windows):           OK (2.90s)
    +++ OK, passed 50000 tests.
  splitExtension (windows):            OK (0.60s)
    +++ OK, passed 50000 tests.
  takeExtension (windows):             OK (0.74s)
    +++ OK, passed 50000 tests.
  replaceExtension (windows):          OK (0.99s)
    +++ OK, passed 50000 tests.
  dropExtension (windows):             OK (0.62s)
    +++ OK, passed 50000 tests.
  addExtension (windows):              OK (0.94s)
    +++ OK, passed 50000 tests.
  hasExtension (windows):              FAIL (0.03s)
    *** Failed! Falsified (after 3395 tests and 1 shrink):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[UnixSep,UnixSep,UnixSep,WindowsSep,UnixSep,UnixSep,UnixSep,WindowsSep,WindowsSep] (Rel2 FileName (NonEmptyString ','":|""9\157814") (Just DS1 (NonEmptyString '('":|"".R") Nothing))) (\\?\a:////\///\\,9𦡶:(.R)
    Use --quickcheck-replay=129836 to reproduce.
    Use -p '/windows/&&/hasExtension (windows)/' to rerun this test only.
  splitExtensions (windows):           FAIL (0.12s)
    *** Failed! Falsified (after 8118 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[UnixSep,UnixSep,WindowsSep,WindowsSep,WindowsSep,WindowsSep,UnixSep,UnixSep,WindowsSep] (Rel2 FileName (NonEmptyString '<'":|""\SUB\36214\a\SIo\f\EOT?\SYN\40354g.\t\16570:") Nothing)) (\\?\a:///\\\\//\<赶o
                                                                                                        ?鶢g.	䂺:)
    Use --quickcheck-replay=619351 to reproduce.
    Use -p '/windows/&&/splitExtensions (windows)/' to rerun this test only.
  dropExtensions (windows):            FAIL (0.23s)
    *** Failed! Falsified (after 12623 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[WindowsSep] (Rel2 FileName (NonEmptyString '\154525'":|"".") Nothing)) (\\?\a:\\𥮝.)
    Use --quickcheck-replay=39137 to reproduce.
    Use -p '/windows/&&/dropExtensions (windows)/' to rerun this test only.
  takeExtensions (windows):            OK (0.40s)
    +++ OK, passed 50000 tests.
  replaceExtensions (windows):         FAIL (0.19s)
    *** Failed! Falsified (after 8184 tests and 3 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[UnixSep] (Rel2 FileName (NonEmptyString '\11591'":|"".r") Nothing)) (\\?\a:\/ⵇ.r)
    ""
    Use --quickcheck-replay=673584 to reproduce.
    Use -p '/windows/&&/replaceExtensions (windows)/' to rerun this test only.
  isExtensionOf (windows):             OK (0.51s)
    +++ OK, passed 50000 tests.
  stripExtension (windows):            OK (0.45s)
    +++ OK, passed 50000 tests.
  splitFileName (windows):             FAIL
    *** Failed! Falsified (after 245 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,UnixSep] (Rel2 FileName (NonEmptyString '('":|""") Nothing)) (\\?\a:/\/()
    Use --quickcheck-replay=177366 to reproduce.
    Use -p '/windows/&&/splitFileName (windows)/' to rerun this test only.
  takeFileName (windows):              FAIL
    *** Failed! Falsified (after 157 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,WindowsSep,WindowsSep,WindowsSep,UnixSep] (Rel2 FileName (NonEmptyString '\1096994'":|""wUN\97297if") Nothing)) (\\?\a:/\\\\/􋴢wUN𗰑if)
    Use --quickcheck-replay=369219 to reproduce.
    Use -p '/windows/&&/takeFileName (windows)/' to rerun this test only.
  replaceFileName (windows):           FAIL
    *** Failed! Falsified (after 414 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[WindowsSep,WindowsSep,WindowsSep,WindowsSep,WindowsSep,UnixSep,WindowsSep,WindowsSep,UnixSep,UnixSep] (Rel2 FileName (NonEmptyString 'v'":|""\RS}\128611(4") Nothing)) (\\?\a:\\\\\\/\\//v}🙣(4)
    ""
    Use --quickcheck-replay=357124 to reproduce.
    Use -p '/windows/&&/replaceFileName (windows)/' to rerun this test only.
  dropFileName (windows):              FAIL
    *** Failed! Falsified (after 231 tests and 1 shrink):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[UnixSep,UnixSep,UnixSep,UnixSep,WindowsSep,UnixSep] (Rel2 FileName (NonEmptyString '.'":|""'\SYNL\83354\ACK\SO_") Nothing)) (\\?\a://///\/.'L𔖚_)
    Use --quickcheck-replay=328899 to reproduce.
    Use -p '/windows/&&/dropFileName (windows)/' to rerun this test only.
  takeBaseName (windows):              FAIL
    *** Failed! Falsified (after 740 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' WindowsSep":|"[WindowsSep,UnixSep,UnixSep,WindowsSep,WindowsSep,UnixSep,WindowsSep,UnixSep,WindowsSep,WindowsSep,UnixSep,UnixSep,WindowsSep] (Rel2 FileName (NonEmptyString ')'":|""w\1028602\143532G<DQ~%\1043888V8\DLE\993471\188230Y") Nothing)) (\\?\a:\\//\\/\/\\//\)w󻇺𣂬G<DQ~%󾶰V8󲢿𭽆Y)
    Use --quickcheck-replay=581857 to reproduce.
    Use -p '/windows/&&/takeBaseName (windows)/' to rerun this test only.
  replaceBaseName (windows):           FAIL
    *** Failed! Falsified (after 379 tests and 3 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep] (Rel2 FileName (NonEmptyString '\NUL'":|""\1056142GWL|") Nothing)) (\\?\a:/\􁶎GWL|)
    ""
    Use --quickcheck-replay=609582 to reproduce.
    Use -p '/windows/&&/replaceBaseName (windows)/' to rerun this test only.
  takeDirectory (windows):             FAIL
    *** Failed! Falsified (after 993 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,WindowsSep,WindowsSep,UnixSep,UnixSep,WindowsSep] (Rel2 FileName (NonEmptyString '\1085460'":|""\DC1XJg%d\1074770J8@A") Nothing)) (\\?\a:/\\\//\􉀔XJg%d􆙒J8@A)
    Use --quickcheck-replay=443621 to reproduce.
    Use -p '/windows/&&/takeDirectory (windows)/' to rerun this test only.
  replaceDirectory (windows):          FAIL
    *** Failed! Falsified (after 188 tests and 2 shrinks):
    NS FileNameSpace [] (NST1 'a' UnixSep":|"[WindowsSep,WindowsSep,WindowsSep,UnixSep,UnixSep] (Rel2 FileName (NonEmptyString '\62007'":|""\DC1F") Nothing)) (\\?\a:/\\\//F)
    ""
    Use --quickcheck-replay=875347 to reproduce.
    Use -p '/windows/&&/replaceDirectory (windows)/' to rerun this test only.
  combine (windows):                   OK (0.81s)
    +++ OK, passed 50000 tests.
  splitPath (windows):                 OK (0.75s)
    +++ OK, passed 50000 tests.
  joinPath (windows):                  OK (0.90s)
    +++ OK, passed 50000 tests.
  splitDirectories (windows):          OK (0.89s)
    +++ OK, passed 50000 tests.
  splitDrive (windows):                OK (0.54s)
    +++ OK, passed 50000 tests.
  joinDrive (windows):                 OK (0.82s)
    +++ OK, passed 50000 tests.
  takeDrive (windows):                 OK (0.23s)
    +++ OK, passed 50000 tests.
  hasDrive (windows):                  OK (0.18s)
    +++ OK, passed 50000 tests.
  dropDrive (windows):                 OK (0.52s)
    +++ OK, passed 50000 tests.
  isDrive (windows):                   OK (0.23s)
    +++ OK, passed 50000 tests.
  hasTrailingPathSeparator (windows):  OK (0.30s)
    +++ OK, passed 50000 tests.
  addTrailingPathSeparator (windows):  OK (0.53s)
    +++ OK, passed 50000 tests.
  dropTrailingPathSeparator (windows): OK (0.53s)
    +++ OK, passed 50000 tests.
  normalise (windows):                 OK (1.14s)
    +++ OK, passed 50000 tests.
  equalFilePath (windows):             OK (1.43s)
    +++ OK, passed 50000 tests.
  makeRelative (windows):              OK (2.17s)
    +++ OK, passed 50000 tests.
  isRelative (windows):                OK (0.19s)
    +++ OK, passed 50000 tests.
  isAbsolute (windows):                OK (0.19s)
    +++ OK, passed 50000 tests.
  isValid (windows):                   OK (0.41s)
    +++ OK, passed 50000 tests.
  makeValid (windows):                 OK (1.03s)
    +++ OK, passed 50000 tests.

12 out of 48 tests failed (23.10s)

hasufell · 2024-01-24T10:12:45Z

More specifically for splitFileName:

This PR:

ghci> System.FilePath.Windows.splitFileName "\\\\?\\a:///\\\\\\\\k1"
("\\\\?\\a:///\\\\\\\\k1","")

filepath-1.4.2.2:

ghci> System.FilePath.Windows.splitFileName "\\\\?\\a:///\\\\\\\\k1"
("\\\\?\\a:///\\\\\\\\","k1")

hasufell · 2024-01-25T08:57:41Z

The bug was in hasPenultimateColon

hasufell · 2024-01-25T09:00:34Z

System/FilePath/Internal.hs

+      | isPathSeparator s1
+      , isPathSeparator s2
+      , Just (s3, s4, bs'') <- uncons2 bs'
+      , s3 == _question


Note that this is in fact a bug (although the old filepath behaves the same way):

ghci> System.FilePath.Windows.splitFileName "\\\\.\\a:foo" ("\\\\.\\","a:foo")

There are three UNC namespaces:

file namespace: //?/

device namespace: //./

NT namespace /??/

I'm still unsure whether fixing those bugs will cause more good than harm, but those are bugs.

Bodigrim requested a review from hasufell January 21, 2024 20:29

Bodigrim force-pushed the windows-strikes-again branch from 0449461 to 4cf5d70 Compare January 21, 2024 20:29

Bodigrim added 2 commits January 21, 2024 20:56

Add more tests on splitFileName and dropFileName

399df96

Fix regression of splitFileName on '//?/A:'

c394803

Bodigrim force-pushed the windows-strikes-again branch from 4cf5d70 to c394803 Compare January 21, 2024 20:56

hasufell reviewed Jan 23, 2024

View reviewed changes

Improve equivalence tests

67a45a4

hasufell added 2 commits January 24, 2024 18:16

Fix AltShow Maybe instance

0530486

Truly fix splitFileName on windows

74713b9

hasufell reviewed Jan 25, 2024

View reviewed changes

hasufell merged commit 74713b9 into master Jan 26, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix W.splitFileName "/\\?/a:" #220

Fix W.splitFileName "/\\?/a:" #220

Bodigrim commented Jan 21, 2024

hasufell Jan 23, 2024

Bodigrim Jan 23, 2024

hasufell Jan 24, 2024

hasufell commented Jan 23, 2024 •

edited

Loading

hasufell Jan 23, 2024

Bodigrim Jan 23, 2024

Bodigrim commented Jan 23, 2024

hasufell commented Jan 24, 2024 •

edited

Loading

hasufell commented Jan 24, 2024

hasufell commented Jan 25, 2024

hasufell Jan 25, 2024

Fix W.splitFileName "/\\?/a:" #220

Fix W.splitFileName "/\\?/a:" #220

Conversation

Bodigrim commented Jan 21, 2024

hasufell Jan 23, 2024

Choose a reason for hiding this comment

Bodigrim Jan 23, 2024

Choose a reason for hiding this comment

hasufell Jan 24, 2024

Choose a reason for hiding this comment

hasufell commented Jan 23, 2024 • edited Loading

hasufell Jan 23, 2024

Choose a reason for hiding this comment

Bodigrim Jan 23, 2024

Choose a reason for hiding this comment

Bodigrim commented Jan 23, 2024

hasufell commented Jan 24, 2024 • edited Loading

hasufell commented Jan 24, 2024

hasufell commented Jan 25, 2024

hasufell Jan 25, 2024

Choose a reason for hiding this comment

hasufell commented Jan 23, 2024 •

edited

Loading

hasufell commented Jan 24, 2024 •

edited

Loading