Drop legacy path support under Rust edition 2018 (or later) #5398
Conversation
src/cargo/util/toml/targets.rs
Outdated
| rename file to `src/lib.rs` or specify lib.path", | ||
| lib.name() | ||
| ) | ||
| return bail(); |
There was a problem hiding this comment.
having to add the return here concerns me. is it correct?
There was a problem hiding this comment.
I would think that just bail() (without both return and semicolon) should work?
There was a problem hiding this comment.
me too! but looks like it doesn't. I blame macro shenanigans?
error[E0308]: if and else have incompatible types
--> src/cargo/util/toml/targets.rs:159:13
|
159 | / if legacy_path.exists() {
160 | | warnings.push(format!(
161 | | "path `{}` was erroneously implicitly accepted for library `{}`,\n\
162 | | please rename the file to `src/lib.rs` or set lib.path in Cargo.toml",
... |
168 | | bail()
169 | | }
| |_____________^ expected struct `std::path::PathBuf`, found enum `std::result::Result`
|
= note: expected type `std::path::PathBuf`
found type `std::result::Result<std::option::Option<core::manifest::Target>, failure::Error>`There was a problem hiding this comment.
Hm, weird.... Anyway, I hope we can get rid of this closure altogether, as I've mentioned in other comments.
src/cargo/util/toml/targets.rs
Outdated
| name = name, | ||
| target_kind = target_kind | ||
| )) | ||
| }; |
There was a problem hiding this comment.
Let's use the bail macro here as well?
There was a problem hiding this comment.
sadly looks like no, because bail returns failure::Error and there's a codepath that buffers error strings in a vec
src/cargo/util/toml/targets.rs
Outdated
| }; | ||
| if edition >= Edition::Edition2018 { | ||
| return err(); | ||
| } | ||
| if let Some(path) = legacy_path(target) { |
There was a problem hiding this comment.
Hm, let's just wrap this if-let into an addition if edition >= 2018, so as to avoid introducing local closures?
There was a problem hiding this comment.
of course.. I find out you can't use && with if let and somehow forgot about just nesting 😆
also I assumed the closure was free. good to know it's not.
src/cargo/util/toml/targets.rs
Outdated
| }; | ||
| if edition >= Edition::Edition2018 { | ||
| return bail(); | ||
| } | ||
| let legacy_path = package_root.join("src").join(format!("{}.rs", lib.name())); | ||
| if legacy_path.exists() { |
There was a problem hiding this comment.
Hm, let's add && edition == 2015 to this condition, instead of introducing the bail closure?
|
thanks for the review @matklad, it's much simpler now :) |
|
✅! 😄 |
|
gentle review bump, please 😄 |
|
Thanks for the bump! @bors r+ |
|
📌 Commit 46f4408 has been approved by |
Drop legacy path support under Rust edition 2018 (or later) builds on #5335 submitted for early feedback: wdyt @matklad? is this what you had in mind? what should change? what should be added? how should we test this? is the current (2015) messaging enough to drop it in 2018? r? @matklad <!--{"baseBranch":"rust-lang:cargo:target-autodiscovery"}-->
|
☀️ Test successful - status-appveyor, status-travis |
builds on #5335
submitted for early feedback: wdyt @matklad? is this what you had in mind? what should change? what should be added? how should we test this? is the current (2015) messaging enough to drop it in 2018?
r? @matklad