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

Allow multiple segments in Stringliterals #297

Merged

Conversation

pawelsadlo
Copy link
Contributor

@pawelsadlo pawelsadlo commented Sep 5, 2024

This PR adds support for multi-segment literal strings.
for example

val path = root / "foo/bar" 
val qux = "qux"
val path = root / "foo/bar" / "baz" / qux
val path = root / "foo/bar" / "baz/qux"

it also parses .. segments from literal as os.up enabling syntax like:

val path = root / "foo" / ".." / "bar" // equivalent to `root / "foo" / os.up / "bar"`
val path = root / "foo" /  "../bar" // equivalent to `root / "foo" / os.up / "bar"`

non-canonical paths used in literals result in compile-time errors, suggesting usage of canonical paths or removing path segment, eg.

val path = root / "foo/./bar" //suggests "foo/bar"
val path = root / "foo//bar" //suggests "foo/bar"
val path = root / "//foo//bar/./baz" //suggests "foo/bar/baz"

val path = root / ""  //suggests removing the segment
val path = root / "." //suggests removing the segment
val path = root / "/" //suggests removing the segment
val path = root / "//" //suggests removing the segment
val path = root / "/./" //suggests removing the segment

Note:
Its not usable in os-Lib itself, due to the fact that it would lead to macro expansion in the same compilation unit as its definition.

@lihaoyi
there is a little bit of hacking involved:

  1. There is a default implicit conversion not being a macro to be used within os library, without this we would have to add a submodule and split the whole project, I'm not even sure if its doable.
  2. Needed to turn off acyclic in Path and particular Macro files (also needed to mock acyclic.skipped in case of scala 3).
  3. Needed to provide another implicit conversion in ViewBoundImplicit trait because macros turn out to be not avaliable as implicit views, this is needed for ArrayPathChunk and SeqPathChunk to work.

@pawelsadlo
Copy link
Contributor Author

@lihaoyi could you rerun checks?

@pawelsadlo
Copy link
Contributor Author

pawelsadlo commented Sep 6, 2024

@lihaoyi
could you take a look at checks after [924a02e]?
did you encounter something like this before?

adding more tests[5a49ac2] succeed in (ubuntu-latest, 17) and almost nothing changed since then (i have only added binary compatibility shims)

After last failed workflow I have changed macro to make StringPathChunk and ArrayPathChunk instead of SubPathChunk, but I dont know why SubPathChunk would fail, expecially only on particular environment and not others

@lihaoyi
Copy link
Member

lihaoyi commented Sep 6, 2024

@pawelsadlo will take a look!

@pawelsadlo
Copy link
Contributor Author

@lihaoyi it succeed now, so I dont know if we need to examine it, anyway could do a review?

os/src/Path.scala Outdated Show resolved Hide resolved
@lihaoyi
Copy link
Member

lihaoyi commented Sep 6, 2024

Will take a look tomorrow

build.sc Outdated Show resolved Hide resolved
os/test/src/PathTests.scala Outdated Show resolved Hide resolved
import java.net.URI
object PathTests extends TestSuite {
private def nonValidPathSegment(chars: String) = s"[$chars] is not a valid path segment."
Copy link
Member

@lihaoyi lihaoyi Sep 7, 2024

Choose a reason for hiding this comment

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

I think we can do a more user-focused error message here, something like

literal path sequence <entire-literal> used in OS-Lib must be in a canonical form, please use <literal-without-leading-slashes> instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lihaoyi
I have provided a NonCanonicalLiteral exception for this case
actually there is also an edge case when sanitizing literal results in empty path, for example //, /., . literals , in such cases i throw:

InvalidSegment(
      literal,
      s"Literal path sequence [$literal] doesn't affect path being formed, please remove it"
    )

If you have any better suggestions for error messages I can change them.

os/test/src/PathTests.scala Outdated Show resolved Hide resolved
@lihaoyi
Copy link
Member

lihaoyi commented Sep 7, 2024

@pawelsadlo looks great, left some comments. If you update the code and update the PR description to have the most up to date state of the PR I think we can merge it

allowing [..] in literal paths
adding literal dedicated compile-errors
moving its tests to os package
@pawelsadlo
Copy link
Contributor Author

@lihaoyi could you please rerun checks?

@pawelsadlo
Copy link
Contributor Author

@lihaoyi let me know if there are any suggestions left

@lihaoyi lihaoyi merged commit 4262d5c into com-lihaoyi:main Sep 10, 2024
9 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Sep 10, 2024

@pawelsadlo looks great, thanks! I merged it, I assume your bank details haven't changed so I'll pay out the bounty to the same account as last time

lihaoyi added a commit to com-lihaoyi/mill that referenced this pull request Sep 10, 2024
Introduced in com-lihaoyi/os-lib#297, these
allow conciseness when working with literal paths/path-segments while
limiting it to compile-time literals to avoid potential path traversal
issues that may arise from parsing dynamic paths
lihaoyi added a commit that referenced this pull request Sep 10, 2024
… segments (#301)

Since #297 landed, the old
error messages have become somewhat misleading. This attempts to make
them a bit more accurate, and suggest to users that literal string path
segments have more flexibility than dynamically computed ones
@lefou lefou added this to the 0.10.7 milestone Sep 10, 2024
majk-p added a commit to majk-p/os-lib that referenced this pull request Sep 13, 2024
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.

3 participants