search

haskell / filepath

Haskell FilePath core library
BSD 3-Clause "New" or "Revised" License
66 stars 33 forks source link

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

Closed Bodigrim closed 5 months ago

Bodigrim commented 5 months ago

Fixes #219.

hasufell commented 5 months ago

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.

Bodigrim commented 5 months ago

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

hasufell commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

The bug was in hasPenultimateColon