Derive common traits for panic::Location. #73583

anp · 2020-06-21T15:00:53Z

Now that #[track_caller] is on track to stabilize, one of the roughest edges of working with it is the fact that you can't do much with Location except turn it back into a (&str, u32, u32). Which makes sense because the type was defined around the panic machinery originally passing around that tuple (it has the same layout as Location even).

This PR derives common traits for the type in accordance with the API guidelines (those apply to core, right?).

There's a risk here, e.g. if we ever change the representation of Location in a way that makes it harder to implement Ord, we might not be able to make that change in a backwards-compatible way. I don't think there's any other compatibility hazard here, as the only changes we currently imagine for the type are to add end fields.

cc @rust-lang/libs

rust-highfive · 2020-06-21T15:00:57Z

r? @kennytm

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

dtolnay · 2020-06-21T19:37:06Z

These all seem fine to me. @rust-lang/libs: this adds the following impls for https://doc.rust-lang.org/nightly/std/panic/struct.Location.html:

Copy
Clone
Eq
Hash
Ord
PartialEq
PartialOrd

@rfcbot fcp merge

Some notes:

PartialOrd is based on comparing file, line, column in that order.

PartialEq is equivalent to what you would get by calling file(), line(), column() and comparing the resulting &str, u32, u32. It is possible for the same location on the filesystem to have different file() in different places and thus compare unequal to itself, but this should be unusual. Example:

// src/main.rs

#![feature(track_caller)]

#[path = "module.rs"]
mod a;

#[path = "../src/module.rs"]
mod b;

fn main() {
    println!("{}", a::get().file());
    println!("{}", b::get().file());
}

// src/module.rs

use std::panic::Location;

pub fn get() -> &'static Location<'static> {
    f()
}

#[track_caller]
fn f() -> &'static Location<'static> {
    Location::caller()
}

src/module.rs
src/../src/module.rs

rfcbot · 2020-06-21T19:37:07Z

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

Concerns:

~~file ordering~~ resolved by Derive common traits for panic::Location. #73583 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 · 2020-06-21T20:07:19Z

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

Aaron1011 · 2020-06-21T21:41:07Z

Since the fields of Location are private, I think we should explicitly document the behavior of PartialOrd.

anp · 2020-06-21T22:11:57Z

Makes sense to me. Sounds like it may be worth adding tests for PartialEq and PartialOrd behavior.

SimonSapin · 2020-06-21T22:15:40Z

@rfcbot concern file ordering

In Location::ord (and partial_ord), should the file component be compared with Path::ord (component-wise) instead of str::ord? I think it would make more sense in the "happy" case, but Path is platform-specific and when cross compiling Location::file is meaningful for the host platform, not the target platform.

anp · 2020-06-22T00:07:30Z

Hm, that’s an interesting subtlety. Two thoughts:

the point you make about Path vs str is valid but the host/target mismatch seems like a dealbreaker
Ord is IMO the least valuable impl of those in this PR, and I included it for completeness based on the API guidelines

Thoughts on removing PartialOrd/Ord from this patch?

SimonSapin · 2020-06-22T13:16:06Z

I agree that "just" using Path::ord doesn’t work because of cross-compilation. We don’t have library support for host-platform paths separately from target-platform paths, and this doesn’t seem worth adding it.

Dropping these impls certainly resolves the issue and unblocks the rest of the PR, and we can always discuss separately adding them later.

That said I can live with using str::ord, we can mention this behavior in docs and users can access components and do something else if they need. I filed a concern so this gets discussed, but I’ll mark it resolved if others think this is the way to go.

Add documentation about the host/target behavior of Location::file.

anp · 2020-06-30T04:00:05Z

OK, I've added some documentation to address the comments here. If having this all documented makes it clear we should remove PartialOrd, let me know, otherwise I'm eager for feedback on the wording.

SimonSapin · 2020-07-17T19:47:56Z

This shows an example of Path ordering (which compares component-wise) being different from string ordering:

use std::path::Path;
pub fn main() {
    dbg!("foo-bar" < "foo/bar");
    dbg!(Path::new("foo-bar") < Path::new("foo/bar"));
    dbg!(vec!["foo-bar"] < vec!["foo", "bar"]);
}

(Playground)

[src/main.rs:3] "foo-bar" < "foo/bar" = true
[src/main.rs:4] Path::new("foo-bar") < Path::new("foo/bar") = false
[src/main.rs:5] vec!["foo-bar"] < vec!["foo", "bar"] = false

I couldn’t find examples other than where a path component on one side is a prefix of the same-position component on the other side, and the next character in the longer component is ordered before the path separator.

The only characters before / (the path separator on Unix) in ASCII are not allowed in a Rust identifier, so having them in a Rust source file path would require include!("…") or #[path = "…"] mod foo;. (On Windows the default separator is \, which sorts after digits and upper-case letter in ASCII.)

It looks like having a different ordering would be rather uncommon, so I think we can live with the slightly-incorrect one.

@rfcbot resolve file ordering

rfcbot · 2020-07-17T19:48:00Z

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

src/libcore/panic.rs

Co-authored-by: Simon Sapin <[email protected]>

rfcbot · 2020-07-27T19:50:36Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

dtolnay

Thanks for the new docs!

dtolnay · 2020-07-27T20:25:11Z

@bors r

bors · 2020-07-27T20:25:13Z

📌 Commit 416dc4b has been approved by dtolnay

bors · 2020-07-27T22:38:31Z

⌛ Testing commit 416dc4b with merge 9be8ffc...

bors · 2020-07-28T00:42:24Z

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing 9be8ffc to master...

Pkgsrc changes: * Remove patches now integrated upstream, many related to SunOS / Illumos. * The LLVM fix for powerpc is also now integrated upstream. * Adapt those patches where the source has moved or parts are integrated. * The randomness patches no longer applies, and I could not find where those files went... * Provide a separate bootstrap for NetBSD/powerpc 9.0, since apparently the C ABI is different from 8.0. Yes, this appears to be specific to the NetBSD powerpc ports. Upstream changes: Version 1.47.0 (2020-10-08) ========================== Language -------- - [Closures will now warn when not used.][74869] Compiler -------- - [Stabilized the `-C control-flow-guard` codegen option][73893], which enables [Control Flow Guard][1.47.0-cfg] for Windows platforms, and is ignored on other platforms. - [Upgraded to LLVM 11.][73526] - [Added tier 3\* support for the `thumbv4t-none-eabi` target.][74419] - [Upgrade the FreeBSD toolchain to version 11.4][75204] - [`RUST_BACKTRACE`'s output is now more compact.][75048] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [`CStr` now implements `Index<RangeFrom<usize>>`.][74021] - [Traits in `std`/`core` are now implemented for arrays of any length, not just those of length less than 33.][74060] - [`ops::RangeFull` and `ops::Range` now implement Default.][73197] - [`panic::Location` now implements `Copy`, `Clone`, `Eq`, `Hash`, `Ord`, `PartialEq`, and `PartialOrd`.][73583] Stabilized APIs --------------- - [`Ident::new_raw`] - [`Range::is_empty`] - [`RangeInclusive::is_empty`] - [`Result::as_deref`] - [`Result::as_deref_mut`] - [`Vec::leak`] - [`pointer::offset_from`] - [`f32::TAU`] - [`f64::TAU`] The following previously stable APIs have now been made const. - [The `new` method for all `NonZero` integers.][73858] - [The `checked_add`,`checked_sub`,`checked_mul`,`checked_neg`, `checked_shl`, `checked_shr`, `saturating_add`, `saturating_sub`, and `saturating_mul` methods for all integers.][73858] - [The `checked_abs`, `saturating_abs`, `saturating_neg`, and `signum` for all signed integers.][73858] - [The `is_ascii_alphabetic`, `is_ascii_uppercase`, `is_ascii_lowercase`, `is_ascii_alphanumeric`, `is_ascii_digit`, `is_ascii_hexdigit`, `is_ascii_punctuation`, `is_ascii_graphic`, `is_ascii_whitespace`, and `is_ascii_control` methods for `char` and `u8`.][73858] Cargo ----- - [`build-dependencies` are now built with opt-level 0 by default.][cargo/8500] You can override this by setting the following in your `Cargo.toml`. ```toml [profile.release.build-override] opt-level = 3 ``` - [`cargo-help` will now display man pages for commands rather just the `--help` text.][cargo/8456] - [`cargo-metadata` now emits a `test` field indicating if a target has tests enabled.][cargo/8478] - [`workspace.default-members` now respects `workspace.exclude`.][cargo/8485] - [`cargo-publish` will now use an alternative registry by default if it's the only registry specified in `package.publish`.][cargo/8571] Misc ---- - [Added a help button beside Rustdoc's searchbar that explains rustdoc's type based search.][75366] - [Added the Ayu theme to rustdoc.][71237] Compatibility Notes ------------------- - [Bumped the minimum supported Emscripten version to 1.39.20.][75716] - [Fixed a regression parsing `{} && false` in tail expressions.][74650] - [Added changes to how proc-macros are expanded in `macro_rules!` that should help to preserve more span information.][73084] These changes may cause compiliation errors if your macro was unhygenic or didn't correctly handle `Delimiter::None`. - [Moved support for the CloudABI target to tier 3.][75568] - [`linux-gnu` targets now require minimum kernel 2.6.32 and glibc 2.11.][74163] Internal Only -------- - [Improved default settings for bootstrapping in `x.py`.][73964] You can read details about this change in the ["Changes to `x.py` defaults"](https://blog.rust-lang.org/inside-rust/2020/08/30/changes-to-x-py-defaults.html) post on the Inside Rust blog. - [Added the `rustc-docs` component.][75560] This allows you to install and read the documentation for the compiler internal APIs. (Currently only available for `x86_64-unknown-linux-gnu`.) [1.47.0-cfg]: https://docs.microsoft.com/en-us/windows/win32/secbp/control-flow-guard [76980]: rust-lang/rust#76980 [75048]: rust-lang/rust#75048 [74163]: rust-lang/rust#74163 [71237]: rust-lang/rust#71237 [74869]: rust-lang/rust#74869 [73858]: rust-lang/rust#73858 [75716]: rust-lang/rust#75716 [75908]: rust-lang/rust#75908 [75516]: rust-lang/rust#75516 [75560]: rust-lang/rust#75560 [75568]: rust-lang/rust#75568 [75366]: rust-lang/rust#75366 [75204]: rust-lang/rust#75204 [74650]: rust-lang/rust#74650 [74419]: rust-lang/rust#74419 [73964]: rust-lang/rust#73964 [74021]: rust-lang/rust#74021 [74060]: rust-lang/rust#74060 [73893]: rust-lang/rust#73893 [73526]: rust-lang/rust#73526 [73583]: rust-lang/rust#73583 [73084]: rust-lang/rust#73084 [73197]: rust-lang/rust#73197 [72488]: rust-lang/rust#72488 [cargo/8456]: rust-lang/cargo#8456 [cargo/8478]: rust-lang/cargo#8478 [cargo/8485]: rust-lang/cargo#8485 [cargo/8500]: rust-lang/cargo#8500 [cargo/8571]: rust-lang/cargo#8571 [`Ident::new_raw`]: https://doc.rust-lang.org/nightly/proc_macro/struct.Ident.html#method.new_raw [`Range::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.Range.html#method.is_empty [`RangeInclusive::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.RangeInclusive.html#method.is_empty [`Result::as_deref_mut`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref_mut [`Result::as_deref`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref [`TypeId::of`]: https://doc.rust-lang.org/nightly/std/any/struct.TypeId.html#method.of [`Vec::leak`]: https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.leak [`f32::TAU`]: https://doc.rust-lang.org/nightly/std/f32/consts/constant.TAU.html [`f64::TAU`]: https://doc.rust-lang.org/nightly/std/f64/consts/constant.TAU.html [`pointer::offset_from`]: https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.offset_from

rust-highfive assigned kennytm Jun 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2020

anp mentioned this pull request Jun 21, 2020

Remove illicit::AnonRc::raw_location anp/moxie#135

Closed

dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 21, 2020

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 Jun 21, 2020

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 21, 2020

anp force-pushed the location-eq branch 2 times, most recently from 4605f60 to 1b4aa00 Compare June 30, 2020 03:56

Derive common traits for panic::Location.

d275739

Add documentation about the host/target behavior of Location::file.

anp force-pushed the location-eq branch from 1b4aa00 to d275739 Compare June 30, 2020 03:58

anp mentioned this pull request Jul 2, 2020

impl Eq, PartialEq for illicit::Layer anp/moxie#152

Open

SimonSapin reviewed Jul 17, 2020

View reviewed changes

src/libcore/panic.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Clarify core::panic::Location's docs on ordering.

416dc4b

Co-authored-by: Simon Sapin <[email protected]>

rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jul 27, 2020

rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jul 27, 2020

dtolnay approved these changes Jul 27, 2020

View reviewed changes

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 Jul 27, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2020

bors merged commit 9be8ffc into rust-lang:master Jul 28, 2020

bors mentioned this pull request Jul 28, 2020

mv std libs to library/ #73265

Merged

anp deleted the location-eq branch July 28, 2020 02:09

dtolnay assigned dtolnay and unassigned kennytm Mar 24, 2024

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derive common traits for panic::Location. #73583

Derive common traits for panic::Location. #73583

anp commented Jun 21, 2020 •

edited

Loading

rust-highfive commented Jun 21, 2020

dtolnay commented Jun 21, 2020

rfcbot commented Jun 21, 2020 •

edited by SimonSapin

Loading

rfcbot commented Jun 21, 2020

Aaron1011 commented Jun 21, 2020

anp commented Jun 21, 2020

SimonSapin commented Jun 21, 2020

anp commented Jun 22, 2020 •

edited

Loading

SimonSapin commented Jun 22, 2020

anp commented Jun 30, 2020

SimonSapin commented Jul 17, 2020

rfcbot commented Jul 17, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rfcbot commented Jul 27, 2020

dtolnay left a comment

dtolnay commented Jul 27, 2020

bors commented Jul 27, 2020

bors commented Jul 27, 2020

bors commented Jul 28, 2020

Derive common traits for panic::Location. #73583

Derive common traits for panic::Location. #73583

Conversation

anp commented Jun 21, 2020 • edited Loading

rust-highfive commented Jun 21, 2020

dtolnay commented Jun 21, 2020

rfcbot commented Jun 21, 2020 • edited by SimonSapin Loading

rfcbot commented Jun 21, 2020

Aaron1011 commented Jun 21, 2020

anp commented Jun 21, 2020

SimonSapin commented Jun 21, 2020

anp commented Jun 22, 2020 • edited Loading

SimonSapin commented Jun 22, 2020

anp commented Jun 30, 2020

SimonSapin commented Jul 17, 2020

rfcbot commented Jul 17, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rfcbot commented Jul 27, 2020

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Jul 27, 2020

bors commented Jul 27, 2020

bors commented Jul 27, 2020

bors commented Jul 28, 2020

anp commented Jun 21, 2020 •

edited

Loading

rfcbot commented Jun 21, 2020 •

edited by SimonSapin

Loading

anp commented Jun 22, 2020 •

edited

Loading