Upgrade deps

[mrkkrp]

This one is not green either.

[mrkkrp]

@NorfairKing are you planning to fix this one?

[NorfairKing]

Looks like a test failure. I'll get to it, but not soon.

[NorfairKing]

@mrkkrp Now it should be clearer why the test failed. I'm not sure what to do here.

[mrkkrp]

"." is a correct path value, right? I'd think the test should not fail for it?

[NorfairKing]

@mrkkrp "." is such a weird value. It goes against four of the validity constraints that are usually on Path Rel Dir. I guess the validity constraints could just have an exception for that particular value.

[mrkkrp]

I guess the validity constraints could just have an exception for that particular value.

This makes sense to me.

[NorfairKing]

Adding Path "." as an exceptionally valid path, only creates more test failures:

  test/ValidityTest.hs:222:39: 
  1) Parsing: Path Rel Dir Produces valid paths when it succeeds
       Falsifiable (after 251 tests and 2 shrinks):
         "."
       Violated: The path has a trailing path separator.
       Violated: System.FilePath considers the path valid.
       Violated: The path is not empty.
       Violated: The path can be identically parsed as a relative directory path.

  To rerun use: --match "/Parsing: Path Rel Dir/Produces valid paths when it succeeds/"

  src/Test/Validity/Property/Utils.hs:43:13: 
  2) Operations: (</>) produces a valid path on when creating valid absolute directory paths
       Falsifiable (after 1 test and 4 shrinks):
         ("/a/",".")
       'validate' reported this value to be invalid: 
       "/a/."
       with explanation
       Violated: The path has a trailing path separator.
       Violated: The path can be identically parsed as an absolute directory path.

  To rerun use: --match "/Operations: (</>)/produces a valid path on when creating valid absolute directory paths/"

  src/Test/Validity/Property/Utils.hs:43:13: 
  3) Operations: (</>) produces a valid path on when creating valid relative directory paths
       Falsifiable (after 1 test and 1 shrink):
         ("a/",".")
       'validate' reported this value to be invalid: 
       "a/."
       with explanation
       Violated: The path has a trailing path separator.
       Violated: The path can be identically parsed as a relative directory path.

  To rerun use: --match "/Operations: (</>)/produces a valid path on when creating valid relative directory paths/"

  test/ValidityTest.hs:50:18: 
  4) Operations: filename filename ($(mkRelDir parent) </> $(mkRelFile filename)) == filename $(mkRelFile filename)
       Falsifiable (after 11 tests and 4 shrinks):
         "."
         "a"
       expected: "a"
        but got: ".a"

Adding Path "./" as an exceptionally valid value produces these new failures:

  src/Test/Validity/Property/Utils.hs:43:13: 
  2) Operations: (</>) produces a valid path on when creating valid absolute directory paths
       Falsifiable (after 224 tests and 4 shrinks):
         ("/a/","./")
       'validate' reported this value to be invalid: 
       "/a/./"
       with explanation
       Violated: The path can be identically parsed as an absolute directory path.

  To rerun use: --match "/Operations: (</>)/produces a valid path on when creating valid absolute directory paths/"

  src/Test/Validity/Property/Utils.hs:43:13: 
  3) Operations: (</>) produces a valid path on when creating valid relative file paths
       Falsifiable (after 57 tests and 5 shrinks):
         ("./","a")
       'validate' reported this value to be invalid: 
       "./a"
       with explanation
       Violated: The path can be identically parsed as a relative file path.

  To rerun use: --match "/Operations: (</>)/produces a valid path on when creating valid relative file paths/"

  src/Test/Validity/Property/Utils.hs:43:13: 
  4) Operations: (</>) produces a valid path on when creating valid relative directory paths
       Falsifiable (after 57 tests and 4 shrinks):
         ("./","a/")
       'validate' reported this value to be invalid: 
       "./a/"
       with explanation
       Violated: The path can be identically parsed as a relative directory path.

So it seems that there are many things wrong with the library at the moment, but they hadn't been found up until now.

[NorfairKing]

@chrisdone what do you think?

[chrisdone]

@NorfairKing The "." path is supposed to be invalid, due to all the inconsistencies and confusion it introduces when manipulating paths. There was an issue in which I disabled it in the parser.

But maybe one of the other maintainers argued for its favor in the past and it crept inside, but it would be surprising to me as it goes against the library's "no surprises" goals.

[chrisdone]

Looks like it did creep in after all.

I said at the time that I didn't like it, but I don't know why I didn't push back on the parsing "." as "" thing. The whole thing makes me uncomfortable.

Does anyone else still think this is a good idea?

[NorfairKing]

I'm okay with parsing . as ``, but then all the functions have to work correctly with it, and they don't. I hope that it's at least clear now that not property testing this code is not an option.

[NorfairKing]

So we have two options now:

1. Revert the PR that allowed for "."
2. Follow a plan like this:
   • Allow for "." by
   • Strictly defining validity constraints on paths, clearly.
   • Ensuring that every function in the library has nice property tests
   • Fixing the functions to make sure that there can never be invalid paths.

I'd prefer the second option, but would need help with the actual work.

@harendra-kumar What do you think? @mrkkrp and you?

[NorfairKing]

I've also added some tests for dirname that weren't there yet, for some reason. Here's another fun problem:

Path.dirname (Path "/")
"./"

[chrisdone]

I'd say: try out (2) and if you get stuck on weird issues while exploring the results of ., do (1).

[harendra-kumar]

@NorfairKing I would not mind reverting the PR. But I have not seen any fundamental problem being articulated about the change. I only see some test failure which has not been investigated. Don't we suspect that the tests might be wrong? I see this in the test output above:

src/Test/Validity/Property/Utils.hs:43:13: 
  2) Operations: (</>) produces a valid path on when creating valid absolute directory paths
       Falsifiable (after 224 tests and 4 shrinks):
         ("/a/","./")
       'validate' reported this value to be invalid: 
       "/a/./"
       with explanation
       Violated: The path can be identically parsed as an absolute directory path.

I cannot reproduce this problem, in my repl I get perfect results:

*Path> $(mkAbsDir "/a/") </> $(mkRelDir "./")
"/a/"

Can we get some simple test case that proves that there is definitely a problem in the library?

[harendra-kumar]

This is by design and correct:

*Path> dirname $(mkAbsDir "/")
"./"

dirname is supposed to give the name part of the dir which is a relative directory. For / that relative directory is just ./. If you put the components together again you will get a perfect original path:

*Path> $(mkAbsDir "/") </> $(mkRelDir "./")
"/"

In any POSIX system you can see that "/./" is equivalent to "/".

[NorfairKing]

@harendra-kumar Try checking out this PR locally and running the tests (maybe with --qc-max-success=10000 or so).

In particular, what do you do about this:

Failures:

  test/ValidityTest.hs:222:39: 

  1) Parsing: Path Rel Dir Produces valid paths when it succeeds

       Falsifiable (after 95 tests):

         "."

       Violated: The path has a trailing path separator.

       Violated: System.FilePath considers the path valid.

       Violated: The path is not empty.

       Violated: The path can be identically parsed as a relative directory path.

[harendra-kumar]

Does anyone else still think this is a good idea?

@chrisdone I do think it is a good idea. I spent a lot of time thinking about it and getting it right. It was not easy. But that is no reason to think that it is a good idea. After a lot of effort everything fell into place nicely, the abstractions were lawful and the change was not complicated. It resolved corner cases and exceptions in the library and made it more elegant. So I think it is a very good thing, but I might be biased about that judgement as I authored the change. I added test cases for whatever cases I could think of. But I might have missed something. Before commenting further, I would like to know specifically what is it that is such a big issue with this change that warrants rethinking it.

If it is found to be a source of bugs I would support reverting it. However, it may not be easy to revert it as I guess that may require changes to some APIs as well. Moreover, it has already been released, it will be a breaking change and may require deprecation. Sorry for the trouble.

[harendra-kumar]

In particular, what do you do about this:

I do not see any problem, are you sure the tests are doing the right thing?

*Path> $(mkRelDir ".")
"./"
*Path> $(mkRelDir "./")
"./"

The test output says:

       Violated: The path has a trailing path separator.

       Violated: System.FilePath considers the path valid.

       Violated: The path is not empty.

       Violated: The path can be identically parsed as a relative directory path.

My repl test above shows that:

• the path has a trailing separator
• it is not empty
• it can be parsed to itself
• I am not sure about the System.FilePath considering it a valid path. But that should not be a criterion anyway if we know that this is a correct POSIX path.

I am wondering if you are you taking out values from the internal Path constructor for your testing? Because Path may definitely have an empty string, that's how it was designed, and an empty value of course will not have a trailing separator and it cannot be parsed. The right thing to do would be to use toFilePath and use that value for your tests. toFilePath handles the conversion from "Path" to the actual path string.

[harendra-kumar]

I took a quick look at this change, my suspicion was right. The change is indeed asserting invariants based on the internal representation of path. It seems to me there is absolutely nothing broken about the library. We should not be asserting invariants based on the internal representation of the path, we should rather use exposed APIs i.e. convert it using toFilePath before asserting anything.

[NorfairKing]

@harendra-kumar We are trying to achieve the same thing: to keep your changes in the library. In order to do that, we need to make sure that these changes are well-tested and that the tests pass.

Part of the validity-based testing entails the precise description of the invariants of a Path b t value. It could be that the validate functions have been implemented wrong, I'm also only human. In that case, this PR serves to fix that.

Given that you have the most experience with the . change, could you please have a close look at the validity constraints and correct them? Then we can have a look at any failing test cases that remain afterwards.

Remember, it is important that the correct functioning of your tests do not assume the correct functioning of the library. So you cannot use toFilePath in the validate functions.

[NorfairKing]

@harendra-kumar :

Remember, it is important that the correct functioning of your tests do not assume the correct functioning of the library. So you cannot use toFilePath in the validate functions.

[harendra-kumar]

@NorfairKing I tried toFilePath in the assertions, you can revert it if you want. But there may be a problem somewhere else in the tests that may have to be fixed because the tests do not finish after this change.

Remember, it is important that the correct functioning of your tests do not assume the correct functioning of the library. So you cannot use toFilePath in the validate functions.

We do not assume correct functioning of the library. I do not think we are assuming correct functioning of toFilePath here. We are checking whether what toFilePath returns is correct as per our assumptions. If we want to unit test the library by examining the internal values of Path then we have to assert the internal invariants, in that case we can exclude the empty Path case perhaps, not sure if there would be any more things to take care of.

[NorfairKing]

We do not assume correct functioning of the library. I do not think we are assuming correct functioning of toFilePath here.

toFilePath is part of the library.

Remember that the Validity constraints are to define what a valid filepath means. The testing happens using those, after that.

in that case we can exclude the empty Path case perhaps, not sure if there would be any more things to take care of.

Yes, that's what I tried in https://github.com/commercialhaskell/path/pull/147#issuecomment-529580361 . It would be great if you could try that out and then take care of the things that come up.

[harendra-kumar]

toFilePath is part of the library.

But we have no assumption that it is correct. We are testing that the output it generates is correct. If its implementation is not correct our tests will fail. If we are doing a blackbox testing we cannot look inside the internal implementation. If we are doing a whitebox testing then we have to be prepared to honor the invariants as per the internal implementation.

Anyway, I pushed an alternative fix excluding the empty path in relative dir case. Again the the tests hang, maybe they are just taking too long. Please take a look if something needs to be fixed in the tests.

[NorfairKing]

But we have no assumption that it is correct. We are testing that the output it generates is correct. If its implementation is not correct our tests will fail. If we are doing a blackbox testing we cannot look inside the internal implementation. If we are doing a whitebox testing then we have to be prepared to honor the invariants as per the internal implementation.

The point is that you don't know what the invariants are, without the Validity instance. That means that you have to implement the vaildity instance by just looking at the contents of the string inside.

[NorfairKing]

Anyway, I pushed an alternative fix excluding the empty path in relative dir case. Again the the tests hang, maybe they are just taking too long. Please take a look if something needs to be fixed in the tests.

Hanging tests usually mean that generators are taking too long to generate something. You may have created an infinite loop. Best to look at which test in particular hangs.

[chrisdone]

@harendra-kumar Coming back to this fresh, my complaint is that "." is special. It adds special cases to the code, and adds special cases to developers' heads that are apparently unintuitive (even to contributors to the library :thinking: -- see above @NorfairKing @mrkkrp), which was the primary complaint I had with other path libraries while creating this one. :point_up: A good rule of thumb could have been: if you can't mkdir x then x probably shouldn't be accepted as a path.

But you're right, it's released now. We're stuck with either explaining to everyone how this works or deprecating. I'm fine with saying that I made a mistake on this, deprecations aren't the end of the world, but I'm (obviously) not an active maintainer, so I accept that you more active guys get to decide the direction of the library (and that what you've come up with does have laws and reasoning). That's open source! :+1:

[chrisdone]

Out of curiosity, I list-ping everyone who's committed to the library in the past to let them weigh in on their unbiased thoughts on this topic.

@chrisdone @mrkkrp @sjakobi @harendra-kumar @NorfairKing @joehillen @kraai @waddlaw @varosi @snoyberg @rkoeninger @rimmington @mgsloan @magthe @hesiod @glasserc @felixonmars @chshersh @chris-martin @brandon-leapyear @bergmark @3noch

[NorfairKing]

@harendra-kumar Have you had a chance to look at why the tests hang?

[harendra-kumar]

@NorfairKing I have no idea how the validity stuff works and I currently do not have time to look into it. The point of my quick commit was just to provide an idea how this can be fixed, not to actively work on this particular PR. I do not think there is any serious problem with the library, most likely the tests need to be fixed. You can please go ahead and fix it.

[mrkkrp]

What's the status of this one? If validity tests are blocking moving forward, maybe let's just disable or drop the failing tests and call it a day.

[NorfairKing]

@mrkkrp We're all a bit busy to actually move this forward. I find "let's not test the code then" a bit of a silly solution but I have nothing else either.

[mrkkrp]

It's a compromise. From the moment you added the validity tests to the library they have been of great value and helped with testing. If I understand correctly the new tests which are failing now were also added by you voluntarily. It seems fair to me that either they should be fixed (all things considered, it looks like you're the most appropriate person to do that) or disable until you have enough time for a proper fix.

[NorfairKing]

I'm inclined to agree to do this, but I'm worried that there are actual safety-problems with the version in this PR and we won't know until we get the tests to pass. I wish I had the bandwidth to take this on.

[NorfairKing]

I just had another closer look at this, and it "just works" for me :hmm:

path> test (suite: validity-test)

Parsing: Path Abs Dir
  Produces valid paths when it succeeds
Parsing: Path Rel Dir
  Produces valid paths when it succeeds
Parsing: Path Abs File
  Produces valid paths when it succeeds
Parsing: Path Rel File
  Produces valid paths when it succeeds
Operations: (</>)
  produces a valid path on when creating valid absolute file paths
  produces a valid path on when creating valid absolute directory paths
  produces a valid path on when creating valid relative file paths
  produces a valid path on when creating valid relative directory paths
Operations: stripProperPrefix
  stripProperPrefix parent (parent </> child) = child
  stripProperPrefix parent (parent </> child) = child
  stripProperPrefix parent (parent </> child) = child
  stripProperPrefix parent (parent </> child) = child
  produces a valid path on when passed a valid absolute file paths
  produces a valid path on when passed a valid absolute directory paths
  produces a valid path on when passed a valid relative file paths
  produces a valid path on when passed a valid relative directory paths
Operations: isProperPrefixOf
  isProperPrefixOf parent (parent </> child)
  isProperPrefixOf parent (parent </> child)
  isProperPrefixOf parent (parent </> child)
  isProperPrefixOf parent (parent </> child)
Operations: parent
  produces a valid path on when passed a valid file path
  produces a valid path on when passed a valid directory path
Operations: filename
  filename ($(mkAbsDir parent) </> $(mkRelFile filename)) == filename $(mkRelFile filename)
  filename ($(mkRelDir parent) </> $(mkRelFile filename)) == filename $(mkRelFile filename)
  produces a valid path on when passed a valid absolute path
  produces a valid path on when passed a valid relative path
Extensions
  if addExtension a b succeeds then parseRelFile b succeeds - 1
  if addExtension a b succeeds then parseRelFile b succeeds - 2
  (toFilePath . fromJust . addExtension ext) file == toFilePath a ++ b
  splitExtension output joins to result in the original file
  splitExtension generates a valid filename and valid extension
  splitExtension >=> uncurry addExtension . swap == return
  uncurry addExtension . swap >=> splitExtension == return
  fileExtension == (fmap snd) . splitExtension
  flip addExtension file >=> fileExtension == return
  (fileExtension >=> flip replaceExtension file) file == return file

Finished in 0.4542 seconds
36 examples, 0 failures

path> Test suite validity-test passed

So now I can't even reproduce this problem. @harendra-kumar What was the issue with the hanging?

[harendra-kumar]

@NorfairKing I ran it again now, I waited for 10 minutes before killing it, with 300% CPU on a macbook using cabal test --allow-newer. Last time I was using stack test instead of cabal, so it is not a stack/cabal issue.

Does the CI include MacOS?

[NorfairKing]

Oh! Do you think the problem may be os-related? I tried again on my other machine and that seems to hang. Now I've got something to hone in on. I think this will be reproducible:

 stack test --ta='--seed=42'

Thanks for the input. I'll have another look.

[NorfairKing]

@harendra-kumar I just pushed some changes to the test suite. Could you have another look? I think I found an infinte loop in dirname on ./.

[NorfairKing]

Scratch that, not an infinite loop in dirname, but for some reason this test suite loops infinitely.

[NorfairKing]

I think I have an idea what could cause this! It could be infinite shrinking.

[NorfairKing]

Jup, it was indeed infinte shrinking. I'll add tests for the shrinking functions too.

[NorfairKing]

@harendra-kumar Now every test is fixed except dirname:

  test/ValidityTest.hs:68:27: 
  1) Operations.dirname dirname parent </> $(mkRelDir dirname)) == dirname $(mkRelDir dirname) Path Abs Dir
       Falsifiable (after 1 test and 67 shrinks):
         "/aa/"
         "./"
       expected: "./"
        but got: "aa/"

  To rerun use: --match "/Operations/dirname/dirname parent </> $(mkRelDir dirname)) == dirname $(mkRelDir dirname) Path Abs Dir/"

  test/ValidityTest.hs:68:27: 
  2) Operations.dirname dirname parent </> $(mkRelDir dirname)) == dirname $(mkRelDir dirname) Path Rel Dir
       Falsifiable (after 4 tests and 100 shrinks):
         "aa/"
         "./"
       expected: "./"
        but got: "aa/"

Can you check that 1. the test correctly asserts something which isn't the case and then 2. fix the code, please?

[NorfairKing]

@mrkkrp this seems to work. @chrisdone I'd appreciate a review.

[NorfairKing]

@mrkkrp I think this is ready to merge.