nim-lang / RFCs

A repository for your Nim proposals.
137 stars 23 forks source link

allow `static string` as tail for `/` func in `std/paths` #490

Open scarf005 opened 2 years ago

scarf005 commented 2 years ago

Abstract

doAssert Path"/foo" / "bar" / "baz" == Path"/foo/bar/baz"

Motivation

Path"/foo" / "bar" / "baz"

is more convenient than

Path"/foo" / Path"bar" / Path"baz"

Description

thought it'd be better to create a RFC first before making PR, not sure if there would be a func crash (still a newbie in nim). the functionality can be achieved by changing

https://github.com/nim-lang/Nim/blob/31be01d78f98a2904725994fd30eae4232e5a47c/lib/std/paths.nim#L42-L53

to

func `/`(head: Path, tail: Path | string): Path {.inline.} =
  joinPath(head.string, tail.string).Path

Code Examples

code below is runnable as-is in nim 1.7.3.

import std/paths
from std/private/ospaths2 import joinPath

func `/`(head: Path, tail: Path | string): Path {.inline.} =
  joinPath(head.string, tail.string).Path

let foo = Path"/a/b"
let bar = Path"/a"
let baz = bar / Path"b"
let qux = bar / "b"

if isMainModule:
  doAssert foo == baz
  doAssert foo == qux

Backwards Compatibility

probably should have no problems.

Araq commented 2 years ago

That weakens type safety. Might not matter, it's too early to tell as there is no official release yet that has std / paths.

Vindaar commented 2 years ago

As an alternative something like this:

import std / paths
proc p(s: string{lit}): Path = Path(s)
doAssert typeof(Path"hello" / p"world") is Path

is a convenient way to work with paths while barely typing more (the AST based overloading is of course not needed, but it's maybe nice to restrict such a simple proc name to the most narrow use case).

Varriount commented 2 years ago

@Araq How does it weaken type safety?

Araq commented 2 years ago

Well I think something like Path"/usr" / stdin.readLine is supposed to be prevented by std / paths. Maybe I'm overthinking it.

scarf005 commented 2 years ago

i think it's okay to allow Path"/usr" / stdin.readLine. also python equivalent:

In [1]: from pathlib import Path
   ...: from sys import stdin
In [2]: Path('usr') / stdin.readline()
bin
Out[2]: PosixPath('usr/bin\n')
In [3]: Path('usr') / input()
bin
Out[3]: PosixPath('usr/bin')

readLine omits newline like python's input() so i guess newline won't be a problem.

Araq commented 2 years ago

What Python does wrt "type safety" (which it doesn't have) is completely irrelevant. It's interesting to see though what Rust and C++ support.

scarf005 commented 2 years ago

is 'reading from stdin' is type unsafe? because Path"/usr" / stdin.readLine feels type safe enough for me

Vindaar commented 2 years ago

is 'reading from stdin' is type unsafe? because Path"/usr" / stdin.readLine feels type safe enough for me

Yes, it is unsafe in the sense that via stdin you can get any string, which includes all sorts of strings which can not represent valid paths (this will be different based on OS etc of course, but certain character sets won't be sensible for a Path). Or in other words: if stdin reading of a Path by itself was safe, the distinction between string and Path would be dubious in the first place (there would still be some benefits in relation to different procedures only being defined for Path, but maybe File and vice versa).

scarf005 commented 2 years ago

which includes all sorts of strings which can not represent valid path

could something like this solve the safety issue?

func `/`(head: Path, tail: Path | string): Path {.inline.} =
  when tail is Path:
    joinPath(head.string, tail.string).Path
  else:
    let safeTail = # do safety check
    joinPath(head.string, safeTail.string).Path
Vindaar commented 2 years ago

which includes all sorts of strings which can not represent valid path

could something like this solve the safety issue?

func `/`(head: Path, tail: Path | string): Path {.inline.} =
  when tail is Path:
    joinPath(head.string, tail.string).Path
  else:
    let safeTail = # do safety check
    joinPath(head.string, safeTail.string).Path

Only partially. That safety check would happen at runtime. But at least for string literals the check can be done at compile time. Of course, whenever one deals with any runtime string that is computed to represent some path, the check will always happen at runtime (but even then there are different "levels of safety" at play here. A stdin based string is pretty much worst case, as it can literally be any string. On a "safer" level, a runtime string could be just some concatenation of CT proven "Path-safe" snippets for example.).

scarf005 commented 2 years ago

make sense, but wouldn't readLine problem apply to current implementation too?

Path"foo" / Path stdin.readLine
Araq commented 2 years ago

The idea is that you don't hack stuff together until it compiles but you make a concious decision to convert the string to Path.

Varriount commented 2 years ago

As a compromise, what if this only applied to string literals?

func `/`(head: Path, tail: Path | string{lit}): Path {.inline.} =
  joinPath(head.string, tail.string).Path

This allows the convenience of adding "fixed" string literals to Path values, while preventing dynamic values from being appended.

Araq commented 2 years ago

Yes, this idea has been brought up before.

scarf005 commented 2 years ago

could i get doc for string{lit}? I wasn't able to find it in the nim manual, and its syntax certainly isn't pragma

ringabout commented 2 years ago

could i get doc for string{lit}? I wasn't able to find it in the nim manual, and its syntax certainly isn't pragma

it is in the experimental documenation => https://nim-lang.org/docs/manual_experimental.html#term-rewriting-macros-parameter-constraints

ringabout commented 2 years ago

It seems that C++ support it

#include <iostream>
#include <filesystem>
#include <string>
using namespace std; 
namespace fs = std::filesystem;
int main() {
    fs::path p1 = "C:";
    string p2 = "Users";
    std::cout << p1 / p2;
}

ref https://en.cppreference.com/w/cpp/filesystem/path/append

mratsim commented 1 year ago

Abstract

doAssert Path"/foo" / "bar" / "baz" == Path"/foo/bar/baz"

Motivation

Path"/foo" / "bar" / "baz"

is more convenient than

Path"/foo" / Path"bar" / Path"baz"

Isn't Path("/foo" / "bar" / "baz") valid as well?

I agree that conversion from string to Path should be explicit, especially when user random OS/user input. That was the whole point of feu TaintedString

Varriount commented 1 year ago

Isn't Path("/foo" / "bar" / "baz") valid as well?

Why would it be, if string (or static[string]) doesn't have a / operator?

mratsim commented 1 year ago

Isn't Path("/foo" / "bar" / "baz") valid as well?

Why would it be, if string (or static[string]) doesn't have a / operator?

https://nim-lang.org/docs/os.html#%2F%2Cstring%2Cstring

proc /(head, tail: string): string {.noSideEffect, inline, ....}

or is it going away?