Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Handle root path correctly #3354

Merged

Conversation

InversionSpaces
Copy link
Contributor

@InversionSpaces InversionSpaces commented Nov 30, 2023

Drop trailing separators in Path creation only if it is not the only thing in the path.

Fixes #3353

Comment on lines 101 to 110
/*
* NOTE: It seems like scala js does not rewrite this to a loop?
* Call stack limit is reached if there are too many separators.
*/
@tailrec
private[file] def dropTrailingSep(path: String): String =
// Drop separator only if there is something else left
if (path.endsWith(sep) && path.length > sep.length)
dropTrailingSep(path.dropRight(sep.length))
else path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does actually 🤔 Here's what it's compiled to:

$c_Lfs2_io_file_Path$.prototype.dropTrailingSep__T__T = (function(path) {
  while (true) {
    if ($f_T__endsWith__T__Z($n(path), this.Lfs2_io_file_Path$__f_sep)) {
      var this$1 = $n(path);
      var this$2 = $n(this.Lfs2_io_file_Path$__f_sep);
      var $$x2 = (this$1.length > this$2.length)
    } else {
      var $$x2 = false
    };
    if ($$x2) {
      var $$x1 = $m_sc_StringOps$();
      var x = path;
      var this$4 = $n(this.Lfs2_io_file_Path$__f_sep);
      path = $n($$x1).dropRight$extension__T__I__T(x, this$4.length)
    } else {
      break
    }
  };
  return path
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes for me. How were you getting the failure?

assertEquals(Path("/".repeat(Int.MaxValue/8)), Path("/"))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange, I wrote this comment when I used IndexedSeqView instead of String.

Comment on lines 47 to 48
forAll((sepLength: Int) =>
if (sepLength >= 0 && sepLength <= 100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These if guards are probably skipping most/all of the generated values. Instead you should do something like this.

Suggested change
forAll((sepLength: Int) =>
if (sepLength >= 0 && sepLength <= 100)
forAll(Gen.choose(0, 100))((sepLength: Int) =>

@tailrec
private[file] def dropTrailingSep(path: String): String =
// Drop separator only if there is something else left
if (path.endsWith(sep) && path.length > sep.length)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like to Officially:tm: support Windows 🤮 but its path can look like C:\ which I think would get re-written to C:. I have no idea if that's good or bad 😅

Copy link
Contributor

@BalmungSan BalmungSan Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing some tests on Powershell

PS C:\Users\PC> cd c:
PS C:\Users\PC> cd C:
PS C:\Users\PC> cd C:\
PS C:\>

Thus, it seems they are not equivalent.


IMHO, it is best to avoid this specific Path modification, also it may be good to add a unit test verifying it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used node API exclusively

Comment on lines 54 to 59
forAll(sepLengthGen)((sepLength: Int) =>
assertEquals(
Path("/".repeat(sepLength)).toString,
"/"
)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a minute, is this test passing even when sepLength = 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it should not. After investigation I found out that scalacheck is mishandled in this suite. Tried to fix that

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@armanbilge armanbilge merged commit 8badc91 into typelevel:main Dec 6, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Root Path is incorrectly handled in ScalaJS
3 participants