Make str into a libcore struct (redux)
#107939
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Lokathor
reviewed
Feb 12, 2023
| #[rustc_has_incoherent_inherent_impls] | ||
| #[lang = "str"] | ||
| #[repr(transparent)] | ||
| pub struct Str([u8]); |
There was a problem hiding this comment.
is there a reason that the struct is Str (uppercase) here instead of the usual str used in all other places?
There was a problem hiding this comment.
well currently it's an implementation detail, so it doesn't really matter what it's named -- the unstable feature gate means you can't even name it via its path
There was a problem hiding this comment.
Are Str and str intended to be the same or different types?
(And the unstable feature gate doesn't necessarily apply in library/core)
There was a problem hiding this comment.
Are Str and str intended to be the same or different types?
They're the same type.
All I'm saying is that there's nothing forcing us to use the str name, since it's never exposed. All name resolution for str currently happens through a special path that handles other primitive types (like i32). If/when we wanted to change str to be resolved via the prelude or something, then yeah, the name would probably have to be changed (though maybe not, since we could just use core::str::Str as str; in the prelude).
Similarly for printing, str as a lang item is specialized to always print as that, to make sure we avoid printing core::str::Str in code when path trimming is explicitly disabled, as it is in certain diagnostics.
There was a problem hiding this comment.
Thanks for explaining, that makes sense. I suspect we'd want it to be str everywhere eventually (even though it doesn't matter) just to keep things clearer in rustdoc source and so on.
(But it's understandable that it might take some time to get to that point)
bjorn3
reviewed
Feb 12, 2023
| @@ -21,6 +21,8 @@ macro_rules! stringify { | |||
| trait Sized {} | |||
| #[lang = "copy"] | |||
| trait Copy {} | |||
| #[lang = "str"] | |||
| struct Str([u8]); | |||
There was a problem hiding this comment.
compiler/rustc_codegen_cranelift/examples/mini_core.rs needs this too.
|
☔ The latest upstream changes (presumably #107869) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
requested changes
Feb 13, 2023
| @@ -259,7 +259,6 @@ pub struct CommonTypes<'tcx> { | |||
| pub u128: Ty<'tcx>, | |||
| pub f32: Ty<'tcx>, | |||
| pub f64: Ty<'tcx>, | |||
| pub str_: Ty<'tcx>, | |||
There was a problem hiding this comment.
should probably remove this in the first commit
compiler/rustc_middle/src/ty/sty.rs
Outdated
| @@ -1854,10 +1854,10 @@ impl<'tcx> Ty<'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn sequence_element_type(self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> { | |||
| // FIXME(str): Remove tcx from this | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
could land this on master on its own
This was referenced Feb 13, 2023
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 14, 2023
…ilstrieb Use `is_str` instead of string kind comparison Split out from rust-lang#107939
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 14, 2023
…ilstrieb Use `is_str` instead of string kind comparison Split out from rust-lang#107939
compiler-errors
added a commit
to compiler-errors/rust
that referenced
this pull request
Feb 25, 2023
…or-auto, r=lcnr Treat `str` as containing `[u8]` for auto trait purposes Wanted to gauge `@rust-lang/lang` and `@rust-lang/types` teams' thoughts on treating `str` as "containing" a `[u8]` slice for auto-trait purposes. `@dtolnay` brought this up in rust-lang#13231 (comment) as a blocker for future `str` type librarification, and I think it's both a valid concern and very easy to fix. I'm interested in actually doing that `str` type librarification (rust-lang#107939), but this probably should be considered in the mean time regardless of that PR. r? types for the impl, though this definitely needs an FCP.
compiler-errors
added a commit
to compiler-errors/rust
that referenced
this pull request
Feb 25, 2023
…or-auto, r=lcnr Treat `str` as containing `[u8]` for auto trait purposes Wanted to gauge `@rust-lang/lang` and `@rust-lang/types` teams' thoughts on treating `str` as "containing" a `[u8]` slice for auto-trait purposes. `@dtolnay` brought this up in rust-lang#13231 (comment) as a blocker for future `str` type librarification, and I think it's both a valid concern and very easy to fix. I'm interested in actually doing that `str` type librarification (rust-lang#107939), but this probably should be considered in the mean time regardless of that PR. r? types for the impl, though this definitely needs an FCP.
Jarcho
pushed a commit
to Jarcho/rust
that referenced
this pull request
Feb 26, 2023
…ilstrieb Use `is_str` instead of string kind comparison Split out from rust-lang#107939
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 26, 2023
…or-auto, r=lcnr Treat `str` as containing `[u8]` for auto trait purposes Wanted to gauge ``@rust-lang/lang`` and ``@rust-lang/types`` teams' thoughts on treating `str` as "containing" a `[u8]` slice for auto-trait purposes. ``@dtolnay`` brought this up in rust-lang#13231 (comment) as a blocker for future `str` type librarification, and I think it's both a valid concern and very easy to fix. I'm interested in actually doing that `str` type librarification (rust-lang#107939), but this probably should be considered in the mean time regardless of that PR. r? types for the impl, though this definitely needs an FCP.
|
Was this closed just because of inactivity or is there some reason not to try again in the future? |
|
@oli-obk: The PR is pretty stale, and I'm somewhat afraid of the perf implications, though I didn't get so far as to test anything. |
|
That's fair; I did have some hopes for this PR since it feels like it would fit nicely along with some flavour of safe-transmute/ref-wrapping shenanigans. Definitely a load of work, though, and rebasing 150+ file changes over 8 months ago sounds like a nightmare. @.@ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Along the same vein as #70911 and #19612, though I only discovered those attempts after finishing this one, so the approach is probably slightly different.
Still somewhat broken, specifically:
FIXME(str)should be addressed or OK'd for later work/librarification effortsI wanted to open this mostly to bring back discussion and solicit some early reviews.
cc @rust-lang/compiler and @rust-lang/libs-api who might care about this being revived
r? @ghost