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

Implement `Display` for `Path` and `PathBuf` #49868

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@withoutboats
Contributor

withoutboats commented Apr 11, 2018

In RFC #474, the issue of how to handle Displaying a Path was left as an open question. The problem is that a Path may contain non-UTF-8 data on most platforms. In the implementation of the RFC, a display method was added, which returns an adapter that implements Display by replacing non-UTF8 data with a unicode replacement character.

Though I can't find a record of the discussion around this issue, I believe there were two primary reasons not to just implement this behavior as the Display impl of Path:

  1. The adapter allocated in the non-UTF8 case, and Rust as a rule tries to avoid allocations that are not explicit in code.
  2. The user may prefer an alternative solution than using the unicode replacement character for handling non-UTF8 data.

In my view, the choice to provide an adapter rather than implement Display has had a high cost in terms of user experience:

  • I almost never remember that I need an adapter, forcing me to go back and edit my code after compiling it and getting an error.
  • It makes my code more noisy to have the display adapter; this detail is rarely important.
  • It is extremely uncommon to actually do something other than call the display adapter when trying to display a path (I have never wanted anything else myself).
  • For new users, it is Yet Another Compiler Error that they have to figure out how to solve, contributing to the sense that Rust nags and obstructs rather than assists & guides.

Therefore, I think time has shown that this has been a detriment to user experience, rather than a helpful reminder. That leaves only the first reason not to implement this: implicit allocations. That problem was happily resolved in June 2017: #42613 provided an alternative implementation which efficiently avoids allocations.

Given that, I think it is time that we implement Display for both Path and PathBuf and deprecate the Path::display method.

r? @alexcrichton
cc @rust-lang/libs

Deprecate `Path::display`, implement `Display` for `Path`
In [RFC #474][rfc474], the issue of how to handle Displaying a
Path was left as an open question. The problem is that a Path
may contain non-UTF-8 data on most platforms. In the
implementation of the RFC, a `display` method was added, which
returns an adapter that implements `Display` by replacing
non-UTF8 data with a unicode replacement character.

Though I can't find a record of the discussion around this
issue, I believe there were two primary reasons not to just
implement this behavior as the `Display` impl of `Path`:

1. The adapter allocated in the non-UTF8 case, and Rust as a
   rule tries to avoid allocations that are not explicit in code.
2. The user may prefer an alternative solution than using the
   unicode replacement character for handling non-UTF8 data.

In my view, the choice to provide an adapter rather than
implement Display has had a high cost in terms of user experience:

* I almost never remember that I need an adapter, forcing me to
  go back and edit my code after compiling it and getting an error.
* It makes my code more noisy to have the display adapter; this
  detail is rarely important.
* It is extremely uncommon to actually do something other than call
  the display adapter when trying to display a path (I have never
  wanted anything else myself).
* For new users, it is Yet Another Compiler Error that they have to
  figure out how to solve, contributing to the sense that Rust nags
  nags and obstructs rather than assists & guides.

Therefore, I think time has shown that this has been a detriment to
user experience, rather than a helpful reminder. That leaves only
the first reason not to implement this: implicit allocations. That
problem was happily resolved in June 2017: #42613 provided an
alternative implementation which efficiently avoids allocations.

Given that, I think it is time that we implement `Display` for both
`Path` and `PathBuf` and deprecate the `Path::display` method.

r? @alexcrichton
cc @rust-lang/libs

[rfc474]: https://github.com/rust-lang/rfcs/blob/master/text/0474-path-reform.md)
@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Apr 11, 2018

Probably checkboxes are justified for this:

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented Apr 11, 2018

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Apr 11, 2018

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:46] configure: rust.quiet-tests     := True
---
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/lib.rs:28:74
[00:05:04]    |
[00:05:04] 28 |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]    |                                                                          ^^^^^^^
[00:05:04]    |
[00:05:04]   ::: tools/tidy/src/cargo.rs:26:18
[00:05:04]    |
[00:05:04] 26 |     for entry in t!(path.read_dir(), path).map(|e| t!(e)) {
[00:05:04]    |                  ------------------------- in this macro invocation
[00:05:04]    |
[00:05:04]    = note: `-D deprecated` implied by `-D warnings`
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/lib.rs:28:74
[00:05:04]    |
[00:05:04] 28 |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]    |                                                                          ^^^^^^^
[00:05:04] ...
[00:05:04] 90 |     for entry in t!(fs::read_dir(path), path) {
[00:05:04]    |                  ---------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/lib.rs:28:74
[00:05:04]    |
[00:05:04] 28 |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]    |                                                                          ^^^^^^^
[00:05:04]    |
[00:05:04]   ::: tools/tidy/src/bins.rs:50:24
[00:05:04]    |
[00:05:04] 50 |         let metadata = t!(fs::symlink_metadata(&file), &file);
[00:05:04]    |                        -------------------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/bins.rs:65:73
[00:05:04]    |
[00:05:04] 65 |                 tidy_error!(bad, "binary checked into source: {}", file.display());
[00:05:04]    |                                                                         ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/style.rs:123:12
[00:05:04]     |
[00:05:04] 123 |         t!(t!(File::open(file), file).read_to_string(&mut contents));
[00:05:04]     |            -------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:126:53
[00:05:04]     |
[00:05:04] 126 |             tidy_error!(bad, "{}: empty file", file.display());
[00:05:04]     |                                                     ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:136:52
[00:05:04]     |
[00:05:04] 136 |                 tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg);
[00:05:04]     |                                                    ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:172:60
[00:05:04]     |
[00:05:04] 172 |             tidy_error!(bad, "{}: incorrect license", file.display());
[00:05:04]     |                                                            ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:175:72
[00:05:04]     |
[00:05:04] 175 |             0 => tidy_error!(bad, "{}: missing trailing newline", file.display()),
[00:05:04]     |                                                                        ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:177:79
[00:05:04]     |
[00:05:04] 177 |             n => tidy_error!(bad, "{}: too many trailing newlines ({})", file.display(), n),
[00:05:04]     |                                                                               ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/errors.rs:84:48
[00:05:04]    |
[00:05:04] 84 |             tidy_error!(bad, "{}:{}: {}", file.display(), line_num, line);
[00:05:04]    |                                                ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/features.rs:110:12
[00:05:04]     |
[00:05:04] 110 |         t!(t!(File::open(&file), &file).read_to_string(&mut contents));
[00:05:04]     |            ---------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/features.rs:114:52
[00:05:04]     |
[00:05:04] 114 |                 tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg);
[00:05:04]     |                                                    ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/features.rs:293:50
[00:05:04]     |
[00:05:04] 293 |                                             file.display(),
[00:05:04]     |                                                  ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/features.rs:305:56
[00:05:04]     |
[00:05:04] 305 |                     tidy_error!(bad, "{}:{}: {}", file.display(), line, msg);
[00:05:04]     |                                                        ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/features.rs:326:12
[00:05:04]     |
[00:05:04] 326 |         t!(t!(File::open(&file), &file).read_to_string(&mut contents));
[00:05:04]     |            ---------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/cargo.rs:96:55
[00:05:04]    |
[00:05:04] 96 |                               depends on it", libfile.display(), krate);
[00:05:04]    |                                                       ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/pal.rs:112:8
[00:05:04]     |
[00:05:04] 112 |     t!(t!(File::open(file), file).read_to_string(contents));
[00:05:04]     |        -------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/pal.rs:130:67
[00:05:04]     |
[00:05:04] 130 |         tidy_error!(bad, "{}:{}: platform-specific cfg: {}", file.display(), line, cfg);
[00:05:04]     |                                                                   ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/deps.rs:229:42
[00:05:04]     |
[00:05:04] 229 |         panic!("{} does not exist", path.display());
[00:05:04]     |                                          ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/deps.rs:241:64
[00:05:04]     |
[00:05:04] 241 |             println!("invalid license {} in {}", license, path.display());
[00:05:04]     |                                                                ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/deps.rs:248:43
[00:05:04]     |
[00:05:04] 248 |         println!("no license in {}", path.display());
[00:05:04]     |                                           ^^^^^^^
[00:05:04]
[00:05:04] error: aborting due to 22 previous errors
[00:05:04]
[00:05:04] error: Could not compile `tidy`.
[00:05:04]
[00:05:04] Caused by:
[00:05:04]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name tidy tools/tidy/src/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=ec0ed652cc88252d -C extra-filename=-ec0ed652cc88252d --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/release/deps --extern serde_derive=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/release/deps/libserde_derive-6fd95ab87191bb85.so --extern serde_json=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps/libserde_json-b3e872b56e3c8f47.rlib --extern serde=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps/libserde-92847b096fc60ebd.rlib` (exit code: 101)
[00:05:04] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/tidy/Cargo.toml" "--features" "" "--message-format" "json"
[00:05:04] expected success, got: exit code: 101
[00:05:04] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:04] Build completed unsuccessfully in 0:01:50
[00:05:04] make: *** [tidy] Error 1
[00:05:04] Makefile:79: recipe for target 'tidy' failed
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:03b47ecb:start=1523441936035098256,finish=1523441936041781453,duration=6683197
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:1c787166
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:1c787166:start=1523441936047411082,finish=1523441936053841006,duration=6429924
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03634257
$ dmesg | grep -i kill
[   11.039019] init: failsafe main process (1092) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Apr 11, 2018

So what I've learned from the logs is that deprecating this is going to cause a lot of warnings. Possibly we should just provide the new API without deprecation?

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Apr 11, 2018

Strong 👍 from me on this. I work with paths a lot, and the number of times I've benefited from the fact that display() is implemented as an adapter rather on the type itself is exactly zero. :-)

I feel like deprecating the display adapter long term is a fine thing to do, but deprecating it now (or even too soon after stabilizing the Display impl), would make the transition much more annoying than it needs to be.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Apr 11, 2018

In general, we shouldn’t emit deprecation warnings in Nightly before the recommended replacement is available at least in the Stable channel.

But I’m not convinced we should mako this specific change.

@rfcbot concern correctness

I believe that the reason not to implement Display is that, since std::fmt is Unicode-based, an implementation must be lossy: its behavior cannot be relied on to later go back to &Path or PathBuf. The concern was that this could cause bugs that would be hard to diagnose because they only occur with non-Unicode paths.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Apr 11, 2018

@SimonSapin

It seems overwhelmingly more common to want to display paths for user consumptions than to try to use the display impl to "round trip" them. I also suspect that these issues would not be hard to debug - they seem very likely to appear as some invalid path which contains the unicode replacement character (I'd agree with the contention that they likely would not be caught in testing unless you're doing fuzz testing with randomly generated valid paths.) And I'm not convinced that forcing users to go .display() is all that likely to get them to realize and respond to the correctness issue when they are roundtripping.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Apr 11, 2018

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:49] configure: rust.quiet-tests     := True
---
[00:04:58] tidy error: /checkout/src/libstd/path.rs:1496: mismatches to previous in: ["since"]
[00:04:58] tidy error: /checkout/src/libstd/path.rs:1503: mismatches to previous in: ["since"]
[00:04:58] tidy error: /checkout/src/libstd/path.rs:2490: mismatches to previous in: ["since"]
[00:04:58] tidy error: /checkout/src/libstd/path.rs:2497: mismatches to previous in: ["since"]
[00:04:59] some tidy checks failed
[00:04:59]
[00:04:59]
[00:04:59] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:59] expected success, got: exit code: 1
[00:04:59]
[00:04:59]
[00:04:59] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:59] Build completed unsuccessfully in 0:01:55
[00:04:59] Makefile:79: recipe for target 'tidy' failed
[00:04:59] make: *** [tidy] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:104ce914:start=1523452151645583261,finish=1523452151653736476,duration=8153215
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:0526cb85
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:0526cb85:start=1523452151661886551,finish=1523452151670246274,duration=8359723
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0b0abae4
$ dmesg | grep -i kill
[   10.409696] init: failsafe main process (1092) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Apr 11, 2018

@SimonSapin An interesting case where this comes up easily is error messages. Namely, I'm pretty sure I almost always use path.display() when formatting an error message with a path, even though that is actually technically incorrect, and could show a file path to an end user that does not actually correspond to a path on their system. The Unicode replacement character would be a tipoff I guess when that happens.

My point here is that, even with the adapter method, I'm already doing the technically incorrect thing, largely because it seems border line impossible to do the right thing. Namely, how am I supposed to print an error message that might not be valid UTF-8? Our error messages are irrevocably tied to valid UTF-8 because our formatting infrastructure is tied to valid UTF-8. I would need to seriously go out of my way to get this right. Now, I don't think this is a huge deal in and of itself, but the display method isn't really doing much for me here.

On the other hand, there are cases where a program wants to print a file path for non-error reasons, e.g., an ls-like program. When I write those sorts of programs, I do care enough to make sure the file paths are roundtripped losslessly when possible. And that code is not at all obvious to write. Here's what I've written in the past:

    #[cfg(unix)]
    fn write_path<P: AsRef<Path>>(&mut self, path: P) -> Result<()> {
        use std::os::unix::ffi::OsStrExt;
        let path = path.as_ref().as_os_str().as_bytes();
        self.wtr.write_all(path)
    }

    #[cfg(not(unix))]
    fn write_path<P: AsRef<Path>>(&mut self, path: P) -> Result<()> {
        let path = path.as_ref().to_string_lossy();
        self.wtr.write_all(path.as_bytes())
    }

In other words, I not only need to abandon our nice formatting infrastructure, but I need to write platform dependent code to do it.

I'm with @withoutboats on this one. While I appreciate the display() method as a possible road bump to notifying users of potential correctness problems, those problems are incredibly thorny, and I don't think the adapter method carries its weight because I'm not convinced that it actually nudges folks in the right direction.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 11, 2018

I definitely sympathize with the problems mentioned here, I run into them all the time myself as well. I feel, though, that we've got a lot of messaging to undo if we start down this road (which I'm not sure we want to do). For example the documentation for std::fmt states:

fmt::Display implementations assert that the type can be faithfully represented as a UTF-8 string at all times. It is not expected that all types implement the Display trait.

which directly implies that Display for Path cannot exist. We can of course change this wording, but I feel like that also has much larger ramifications. Working with Path is often brought up as either the worst or the best part of Rust. It's not great because it's so unergonomic and there's so many unwrap or Display gotchas. It's also sometimes the best because it has all these gotchas which have been bugs in real-world programs.

Along those lines, though, I would have a question of why stop here? If our goal is to make Path more easily usable why stop at Display for Path? Why not make parent return Path? Why not add Display for OsStr? etc/etc.

Is there a strong rationale for stopping here and not adding further conveniences?

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Apr 11, 2018

Is there a strong rationale for stopping here and not adding further conveniences?

To me this seems like a very strange way to frame the question! Every decision is a trade off & should be judged on its own merits. Why allow the display adapter at all - I believe most users use it thoughtlessly without question & don't actually consider whether its correct; why shouldn't we make them jump through more hoops to lossily display their Paths and potentially introduce errors? Why should Option, which parent returns, have an unwrap convenience when it can be the source of runtime panics? Etc etc, you can ask infinite questions in either direction on the line between Coq and JavaScript.

We can't make some sort of sweeping philosophical argument in a great "convenience vs correctness" debate. For each API decision we have to ask: is this choice improving users' lives & work? I think this is a case where our historical decision was a clear net negative for users.

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Apr 11, 2018

@alexcrichton Yeah I'm not sure if there is a super strong rationale separating these things. It's definitely a little blurry, so we'd probably need to employ reasoning like, "paths are frequently used and printed, and the specific display adapter we have for it isn't carrying its weight." It's not an especially generic argument that can be applied to other things I think.

The docs on fmt::Display are definitely interesting. Not sure what to make of that.

@WiSaGaN

This comment has been minimized.

Contributor

WiSaGaN commented Apr 11, 2018

Does deprecating a stable interface not require a RFC?

Edit: If we need a RFC to deprecate a stable interface, I think introducing a replacement of the current interface without deprecating also needs a RFC:

  • If we do not follow up with a deprecation, we are introducing another way to do the same thing conflicting with the existing one.
  • If we follow up with a deprecation, we need a RFC in the first place, thus we might as well do it before hand to reduce the churn.

@withoutboats withoutboats changed the title from Deprecate `Path::display`, implement `Display` for `Path` to Implement `Display` for `Path` and `PathBuf` Apr 11, 2018

@retep998

This comment has been minimized.

Member

retep998 commented Apr 11, 2018

The problem with implementing Display directly is now you can call to_string directly which carries no connotation that the conversion is lossy and it has the convenience of not failing, which will cause people to reach for to_string instead of the current situation where they have to use to_string_lossy which is clear about its lossiness or to_str which returns an Option so they are aware it may fail.

Strong opposition from me on this change.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 11, 2018

@withoutboats I agree in general yes but I'd personally find the decision to only add Display for Path (and PathBuf) to be somewhat incoherent. For example why would we add it to these types and not OsString? This means that you could print a path but you couldn't print the return value of file_name().

Put another way I feel like there's a lot of "papercuts" when working with Path which are actually intentional design decisions. While some papercuts come up more often than others I think that the fact that it's a papercut doesn't necessarily undermine the original design decision, which in this case is ensuring lossy conversions are always opt-in rather than "idiomatically silent".

I think there's perhaps an argument to be made, however, that the "eat your vegetables" approach we have today ends up benefitting nearly no one. Those who are aware of the issues don't necessarily need the constant ergonomic overhead and those who aren't aware of the issues are likely to do the (subtly) wrong thing anyway.

Even if this provides a strong motivation for this change though I think we have more issues to work through than just adding a Display implementation, for example re-rationalizing what Display means when implemented and the idiomatic relationship between Display and FromStr, for example

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Apr 11, 2018

I think there's perhaps an argument to be made, however, that the "eat your vegetables" approach we have today ends up benefitting nearly no one. Those who are aware of the issues don't necessarily need the constant ergonomic overhead and those who aren't aware of the issues are likely to do the (subtly) wrong thing anyway.

Yes, this is my opinion.

@Kimundi

This comment has been minimized.

Member

Kimundi commented Apr 12, 2018

I think I agree with @retep998 that the biggest footgun this opens up is enabling to_string().

Rust already has way to many conversion mechanisms that we try hard to keep somewhat sane, with beginners often just blindly trying stuff until it works. And most languages don't bother to keep strings and paths as separated as Rust does, so people often don't expect to look out for it.

But its also true that there already many ways to misuse path-string conversions in other ways, so I also understand seeing this as a needless hurdle in thew API.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Apr 12, 2018

I agree that the "biggest footgun" is to_string - in that it seems very unlikely someone is trying to roundtrip a path from printing it with "{}", so to_string is really the footgun. I think all the arguments about the frequency that someone does this vs how frequently they don't want to roundtrip it, and how ineffectual making them write .display().to_string() is to prevent them from doing it, all apply to to_string(). In other words, what we've been talking about this whole time is primarily to_string().

When this had a deprecation warning I immediately found lots of places checked into this repo that were doing exactly this display().to_string() instead of a "more correct" to_string_lossy().into_owned(). I did not investigate if any of them try to resuscitate a path from the string elsewhere in the program.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 12, 2018

I don't think I personally agree with to_string as being the only issue here, in Cargo we've had to specially treat serialization of paths to files or otherwise into a Write. These are 99% of the time intended for consumption by another tool, for example make or a future run of cargo. In these cases non-unicode paths on Windows translate to errors that end up getting propagated to users.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 12, 2018

Another way to think about this perhaps is that if you do want to "do the right thing" then it's nearly impossible to audit with Display for Path because that can be subtly used in so many different contexts. Otherwise grepping for to_string_lossy() or .display() is much easier to do.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 12, 2018

One way to approach this problem which I would personally be more comfortable with is to have Display panic or return an error on non-utf8 paths (probably panic). That to me upholds the critical contract here (it's lossless on Display) and matches at least my own personal experience of non-utf8 paths basically never existing and when they do the application is horribly broken in many other ways.

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Apr 12, 2018

@alexcrichton That's tempting, but the problem there is when formatting error messages. If you do format!("{}: file does not exist", path) and path contains invalid UTF-8, then you definitely don't want that panicking.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 12, 2018

@BurntSushi true yeah, but I feel like we have to give on something here. We've explicitly documented that we cannot do what this PR currently proposes, so something's gotta give.

I would personally prefer to stick to the "Display is lossless" guarantee moreso than not panicking, and our messaging would be that if you want to make sure you're compatible with non-utf8 paths then you should have a test case (as that's what you'd really need anyway). It's true that panics will come up from time to time, but it's sort of just another "here's a reminder you need to handle a bug here". It's not as obvious as option.unwrap() but it's arguably far rarer to occur than an unwrap

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Apr 12, 2018

@alexcrichton Yeah I guess what I'm trying to say is that if our choice is between "status quo" and "impl Display, but panic on invalid UTF-8," then I'd probably pick the status quo.

My opinion has weakened on this over time. I dunno what to do. The to_string problem is not something I had considered, and is a tough pill to swallow.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Apr 12, 2018

I don't think people use or understand display as lossless - I think they use and understand it as pretty printing intended for the end user. For example, the Display impl of the Context type in failure prints only the context and not the underlying error - two different Contexts with the same context but different errors will Display the same (and this wasn't my design, it was @aturon's). This is based on this much looser definition of Display which is how I think Display is used.

I'm unclear how the existing situation upholds that requirement either; why is the Display impl for the std::path::Display type "faithful" if the impls in this PR are not "faithful", when all they do is delegate to that impl? This definition seems no more rigorous than any other choice.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Apr 13, 2018

This does seem like a case where people aren't clear on what the alternative is. If you really, really want to emit a non-UTF-8 path, presumably you'd have to treat the Path or PathBuf as bytes and Write those bytes. Doing so to a file, if the file format expects a raw path, can work. Doing so to stdout may or may not work, depending on if it goes to a UTF-8 terminal or to a pipe to some other program. But either way, do people know to do that, and know under what circumstances they should do that rather than just using Display (or .display())?

@shepmaster

This comment has been minimized.

Member

shepmaster commented May 7, 2018

This has not had a comment in many weeks, so ping from triage.

@retep998

This comment has been minimized.

Member

retep998 commented May 7, 2018

If you really, really want to emit a non-UTF-8 path, presumably you'd have to treat the Path or PathBuf as bytes and Write those bytes.

Which of course doesn't work work on Windows.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented May 7, 2018

@aturon had an alternative proposal

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented May 21, 2018

Ping from triage @rust-lang/libs! What's the consensus on this?

@TimNN

This comment has been minimized.

Contributor

TimNN commented Jun 12, 2018

Ping from triage @rust-lang/libs, what is the status of this?

@stokhos stokhos added the I-nominated label Jun 29, 2018

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Jul 13, 2018

@TimNN I think the status is that there is no consensus so far.

@smmalis37 Disposition only means that one team member proposed FCP in that direction. #49868 (comment) lists a concern that blocks the start of FCP.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Jul 13, 2018

I think we don't have time to hash this out right now, will revisit in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment