Rollup merge of rust-lang#144510 - orlp:fix-location-ord, r=ibraheemdev

Fix Ord, Eq and Hash implementation of panic::Location

Fixes rust-lang#144486.

Now properly compares/hashes the filename rather than the pointer to the string.

Author: jhpratt

core/src/panic/location.rs

@@ -1,5 +1,7 @@
+use crate::cmp::Ordering;
 use crate::ffi::CStr;
 use crate::fmt;
+use crate::hash::{Hash, Hasher};
 use crate::marker::PhantomData;
 use crate::ptr::NonNull;
 
@@ -32,7 +34,7 @@ use crate::ptr::NonNull;
 /// Files are compared as strings, not `Path`, which could be unexpected.
 /// See [`Location::file`]'s documentation for more discussion.
 #[lang = "panic_location"]
-#[derive(Copy, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)]
+#[derive(Copy, Clone)]
 #[stable(feature = "panic_hooks", since = "1.10.0")]
 pub struct Location<'a> {
     // A raw pointer is used rather than a reference because the pointer is valid for one more byte
@@ -44,6 +46,44 @@ pub struct Location<'a> {
     _filename: PhantomData<&'a str>,
 }
 
+#[stable(feature = "panic_hooks", since = "1.10.0")]
+impl PartialEq for Location<'_> {
+    fn eq(&self, other: &Self) -> bool {
+        // Compare col / line first as they're cheaper to compare and more likely to differ,
+        // while not impacting the result.
+        self.col == other.col && self.line == other.line && self.file() == other.file()
+    }
+}
+
+#[stable(feature = "panic_hooks", since = "1.10.0")]
+impl Eq for Location<'_> {}
+
+#[stable(feature = "panic_hooks", since = "1.10.0")]
+impl Ord for Location<'_> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.file()
+            .cmp(other.file())
+            .then_with(|| self.line.cmp(&other.line))
+            .then_with(|| self.col.cmp(&other.col))
+    }
+}
+
+#[stable(feature = "panic_hooks", since = "1.10.0")]
+impl PartialOrd for Location<'_> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+#[stable(feature = "panic_hooks", since = "1.10.0")]
+impl Hash for Location<'_> {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.file().hash(state);
+        self.line.hash(state);
+        self.col.hash(state);
+    }
+}
+
 #[stable(feature = "panic_hooks", since = "1.10.0")]
 impl fmt::Debug for Location<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

coretests/tests/panic/location.rs

@@ -3,6 +3,23 @@ use core::panic::Location;
 // Note: Some of the following tests depend on the source location,
 // so please be careful when editing this file.
 
+mod file_a;
+mod file_b;
+mod file_c;
+
+// A small shuffled set of locations for testing, along with their true order.
+const LOCATIONS: [(usize, &'static Location<'_>); 9] = [
+    (7, file_c::two()),
+    (0, file_a::one()),
+    (3, file_b::one()),
+    (5, file_b::three()),
+    (8, file_c::three()),
+    (6, file_c::one()),
+    (2, file_a::three()),
+    (4, file_b::two()),
+    (1, file_a::two()),
+];
+
 #[test]
 fn location_const_caller() {
     const _CALLER_REFERENCE: &Location<'static> = Location::caller();
@@ -20,7 +37,7 @@ fn location_const_file() {
 fn location_const_line() {
     const CALLER: &Location<'static> = Location::caller();
     const LINE: u32 = CALLER.line();
-    assert_eq!(LINE, 21);
+    assert_eq!(LINE, 38);
 }
 
 #[test]
@@ -34,6 +51,28 @@ fn location_const_column() {
 fn location_debug() {
     let f = format!("{:?}", Location::caller());
     assert!(f.contains(&format!("{:?}", file!())));
-    assert!(f.contains("35"));
+    assert!(f.contains("52"));
     assert!(f.contains("29"));
 }
+
+#[test]
+fn location_eq() {
+    for (i, a) in LOCATIONS {
+        for (j, b) in LOCATIONS {
+            if i == j {
+                assert_eq!(a, b);
+            } else {
+                assert_ne!(a, b);
+            }
+        }
+    }
+}
+
+#[test]
+fn location_ord() {
+    let mut locations = LOCATIONS.clone();
+    locations.sort_by_key(|(_o, l)| **l);
+    for (correct, (order, _l)) in locations.iter().enumerate() {
+        assert_eq!(correct, *order);
+    }
+}

coretests/tests/panic/location/file_a.rs

@@ -0,0 +1,15 @@
+use core::panic::Location;
+
+// Used for test super::location_{ord, eq}. Must be in a dedicated file.
+
+pub const fn one() -> &'static Location<'static> {
+    Location::caller()
+}
+
+pub const fn two() -> &'static Location<'static> {
+    Location::caller()
+}
+
+pub const fn three() -> &'static Location<'static> {
+    Location::caller()
+}

coretests/tests/panic/location/file_b.rs

@@ -0,0 +1,15 @@
+use core::panic::Location;
+
+// Used for test super::location_{ord, eq}. Must be in a dedicated file.
+
+pub const fn one() -> &'static Location<'static> {
+    Location::caller()
+}
+
+pub const fn two() -> &'static Location<'static> {
+    Location::caller()
+}
+
+pub const fn three() -> &'static Location<'static> {
+    Location::caller()
+}

coretests/tests/panic/location/file_c.rs

@@ -0,0 +1,11 @@
+// Used for test super::location_{ord, eq}. Must be in a dedicated file.
+
+// This is used for testing column ordering of Location, hence this ugly one-liner.
+// We must fmt skip the entire containing module or else tidy will still complain.
+#[rustfmt::skip]
+mod no_fmt {
+    use core::panic::Location;
+    pub const fn one() -> &'static Location<'static> { Location::caller() }    pub const fn two() -> &'static Location<'static> { Location::caller() }    pub const fn three() -> &'static Location<'static> { Location::caller() }
+}
+
+pub use no_fmt::*;