Implement FromStr for PathBuf#48292

topecongiro · 2018-02-17T07:36:46Z

rust-highfive · 2018-02-17T07:36:49Z

(rust_highfive has picked a reviewer for you, use r? to override)

topecongiro · 2018-02-17T07:55:56Z

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 · 2018-02-17T11:38:02Z

@topecongiro try to implement it for &'a Path

topecongiro · 2018-02-17T12:03:54Z

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

kennytm · 2018-02-17T14:29:59Z

@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.

kennytm · 2018-02-17T14:33:51Z

src/libstd/path.rs

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

pub enum ParsePathError {}

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

We shouldn't reuse an existing type.

kennytm · 2018-02-17T14:34:53Z

src/libstd/path.rs

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.

sfackler · 2018-02-20T05:59:34Z

src/libstd/path.rs

This should just be match *self {}.

alexcrichton · 2018-02-26T15:16:40Z

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

kennytm · 2018-02-26T15:36:02Z

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

(cc @topecongiro)

shepmaster · 2018-03-02T19:35:04Z

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

topecongiro · 2018-03-05T16:06:17Z

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

alexcrichton · 2018-03-05T17:12:06Z

@rfcbot fcp merge

rfcbot · 2018-03-05T17:12:07Z

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

Concerns:

~~error name~~ resolved by Implement FromStr for PathBuf #48292 (comment)

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.

dtolnay · 2018-03-08T05:45:45Z

`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 · 2018-03-08T05:50:43Z

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 · 2018-03-08T06:04:26Z

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

sfackler · 2018-03-08T06:13:36Z

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

alexcrichton · 2018-03-08T15:51:21Z

@bors: r+

bors · 2018-03-08T15:51:21Z

📌 Commit 05a9acc has been approved by alexcrichton

bors · 2018-03-08T16:02:31Z

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

bors · 2018-03-08T19:05:18Z

💔 Test failed - status-appveyor

kennytm · 2018-03-08T19:12:47Z

@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:

Details

_____________________	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

…athbuf, r=alexcrichton Implement FromStr for PathBuf Closes rust-lang#44431.

Rollup of 7 pull requests - Successful merges: #48292, #48682, #48699, #48738, #48752, #48789, #48808 - Failed merges:

…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.

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)

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)

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)

rust-highfive assigned kennytm Feb 17, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2018

topecongiro force-pushed the from_str-for-path-and-pathbuf branch from 74984b3 to 5cfbda9 Compare February 17, 2018 07:53

topecongiro changed the title ~~Implement FromStr for Path and PathBuf~~ Implement FromStr for PathBuf Feb 17, 2018

topecongiro changed the title ~~Implement FromStr for PathBuf~~ Implement FromStr for PathBuf and Path Feb 17, 2018

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 reviewed Feb 17, 2018

View reviewed changes

kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 17, 2018

topecongiro force-pushed the from_str-for-path-and-pathbuf branch from f6c289c to a9b935f Compare February 17, 2018 15:39

kennytm mentioned this pull request Feb 17, 2018

Path & PathBuf missing FromStr trait #44431

Closed

sfackler reviewed Feb 20, 2018

View reviewed changes

src/libstd/path.rs Outdated

Copy link

Member

sfackler Feb 20, 2018

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 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

Implement FromStr for PathBuf

05a9acc

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

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 changed the title ~~Implement FromStr for PathBuf and Path~~ Implement FromStr for PathBuf Mar 8, 2018

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

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 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

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

Rollup merge of rust-lang#48292 - topecongiro:from_str-for-path-and-p…

f12d5aa

…athbuf, r=alexcrichton Implement FromStr for PathBuf Closes rust-lang#44431.

Manishearth mentioned this pull request Mar 8, 2018

Rollup of 7 pull requests #48849

Merged

bors added a commit that referenced this pull request Mar 8, 2018

Auto merge of #48849 - Manishearth:rollup, r=Manishearth

604d4ce

Rollup of 7 pull requests - Successful merges: #48292, #48682, #48699, #48738, #48752, #48789, #48808 - Failed merges:

bors merged commit 05a9acc into rust-lang:master Mar 9, 2018

topecongiro deleted the from_str-for-path-and-pathbuf branch March 9, 2018 01:04

SimonSapin mentioned this pull request Apr 21, 2018

Revert stabilization of never_type (!) et al #50121

Merged

alexcrichton mentioned this pull request May 2, 2018

Revert "Implement FromStr for PathBuf" #50401

Merged

SimonSapin added a commit to SimonSapin/rust that referenced this pull request Oct 17, 2018

Implement FromStr for PathBuf

a0df420

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)

SimonSapin mentioned this pull request Oct 17, 2018

Implement FromStr for PathBuf #55148

Merged

ischeinkman pushed a commit to ischeinkman/libnx-rs-std that referenced this pull request Dec 20, 2018

Implement FromStr for PathBuf

35d9901

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)

Uh oh!

Conversation

topecongiro commented Feb 17, 2018

Uh oh!

rust-highfive commented Feb 17, 2018

Uh oh!

topecongiro commented Feb 17, 2018

Uh oh!

kennytm commented Feb 17, 2018

Uh oh!

topecongiro commented Feb 17, 2018

Uh oh!

kennytm commented Feb 17, 2018

Uh oh!

kennytm Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum Feb 17, 2018

Choose a reason for hiding this comment

Uh oh!

kennytm Feb 17, 2018

Choose a reason for hiding this comment

Uh oh!

sfackler Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Feb 26, 2018

Uh oh!

kennytm commented Feb 26, 2018

Uh oh!

shepmaster commented Mar 2, 2018

Uh oh!

topecongiro commented Mar 5, 2018

Uh oh!

alexcrichton commented Mar 5, 2018

Uh oh!

rfcbot commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtolnay commented Mar 8, 2018

std::path::ParsePathError

Uh oh!

sfackler commented Mar 8, 2018

Uh oh!

rfcbot commented Mar 8, 2018

Uh oh!

sfackler commented Mar 8, 2018

Uh oh!

alexcrichton commented Mar 8, 2018

Uh oh!

bors commented Mar 8, 2018

Uh oh!

bors commented Mar 8, 2018

Uh oh!

bors commented Mar 8, 2018

Uh oh!

kennytm commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

kennytm Feb 17, 2018 •

edited

Loading

rfcbot commented Mar 5, 2018 •

edited

Loading

`std::path::ParsePathError`

kennytm commented Mar 8, 2018 •

edited

Loading