Skip to content

Deprecate std::path::Path's aliases of std::fs functions. #80741

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

Closed

Conversation

sunfishcode
Copy link
Member

@sunfishcode sunfishcode commented Jan 6, 2021

std::path::Path has several functions which are trivial aliases for std::fs
functions (eg. metadata) and a few which are minor convenience wrappers for
std::fs functions (eg. is_file).

This PR deprecates these functions, so that std only has one preferred name
for these operations. And it clarifies the role of the std::path module, which
otherwise just has pure data structures and algorithms.

The convenience wrappers, is_file, is_dir, and exists, are operations for
which seemingly obvious uses are prone to TOCTTOU errors. So while it would be
straightforward to add corresponding similar convenience functions to std::fs,
this PR does not propose doing so.

This PR also adds convenience functions exists, is_file, is_dir, to std::fs,
in a dedicated commit.

Specifically, this PR deprecates the 5 functions in std::path::Path which are
trivial aliases of functions in std::fs:

and the 3 which are convenience wrappers around std::fs::metadata:

`std::path::Path` has several functions which are trivial aliases for `std::fs`
functions (eg. [`metadata`]) and a few which are minor convenience wrappers for
`std::fs` functions (eg. [`is_file`]).

[`metadata`]: https://doc.rust-lang.org/std/path/struct.Path.html#method.metadata
[`is_file`]: https://doc.rust-lang.org/std/path/struct.Path.html#method.is_file

This PR deprecates these functions, so that `std` only has one preferred name
for these operations. And it clarifies the role of the `std::path` module, which
otherwise just has pure data structures and algorithms.

The convenience wrappers, `is_file`, `is_dir`, and `exists`, are operations for
which seemingly obvious uses are prone to TOCTTOU errors. So while it would be
straightforward to add corresponding similar convenience functions to `std::fs`,
this PR does not propose doing so.

Specifically, this PR deprecates the 5 functions in `std::path::Path` which are
trivial aliases of functions in `std::fs`:
 - [`metadata`](https://doc.rust-lang.org/std/path/struct.Path.html#method.metadata)
 - [`symlink_metadata`](https://doc.rust-lang.org/std/path/struct.Path.html#method.symlink_metadata)
 - [`canonicalize`](https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize)
 - [`read_link`](https://doc.rust-lang.org/std/path/struct.Path.html#method.read_link)
 - [`read_dir`](https://doc.rust-lang.org/std/path/struct.Path.html#method.read_dir)

and the 3 which are convenience wrappers around `std::fs::metadata`:
 - [`is_file`](https://doc.rust-lang.org/std/path/struct.Path.html#method.is_file)
 - [`is_dir`](https://doc.rust-lang.org/std/path/struct.Path.html#method.is_dir)
 - [`exists`](https://doc.rust-lang.org/std/path/struct.Path.html#method.exists)
@rust-highfive
Copy link
Contributor

r? @withoutboats

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment on lines +2501 to +2510
///
/// When the goal is simply to read from the source, the most reliable way to
/// test the source can be read is to open it. See [`fs::read_dir`] for more
/// information.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to update the deprecated docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this because is_file already has a similar comment, and these comments relate to one of the reasons for deprecating these functions, so it seems like useful context.

@pickfire
Copy link
Contributor

pickfire commented Jan 6, 2021

I would find the helper function still useful when we only have Path. I thought it was deprecated for performance issue, like reading the metadata twice but looks like that is not the case. Even if performance is the case, I think it would be better to have a comment in the docs to show that it is more preferable to check the metadata only once. Deprecating them can increase the chance that the users will call the metadata once but we lost an ergonomic function. (Much like having input() to take input)

@rust-log-analyzer

This comment has been minimized.

@sunfishcode sunfishcode force-pushed the deprecate-path-fs-aliases branch from 5744648 to cc65c85 Compare January 6, 2021 02:54
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@tesuji
Copy link
Contributor

tesuji commented Jan 6, 2021

Doesn't deprecating those methods cause too much churns represented by now?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@sunfishcode sunfishcode force-pushed the deprecate-path-fs-aliases branch from 4065cd5 to 4ee86d2 Compare January 6, 2021 10:27
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.................................................................................................... 9000/11244
.................................................................................................... 9100/11244
.................................................................................................... 9200/11244
........................................i......i.................................................... 9300/11244
...............................................................................iiiiii..iiiiii.i..... 9400/11244
.................................................................................................... 9600/11244
.................................................................................................... 9700/11244
.................................................................................................... 9800/11244
.................................................................................................... 9900/11244
---
 finished in 0.438 seconds
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 27 tests
iiiiiiiiiiiiiiiiiiiiiiiiiii

 finished in 0.074 seconds
Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 116 tests
iiiiiiiiii.i.i..i..i..ii.....ii....ii..........iiii.........i.....i...i.......ii.i.ii.....iiii.....i 100/116
test result: ok. 78 passed; 0 failed; 38 ignored; 0 measured; 0 filtered out; finished in 2.44s

 finished in 2.519 seconds
Suite("src/test/ui-fulldeps") not skipped for "bootstrap::test::UiFullDeps" -- not in ["src/tools/tidy"]
---
   Compiling rustc-rayon v0.3.0
   Compiling tempfile v3.1.0
   Compiling regex v1.3.9
   Compiling rustdoc v0.0.0 (/checkout/src/librustdoc)
error: use of deprecated associated function `std::path::Path::is_file`: other processes may remove, rename, or replace files at any time
    |
    |
418 |             if !p.is_file() {
    |                   ^^^^^^^ help: replace the use of the deprecated associated function: `use `std::fs::File::open` or `std::fs::metadata``
    |
    = note: `-D deprecated` implied by `-D warnings`

error: use of deprecated associated function `std::path::Path::is_file`: other processes may remove, rename, or replace files at any time
    |
    |
431 |                 if !theme_file.is_file() {
    |                                ^^^^^^^ help: replace the use of the deprecated associated function: `use `std::fs::File::open` or `std::fs::metadata``

error: use of deprecated associated function `std::path::Path::is_file`: other processes may remove, rename, or replace files at any time
    |
    |
501 |             if !index_page.is_file() {
    |                            ^^^^^^^ help: replace the use of the deprecated associated function: `use `std::fs::File::open` or `std::fs::metadata``

error: use of deprecated associated function `std::path::Path::exists`: other processes may remove or rename files at any time
   --> src/librustdoc/html/render/mod.rs:903:17
    |
903 |         if path.exists() {
    |                 ^^^^^^ help: replace the use of the deprecated associated function: `use `std::fs::metadata``

error: use of deprecated associated function `std::path::Path::exists`: other processes may remove or rename files at any time
   --> src/librustdoc/html/render/mod.rs:929:17
    |
929 |         if path.exists() {
    |                 ^^^^^^ help: replace the use of the deprecated associated function: `use `std::fs::metadata``

error: use of deprecated associated function `std::path::Path::is_dir`: other processes may remove, rename, or replace directories at any time
  --> src/librustdoc/html/render/cache.rs:35:23
   |
35 |     if local_location.is_dir() {
   |                       ^^^^^^ help: replace the use of the deprecated associated function: `use `std::fs::read_dir` or `std::fs::metadata``
error: aborting due to 6 previous errors

error: could not compile `rustdoc`

@sunfishcode sunfishcode force-pushed the deprecate-path-fs-aliases branch from 4ee86d2 to b7aae1b Compare January 6, 2021 11:21
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking quote v1.0.7
   Compiling psm v0.1.11
   Compiling stacker v0.1.12
   Compiling rustc_llvm v0.0.0 (/checkout/compiler/rustc_llvm)
error[E0425]: cannot find function `exists` in module `fs`
    |
    |
209 |         } else if fs::exists(Path::new(lib)) {
    |                       ^^^^^^ not found in `fs`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.
error: could not compile `rustc_llvm`
error: could not compile `rustc_llvm`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" " llvm max_level_info" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "-p" "rustc-main" "-p" "rustc_driver" "-p" "rustc_metadata" "-p" "rustc_macros" "-p" "rustc_index" "-p" "rustc_expand" "-p" "rustc_ast_passes" "-p" "rustc_lexer" "-p" "rustc_attr" "-p" "rustc_mir" "-p" "rustc_apfloat" "-p" "coverage_test_macros" "-p" "rustc_infer" "-p" "rustc_graphviz" "-p" "rustc_trait_selection" "-p" "rustc_parse_format" "-p" "rustc_errors" "-p" "rustc_lint_defs" "-p" "rustc_hir" "-p" "rustc_span" "-p" "rustc_arena" "-p" "rustc_parse" "-p" "rustc_serialize" "-p" "rustc_lint" "-p" "rustc_data_structures" "-p" "rustc_ast" "-p" "rustc_plugin_impl" "-p" "rustc_save_analysis" "-p" "rustc_ast_pretty" "-p" "rustc_middle" "-p" "rustc_type_ir" "-p" "rustc_query_system" "-p" "rustc_feature" "-p" "rustc_error_codes" "-p" "rustc_target" "-p" "rustc_session" "-p" "rustc_fs_util" "-p" "rustc_hir_pretty" "-p" "rustc_interface" "-p" "rustc_resolve" "-p" "rustc_mir_build" "-p" "rustc_codegen_llvm" "-p" "rustc_llvm" "-p" "rustc_privacy" "-p" "rustc_passes" "-p" "rustc_symbol_mangling" "-p" "rustc_ast_lowering" "-p" "rustc_typeck" "-p" "rustc_traits" "-p" "rustc_incremental" "-p" "rustc_ty_utils" "-p" "rustc_builtin_macros" "-p" "rustc_codegen_ssa" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:33

@sunfishcode
Copy link
Member Author

I don't have a strong opinion about the convenience functions, and it turns out they're used a lot in Rust's own codebase, so I've now added new convenience functions exists, is_file, and is_dir, in std::fs, to this PR.

This PR is a work in progress at this point; there's more code in Rust that needs to be updated to avoid the new deprecation warnings, since these warnings are interpreted as errors in the Rust build.

sunfishcode added a commit to sunfishcode/rust that referenced this pull request Jan 6, 2021
This also eliminates a use of a `Path` convenience function, in support
of rust-lang#80741, refactoring `std::path` to focus on pure data structures and
algorithms.
sunfishcode added a commit to sunfishcode/rust that referenced this pull request Jan 6, 2021
This also eliminates a use of a `Path` convenience function, in support
of rust-lang#80741, refactoring `std::path` to focus on pure data structures and
algorithms.
sunfishcode added a commit to sunfishcode/rust that referenced this pull request Jan 6, 2021
…ion.

This also eliminates a use of a `Path` convenience function, in support
of rust-lang#80741, refactoring `std::path` to focus on pure data structures and
algorithms.
sunfishcode added a commit to sunfishcode/rust that referenced this pull request Jan 6, 2021
This also eliminates a use of a `Path` convenience function, in support
of rust-lang#80741, refactoring `std::path` to focus on pure data structures and
algorithms.
@sunfishcode
Copy link
Member Author

This has turned out to be a complex PR, so I've now started splitting out parts into separate PRs: #80754, #80755, #80756, and #80757.

@sunfishcode sunfishcode marked this pull request as draft January 6, 2021 17:16
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2021
…l, r=davidtwco

Optimize away a `fs::metadata` call.

This also eliminates a use of a `Path` convenience function, in support
of rust-lang#80741, refactoring `std::path` to focus on pure data structures and
algorithms.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2021
…ental, r=nagisa

Optimize away some `fs::metadata` calls.

This also eliminates a use of a `Path` convenience function, in support
of rust-lang#80741, refactoring `std::path` to focus on pure data structures and
algorithms.
@bors
Copy link
Collaborator

bors commented Jan 8, 2021

☔ The latest upstream changes (presumably #80756) made this pull request unmergeable. Please resolve the merge conflicts.

@sunfishcode
Copy link
Member Author

From here and the internals thread it sounds like there isn't a lot of enthusiasm for these changes, so I'll close this.

@sunfishcode sunfishcode closed this Jan 8, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2021
Optimize away some path lookups in the generic `fs::copy` implementation

This also eliminates a use of a `Path` convenience function, in support
of rust-lang#80741, refactoring `std::path` to focus on pure data structures and
algorithms.
@sunfishcode sunfishcode deleted the deprecate-path-fs-aliases branch January 13, 2021 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants