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

Derive common traits for panic::Location. #73583

Merged
merged 2 commits into from
Jul 28, 2020
Merged

Conversation

anp
Copy link
Member

@anp anp commented Jun 21, 2020

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
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 Jun 21, 2020
@dtolnay 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
@dtolnay
Copy link
Member

dtolnay commented Jun 21, 2020

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

rfcbot commented Jun 21, 2020

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

Concerns:

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 21, 2020
@rfcbot
Copy link

rfcbot commented Jun 21, 2020

🔔 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 Jun 21, 2020
@Aaron1011
Copy link
Member

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

@anp
Copy link
Member Author

anp commented Jun 21, 2020

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

@SimonSapin
Copy link
Contributor

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

@rfcbot 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
Copy link
Member Author

anp commented Jun 22, 2020

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

  1. the point you make about Path vs str is valid but the host/target mismatch seems like a dealbreaker
  2. 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
Copy link
Contributor

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.

@anp anp force-pushed the location-eq branch 2 times, most recently from 4605f60 to 1b4aa00 Compare June 30, 2020 03:56
Add documentation about the host/target behavior of Location::file.
@anp
Copy link
Member Author

anp commented Jun 30, 2020

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.

@Muirrum Muirrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 17, 2020
@SimonSapin
Copy link
Contributor

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

rfcbot commented Jul 17, 2020

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

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

This comment has been minimized.

@anp

This comment has been minimized.

@LukasKalbertodt

This comment has been minimized.

@anp

This comment has been minimized.

@LukasKalbertodt

This comment has been minimized.

@anp

This comment has been minimized.

@anp

This comment has been minimized.

@LukasKalbertodt

This comment has been minimized.

@anp

This comment has been minimized.

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

rfcbot commented Jul 27, 2020

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.

@rfcbot 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
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks for the new docs!

@dtolnay
Copy link
Member

dtolnay commented Jul 27, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2020

📌 Commit 416dc4b has been approved by dtolnay

@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 Jul 27, 2020
@bors
Copy link
Contributor

bors commented Jul 27, 2020

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

@bors
Copy link
Contributor

bors commented Jul 28, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2020
@bors bors merged commit 9be8ffc into rust-lang:master Jul 28, 2020
@bors bors mentioned this pull request Jul 28, 2020
@anp anp deleted the location-eq branch July 28, 2020 02:09
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 13, 2020
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
@dtolnay dtolnay assigned dtolnay and unassigned kennytm Mar 24, 2024
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. 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.