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

Implement FromStr for PathBuf #48292

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

topecongiro
Copy link
Contributor

Closes #44431.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Feb 17, 2018
@topecongiro topecongiro changed the title Implement FromStr for Path and PathBuf Implement FromStr for PathBuf Feb 17, 2018
@topecongiro
Copy link
Contributor Author

Implementing FromStr for Path caused following errors so I removed it from this PR:

error[E0277]: the trait bound `[u8]: core::marker::Sized` is not satisfied in `path::Path`
    --> libstd/path.rs:1414:6
     |
1414 | impl FromStr for Path {
     |      ^^^^^^^ `[u8]` does not have a constant size known at compile-time
     |
     = help: within `path::Path`, the trait `core::marker::Sized` is not implemented for `[u8]`
     = note: required because it appears within the type `path::Path`

error[E0277]: the trait bound `[u8]: core::marker::Sized` is not satisfied in `path::Path`
    --> libstd/path.rs:1417:5
     |
1417 | /     fn from_str(s: &str) -> Result<Self, Self::Err> {
1418 | |         Ok(Path::new(s))
1419 | |     }
     | |_____^ `[u8]` does not have a constant size known at compile-time
     |
     = help: within `path::Path`, the trait `core::marker::Sized` is not implemented for `[u8]`
     = note: required because it appears within the type `path::Path`
     = note: required by `core::result::Result`

@kennytm
Copy link
Member

kennytm commented Feb 17, 2018

@topecongiro try to implement it for &'a Path

@topecongiro
Copy link
Contributor Author

@kennytm Thank you for the review! I added a commit to implement FromStr for &'a Path.

@topecongiro topecongiro changed the title Implement FromStr for PathBuf Implement FromStr for PathBuf and Path Feb 17, 2018
@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 17, 2018
@kennytm
Copy link
Member

kennytm commented Feb 17, 2018

@topecongiro Oh sorry, looks like it is not possible to implement FromStr for Path after all (it requires GAT to support such conversion). The current implementation will cause undefined behavior. We may introduce impl<'a> From<&'a str> for &'a Path though, which seems to be missing.

cc @rust-lang/libs — This PR introduces impl FromStr for PathBuf which will be insta-stable. #44431 is marked C-feature-accepted so I'll approve it unless any of you disagrees.

/// An error returned when parsing `Path` or `PathBuf` using `from_str` fails.
#[derive(Debug, Clone, PartialEq, Eq)]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct ParsePathError { _priv: () }
Copy link
Member

@kennytm kennytm Feb 17, 2018

Choose a reason for hiding this comment

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

Since parsing a Path will never fail, please make it an empty enum:

pub enum ParsePathError {}

Alternatively, simply return std::string::ParseError.

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't reuse an existing type.

}
}

#[stable(feature = "rust1", since = "1.0.0")]
Copy link
Member

Choose a reason for hiding this comment

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

The feature should not be rust1. Please create a feature name e.g. path_from_str and set the since to be 1.26.0.

@kennytm kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 17, 2018
#[stable(feature = "path_from_str", since = "1.26.0")]
impl fmt::Display for ParsePathError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
"provided string was not a valid path".fmt(f)
Copy link
Member

Choose a reason for hiding this comment

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

This should just be match *self {}.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2018
@alexcrichton
Copy link
Member

@kennytm yeah this should be safe to r+ as soon as it's ready, and looks good to me modulo @sfackler's comment!

@kennytm
Copy link
Member

kennytm commented Feb 26, 2018

Yeah following the implementation of std::string::ParseError it should simply be implemented as match *self {}.

(cc @topecongiro)

@shepmaster
Copy link
Member

Ping from triage, @topecongiro ! Will you have time soon to address the feedback above?

@topecongiro topecongiro force-pushed the from_str-for-path-and-pathbuf branch from a9b935f to 05a9acc Compare March 5, 2018 16:05
@topecongiro
Copy link
Contributor Author

I am sorry for the late response. I have updated the PR to use match *self {}. Thank you for the reviews!

@alexcrichton
Copy link
Member

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 5, 2018

Team member @alexcrichton 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.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 5, 2018
@dtolnay dtolnay changed the title Implement FromStr for PathBuf and Path Implement FromStr for PathBuf Mar 8, 2018
@dtolnay
Copy link
Member

dtolnay commented Mar 8, 2018

std::path::ParsePathError

If we aren't going to be parsing any other things in std::path, we should do like std::string::ParseError and leave out the repetitive bit. If we are going to be parsing other things in std::path in the future, perhaps ParsePathError is not the best name for parsing not a Path but a PathBuf.

@rfcbot concern error name

@sfackler
Copy link
Member

sfackler commented Mar 8, 2018

Both ParsePathError and string::ParseError are going to turn into typedefs for ! when it stabilizes, which may be at around the same time as this API.

@rfcbot
Copy link

rfcbot commented Mar 8, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 8, 2018
@sfackler
Copy link
Member

sfackler commented Mar 8, 2018

My point was that we may not have to stabilize ParsePathError at all if ! is stable in the right timeframe.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 8, 2018

📌 Commit 05a9acc has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2018
@bors
Copy link
Contributor

bors commented Mar 8, 2018

⌛ Testing commit 05a9acc with merge 38c616009116c7e770eb16020c6beba55dd46403...

@bors
Copy link
Contributor

bors commented Mar 8, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 2018
@kennytm
Copy link
Member

kennytm commented Mar 8, 2018

@bors retry rollup

3 hour timeout in check i686-pc-windows-msvc (1).

It took 43 minutes to compile stage1-rustc 🙄.

Timing breakdown
make-prepare                	  71.85
stage0-tidy                 	  83.90
bootstrap                   	  21.25
stage0-rustc                	1699.66
llvm                        	 760.98
stage0-trans-llvm           	 173.86
stage1-std                  	 127.63
stage1-test                 	  38.91
stage1-rustc                	2587.31
stage1-trans-llvm           	 258.56
stage0-compiletest          	 182.32
test/ui                     	 205.61
test/parse-fail             	   8.04
test/run-fail               	  38.24
test/run-pass-valgrind      	   6.63
test/mir-opt                	  44.07
test/codegen                	   8.12
test/codegen-units          	   7.40
test/incremental            	  43.25
test/ui-fulldeps            	  92.72
test/run-pass-fulldeps      	 600.14
test/run-fail-fulldeps      	   7.21
test/compile-fail-fulldeps  	 170.95
test/incremental-fulldeps   	   5.17
...
Relevant logs
[01:00:47] Building stage1 compiler artifacts (i686-pc-windows-msvc -> i686-pc-windows-msvc)
[01:00:48]    Compiling rustc v0.0.0 (file:///C:/projects/rust/src/librustc)
[01:00:48]    Compiling winapi-build v0.1.1
[01:00:49]    Compiling bitflags v1.0.1
[01:00:49]    Compiling cc v1.0.4
[01:00:50]    Compiling libc v0.2.36
[01:00:50]    Compiling byteorder v1.2.1
[01:00:52]    Compiling rustc-serialize v0.3.24
[01:01:23]    Compiling winapi v0.2.8
[01:01:23]    Compiling arena v0.0.0 (file:///C:/projects/rust/src/libarena)
[01:01:25]    Compiling ar v0.3.1
[01:01:26]    Compiling serialize v0.0.0 (file:///C:/projects/rust/src/libserialize)
[01:01:50]    Compiling rustc_platform_intrinsics v0.0.0 (file:///C:/projects/rust/src/librustc_platform_intrinsics)
[01:02:16]    Compiling rustc_driver v0.0.0 (file:///C:/projects/rust/src/librustc_driver)
[01:02:23]    Compiling smallvec v0.6.0
[01:02:33]    Compiling lazy_static v0.2.11
[01:02:34]    Compiling rustc-demangle v0.1.5
[01:03:00]    Compiling stable_deref_trait v1.0.0
[01:03:00]    Compiling rustc_metadata v0.0.0 (file:///C:/projects/rust/src/librustc_metadata)
[01:03:01]    Compiling graphviz v0.0.0 (file:///C:/projects/rust/src/libgraphviz)
[01:03:03]    Compiling quick-error v1.2.1
[01:03:03]    Compiling fmt_macros v0.0.0 (file:///C:/projects/rust/src/libfmt_macros)
[01:03:04]    Compiling rustc_back v0.0.0 (file:///C:/projects/rust/src/librustc_back)
[01:03:04]    Compiling syntax v0.0.0 (file:///C:/projects/rust/src/libsyntax)
[01:03:05]    Compiling rustc_incremental v0.0.0 (file:///C:/projects/rust/src/librustc_incremental)
[01:03:06]    Compiling unicode-width v0.1.4
[01:03:06]    Compiling cfg-if v0.1.2
[01:03:06]    Compiling winapi v0.3.4
[01:03:06]    Compiling lazy_static v1.0.0
[01:03:06]    Compiling rustc_cratesio_shim v0.0.0 (file:///C:/projects/rust/src/librustc_cratesio_shim)
[01:03:07]    Compiling kernel32-sys v0.2.2
[01:03:09]    Compiling rand v0.3.20
[01:03:12]    Compiling rls-span v0.4.0
[01:03:14]    Compiling miniz-sys v0.1.10
[01:03:17]    Compiling log_settings v0.1.1
[01:03:17]    Compiling owning_ref v0.3.3
[01:03:18]    Compiling humantime v1.1.1
[01:03:18]    Compiling log v0.4.1
[01:03:20]    Compiling rustc_apfloat v0.0.0 (file:///C:/projects/rust/src/librustc_apfloat)
[01:03:21]    Compiling rls-data v0.15.0
[01:03:26]    Compiling jobserver v0.1.9
[01:03:26]    Compiling ena v0.9.2
[01:03:36]    Compiling parking_lot_core v0.2.9
[01:03:36]    Compiling wincolor v0.1.4
[01:03:38]    Compiling atty v0.2.6
[01:03:39]    Compiling backtrace v0.3.5
[01:03:42]    Compiling flate2 v1.0.1
[01:03:43]    Compiling termcolor v0.3.3
[01:03:46]    Compiling parking_lot v0.5.3
[01:03:49]    Compiling env_logger v0.5.4
[01:03:50]    Compiling rustc_data_structures v0.0.0 (file:///C:/projects/rust/src/librustc_data_structures)
[01:04:02]    Compiling syntax_pos v0.0.0 (file:///C:/projects/rust/src/libsyntax_pos)
[01:04:14]    Compiling rustc_errors v0.0.0 (file:///C:/projects/rust/src/librustc_errors)
[01:08:38]    Compiling proc_macro v0.0.0 (file:///C:/projects/rust/src/libproc_macro)
[01:08:38]    Compiling rustc_const_math v0.0.0 (file:///C:/projects/rust/src/librustc_const_math)
[01:09:07]    Compiling syntax_ext v0.0.0 (file:///C:/projects/rust/src/libsyntax_ext)
[01:24:23]    Compiling rustc_mir v0.0.0 (file:///C:/projects/rust/src/librustc_mir)
[01:24:23]    Compiling rustc_typeck v0.0.0 (file:///C:/projects/rust/src/librustc_typeck)
[01:30:34]    Compiling rustc_allocator v0.0.0 (file:///C:/projects/rust/src/librustc_allocator)
[01:35:30]    Compiling rustc_resolve v0.0.0 (file:///C:/projects/rust/src/librustc_resolve)
[01:38:38]    Compiling rustc_privacy v0.0.0 (file:///C:/projects/rust/src/librustc_privacy)
[01:38:41]    Compiling rustc_save_analysis v0.0.0 (file:///C:/projects/rust/src/librustc_save_analysis)
[01:39:11]    Compiling rustc_borrowck v0.0.0 (file:///C:/projects/rust/src/librustc_borrowck)
[01:40:15]    Compiling rustc_passes v0.0.0 (file:///C:/projects/rust/src/librustc_passes)
[01:40:18]    Compiling rustc_lint v0.0.0 (file:///C:/projects/rust/src/librustc_lint)
[01:41:07]    Compiling rustc_trans_utils v0.0.0 (file:///C:/projects/rust/src/librustc_trans_utils)
[01:41:23]    Compiling rustc_plugin v0.0.0 (file:///C:/projects/rust/src/librustc_plugin)
[01:43:54]    Compiling rustc-main v0.0.0 (file:///C:/projects/rust/src/rustc)
[01:43:55]     Finished release [optimized] target(s) in 2587.31 secs

For comparison, the build timings of the all jobs in this build:

_____________________ msvc 64 msvc 32 (1) msvc 32 (2) check-aux cargotest tools gnu 32 (1) gnu 32 (2) gnu 64
make-prepare 56.27 71.85 51.27 61.58 51.48 52.78 135.40 151.58 141.20
stage0-rustc 1078.83 1699.66 1127.29 1132.70 1128.38 1188.20 1071.22 1039.24 1072.40
llvm 676.31 760.98 643.38 684.11 649.86 702.65 700.12 841.12 761.67
stage0-trans-llvm 142.24 173.86 149.75 148.69 168.25 155.19 92.93 106.90 94.76
stage1-std 86.28 127.63 94.67 85.30 98.90 85.76 89.79 106.60 85.79
stage1-test 19.59 38.91 25.16 19.87 20.36 23.20 19.89 23.73 19.12
stage1-rustc 1517.80 2587.31 1856.99 1685.40 1590.54 1683.64 1539.36 1849.28 1520.96
stage1-trans-llvm 166.29 258.56 201.93 163.17 198.00 169.29 125.50 147.61 124.95

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2018
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 8, 2018
…athbuf, r=alexcrichton

Implement FromStr for PathBuf

Closes rust-lang#44431.
bors added a commit that referenced this pull request Mar 8, 2018
Rollup of 7 pull requests

- Successful merges: #48292, #48682, #48699, #48738, #48752, #48789, #48808
- Failed merges:
@bors bors merged commit 05a9acc into rust-lang:master Mar 9, 2018
@topecongiro topecongiro deleted the from_str-for-path-and-pathbuf branch March 9, 2018 01:04
kennytm added a commit to kennytm/rust that referenced this pull request May 3, 2018
…r=sfackler

Revert "Implement FromStr for PathBuf"

This reverts commit 05a9acc.

The libs team was discussing rust-lang#44431 today and the changes originally added in rust-lang#48292 and the conclusion was that we'd like to revert this for now until `!` is stable. This'll provide us maximal flexibility to tweak the error type here in the future, and it looks like `!` is close-ish to stabilization so hopefully this won't be delayed for too long.
SimonSapin added a commit to SimonSapin/rust that referenced this pull request Oct 17, 2018
Initially landed in rust-lang#48292
and reverted in rust-lang#50401.
This time, use `std::string::ParseError` as suggested in
rust-lang#44431 (comment)
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Oct 27, 2018
Implement FromStr for PathBuf

Initially landed in rust-lang#48292 and reverted in rust-lang#50401. This time, use `std::string::ParseError` as suggested in rust-lang#44431 (comment)
kennytm added a commit to kennytm/rust that referenced this pull request Oct 28, 2018
Implement FromStr for PathBuf

Initially landed in rust-lang#48292 and reverted in rust-lang#50401. This time, use `std::string::ParseError` as suggested in rust-lang#44431 (comment)
ischeinkman pushed a commit to ischeinkman/libnx-rs-std that referenced this pull request Dec 20, 2018
Initially landed in rust-lang#48292
and reverted in rust-lang#50401.
This time, use `std::string::ParseError` as suggested in
rust-lang#44431 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants