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

Make str a library type (and lang item) #19036

Closed
aturon opened this issue Nov 17, 2014 · 16 comments
Closed

Make str a library type (and lang item) #19036

aturon opened this issue Nov 17, 2014 · 16 comments
Labels
A-collections Area: `std::collections` A-type-system Area: Type system P-medium Medium priority

Comments

@aturon
Copy link
Member

aturon commented Nov 17, 2014

With DST, there is no longer any reason that str needs to be a primitive type; it can instead be defined in the library as a newtype wrapper around [u8]. (It will, of course, still need to be a lang item for string literals to work).

The main visible consequence of this change would be the removal of several prelude traits in favor of inherent methods -- so it has some impact on library stabilization.

It may also be that the name could/should be revisited if it is no longer primitive.

@aturon
Copy link
Member Author

aturon commented Nov 17, 2014

cc @nick29581 @nikomatsakis

@aturon
Copy link
Member Author

aturon commented Nov 17, 2014

Nominating. I think this is P-backcompat-libs, but not 1.0 (since we can live with the extension traits if we have to).

@aturon aturon added A-libs A-type-system Area: Type system labels Nov 17, 2014
@japaric
Copy link
Member

japaric commented Nov 17, 2014

Interesting! I tried this once before, and found an ICE which has been already solved. I'll give it another try soon (only the library side, I don't know how to do the lang item part)

@aturon
Copy link
Member Author

aturon commented Nov 17, 2014

@japaric Awesome! Let me know if I can do anything to help.

@nikomatsakis
Copy link
Contributor

When we discussed things at the work week where we planned out DST, we did intend for str to be a library type.

@japaric
Copy link
Member

japaric commented Nov 18, 2014

@aturon As a test I created the Str([u8]) type out of tree (see the docs). I removed StrPrelude and replaced it with inherent methods on the Str type, moved the from_utf8 free function into Str, and also checked that the doctests pass. The only issue that I found in the process was #19037 which already got fixed.

About the implementation process, I guess these are the steps to take?

  • Add a str lang item for the string literals ("foo")
  • Add the str([u8]) struct, mark it with the str lang item, remove StrPrelude, add inherent methods
  • Remove the compiler support that glues the built-in str to string literals.
  • Remove the rest of the compiler support for the built-in str (like being specially treated as a DST?)

I really hope that we would be able to split the work like that (although probably parts 2 and 3 will have to get done at the same time), otherwise it's going to be a big patch. Also, depending if we want to rename str to Str (since it would no longer be a built-in/primitive type), we may end up with a massive code churn and downstream breakage (probably not worth it).

@nikomatsakis
Copy link
Contributor

@japaric what do you mean by "adding a str lang item" as a step? that seems like it's... a line in a table, or perhaps you mean something more?

@japaric
Copy link
Member

japaric commented Nov 18, 2014

@nikomatsakis I meant modify the compiler to allow something like this:

#[lang = "str"]
struct Str([u8]);

let foo: &'static Str = "Hello world!";

Or can I simply do:

// libcore/str.rs
struct str([u8]);

impl str { fn contains__(&self, needle: &str) -> bool { /* .. */ } }

// and this will just work?
let foo: &'static str = "Hello!";
assert!(foo.contains__("el"));

@nikomatsakis
Copy link
Contributor

@japaric no, we will definitely need a lang item, but it seems like at least some of the steps you listed must be done atomically (adding the lang item; changing the type of literals to use this lang item). We could (and should) move the extension traits over separately, once that transition is done.

Is this the staging you had in mind?

@nikomatsakis
Copy link
Contributor

(If nothing else, we probably want to do a snapshot before changing the extension traits)

@nikomatsakis
Copy link
Contributor

Oh, and: I think that we should not change the name of str as part of this issue. If we want to do that, that's something to consider separately. I'd certainly want a stronger justification (which I think may exist, but that's another story) than "it's a lang item and hence only partially built-in to the compiler".

@japaric
Copy link
Member

japaric commented Nov 18, 2014

Is this the staging you had in mind?

Yes, something like that. I can help with the second part (extension trait removal). (I'd help with the first part (lang item + literals) but I'm not familiar with the compiler internals, it'll be faster if someone who is familiar with that part works on it).

@pnkfelix
Copy link
Member

If we get to this before 1.0, then we'll remove the extension traits.

If we don't, then we'll just have to keep the extension traits around. But that's something we can live with.

So, P-high, not 1.0.

@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Nov 20, 2014
@kmcallister kmcallister added the A-collections Area: `std::collections` label Jan 16, 2015
@seanmonstar
Copy link
Contributor

This is still wanted? It seemed like #19612 it was stated that the core team has changed its mind...

@aturon
Copy link
Member Author

aturon commented Feb 6, 2015

@seanmonstar Thanks for the ping. I'm going to close this issue, since it was determined that this change had more downsides than benefits.

@aturon aturon closed this as completed Feb 6, 2015
@seanmonstar
Copy link
Contributor

Was mostly curious since @brson just linked it on the discuss thread about nice-to-haves for 1.0.

lnicola pushed a commit to lnicola/rust that referenced this issue Feb 10, 2025
Split out `ExpressionStore` from `Body`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collections` A-type-system Area: Type system P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants