Repository navigation
Remove remaining mentions of Unique - #164573
Conversation
|
Some changes occurred in src/etc/gdb_providers.py cc @Walnut356 Some changes occurred in src/etc/lldb_providers.py cc @Walnut356 Some changes occurred in src/etc/natvis cc @Walnut356 |
|
|
59fdc09 to
5fa4209
Compare
| // It's not possible to extract the inner Uniq directly from the Box, | ||
| // instead we cast it to a *const which aliases the Unique | ||
| // It's not possible to extract the inner pointer directly from the Box, | ||
| // instead we cast it to a *const which aliases it |
There was a problem hiding this comment.
Not entirely sure what this is saying (is it unsound to access the pointer of Box directly outside the destructor?) I'll just leave the comment's phrasing as-is for now, unless someone has a better idea.
There was a problem hiding this comment.
The ptr field of Unique was inaccessible due to privacy, so this code uses the builtin box deref to get a pointer to the contents of the box.
(did some git history, this is ancient code from pre 1.0)
There was a problem hiding this comment.
Still a bit strange, Unique implemented Pointer too, but idk how this looked pre 1.0. Thanks! ^^
There was a problem hiding this comment.
Looks like the original reason for this comment was #24144 (comment) which shouldn't be an issue any more. Let's just remove this comment entirely.
|
@bors try jobs=test-aarch64-apple-1,test-x86_64-msvc-1 |
Remove remaining mentions of `Unique` try-job: test-aarch64-apple-1 try-job: test-x86_64-msvc-1
This comment has been minimized.
This comment has been minimized.
| // It's not possible to extract the inner Uniq directly from the Box, | ||
| // instead we cast it to a *const which aliases the Unique | ||
| // It's not possible to extract the inner pointer directly from the Box, | ||
| // instead we cast it to a *const which aliases it |
There was a problem hiding this comment.
Looks like the original reason for this comment was #24144 (comment) which shouldn't be an issue any more. Let's just remove this comment entirely.
|
|
||
|
|
||
| def unwrap_unique_or_non_null(unique_or_nonnull): | ||
| def unwrap_boxraw_or_non_null(boxraw_or_nonnull): |
There was a problem hiding this comment.
I'm a bit worried that this change as-is trades one kind of confusing (what's "unique"?) for another (why BoxRaw mentions in String, VecDeque, etc.?). Also, it's debatable if we should even purge mentions of Unique from the debug providers as long as they're making an effort to be backwards compatible with versions where Unique still existed.
How about calling this unwrap_non_null_or_wrapper and leave a doc comment to the effect of this also handles BoxRaw and the previous Unique type, both of which have a pointer: NonNull field.
Plus a FIXME that functions + callers can be simplified once we no longer care about backcompat with Rust < 1.101 (or whatever it was where Unique was removed).
There was a problem hiding this comment.
Also, it's debatable if we should even purge mentions of Unique from the debug providers as long as they're making an effort to be backwards compatible with versions where Unique still existed
I'd have to check again, but wasnt backwards compatibility broken when the field-retrieval-path of the String/ Vec pointer was changed in the previous patch?
It's not a huge deal, we technically dont have any kind of firm policy on backwards compat for these. I've mostly been informally keeping some since debuggers (e.g. codelldb) will generally pull from the stable toolchain instead of whatever toolchain was actually used to compile. While that's technically not our problem, it's usually not too much hassle and saves on bug reports.
I'll try to gather some opinions about a deprecation window or whatever for changes in the future. There might also be a way for us to print a warning when using visualizers that dont match the version the binary was compiled with.
There was a problem hiding this comment.
The changes to natvis are breaking, but I don't think that has any compatibility concerns because natvis is embedded in the binary when it's linked, so it should always match the toolchain version.
The change to lldb_providers in #162804 should be backwards compatible, it changed several direct accesses to foo.pointer.pointer to unwrap_unique_or_non_null(foo) where foo was Unique before and is now NonNull. The gdb providers were already doing the latter before. I didn't find any other change to the providers in the previous PRs.
I'll try to gather some opinions about a deprecation window or whatever for changes in the future. There might also be a way for us to print a warning when using visualizers that dont match the version the binary was compiled with.
Thanks, some guidance for how to navigate these kind of changes would be great!
There was a problem hiding this comment.
I don't think that has any compatibility concerns because natvis is embedded in the binary when it's linked, so it should always match the toolchain version.
There is one minor footgun with that (and GDB too, since it works the same way), which is that the "embedding" is just a filepath pointing to the file in the toolchain install e.g. (compiled with cargo +1.90-x86_64-pc-windows-msvc build)
llvm-pdbutil pdb2yaml --all ".\target\debug\sample.pdb" | rg natvis
- 'C:\Users\ant_b\.rustup\toolchains\1.90-x86_64-pc-windows-msvc\lib\rustlib\etc\libstd.natvis'
...
If you compile with stable and then update stable, the stable folder stays the same, so you can end up having an old binary point to new scripts:
llvm-pdbutil pdb2yaml --all ".\target\debug\sample.pdb" | rg natvis
- 'C:\Users\ant_b\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\etc\libstd.natvis'
...
That should be rare enough though not to matter, since most of the time peoples' IDE's will recompile before running the debugger anyway.
(as an aside, that footgun doesn't affect anything that uses #![debugger_visualizer] since that attribute does literally copypaste the script contents into the binary)
...i really need to document that somewhere lmao
The change to lldb_providers in #162804 should be backwards compatible, it changed several direct accesses to foo.pointer.pointer to unwrap_unique_or_non_null(foo) where foo was Unique before and is now NonNull.
Ah, excellent. I misread that patch a little. Thanks for the clarification 😄 I tested it just to be sure and it definitely is backwards compatible
There was a problem hiding this comment.
nice, accidental backwards compatibility.
what do we do with the Uniques in natvis now (and what needs to change in this PR wrt the function here)?
|
Gonna get around to this later, gotta help someone move a boulder up a |
5fa4209 to
43f527a
Compare
|
@rustbot ready |
|
@bors r+ rollup |
…uwer Rollup of 7 pull requests Successful merges: - #163270 (Allow simd types in improper_ctypes lint) - #164855 (Stop mangling rust_eh_personality again for now) - #153679 (When multiple traits are applicable for method, do not specialize main message to the first one that applied) - #164248 (Retroactively add missing stabilized APIs to relnotes) - #164573 (Remove remaining mentions of `Unique`) - #164824 (PassWrapper: stop setting Options.ExceptionModel on LLVM 24) - #164844 (Add tracking issue for `#![feature(char_is_xid)]`)
Rollup merge of #164573 - maxdexh:unique-cleanup, r=hanna-kruppe Remove remaining mentions of `Unique` Removes the remaining mentions of `Unique` from the codebase I didn't touch the standalone `core_mini` codegen examples that declare their own `struct Unique` because I don't know what they are intended to show, and also some of them have a very outdated version of the layout of `Box` (no allocators). The only mentions of `Unique` I touched were the ones actually referring to `core::ptr::unique::Unique`. cc #164146 (comment) r? hanna-kruppe
View all comments
Removes the remaining mentions of
Uniquefrom the codebaseI didn't touch the standalone
core_minicodegen examples that declare their ownstruct Uniquebecause I don't know what they are intended to show, and also some of them have a very outdated version of the layout ofBox(no allocators).The only mentions of
UniqueI touched were the ones actually referring tocore::ptr::unique::Unique.cc #164146 (comment)
r? hanna-kruppe