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

Improve the experience of using syntax extensions with #![no_std] #17100

Closed
wants to merge 9 commits into from

Conversation

kmcallister
Copy link
Contributor

This isn't as exciting as crate capture for macros, but it removes a number of immediate pain points.

@@ -764,6 +765,8 @@ mod test_map {

#[test]
fn test_show() {
use std::to_string::ToString;
Copy link
Member

Choose a reason for hiding this comment

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

Import the trait but still remove the to_string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed for this call below:

let map_str = map.to_string();

Copy link
Member

Choose a reason for hiding this comment

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

Hm; why is there the to_string -> String::from_str change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When libcollections is built with cfg(test) it links the real libstd. This means that to_string returns an std::string::String, re-exported from a previous build of libcollections, rather than the string::String defined in the crate under test. We can still compare them with .as_slice(), but for most purposes it's better to use string::String.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. Different types with the same name!

@kmcallister
Copy link
Contributor Author

I pushed commits addressing the comments. I can squash these after review.

@huonw
Copy link
Member

huonw commented Sep 12, 2014

I'd like someone else to glance over this (maybe @alexcrichton and @sfackler); r=me if they're ok with it.

(This is actually patch is along the lines of how I was thinking of solving this problem, but I never got around to it because I'm not totally in love with this solution; but it's mostly just a general unease about increasing the number of syntax extension name resolution hacks that we have, rather than something concrete.)

@kmcallister
Copy link
Contributor Author

Rebased.

Yeah, I don't love this solution either. But I want to solve these issues ASAP, so we can deliver on the promise of writing C libraries in Rust. And this seems like a fine stopgap fix until we have a way to reliably capture references to crates.

One thing I'm wondering is to what extent the "facade" pattern of libstd re-exports will appear in other Rust projects besides the standard libraries. If those projects also define syntax extensions, they'll need similar hacks.

@sfackler
Copy link
Member

I'm okay with this for now, I guess. We should really figure out how we want to fix this in the long run, and how much of that we can get done before 1.0.

(also, e17977f now refers to a nonexistent commit in the message since you rebased)

@alexcrichton
Copy link
Member

This is expanding some core apis that I'd like to consider a bit before landing. It would be great if we could avoid expanding them for now.

Notably:

  • std::collections::hash is introduced, and is a duplicate of std::hash.
  • String::format was introduced, and is a duplicate of std::fmt::format

Also, as others have noted, this is just a band-aid, it's not really fixing the underlying issue. The bugs being "fixed" here aren't really bugs per se but rather all victims of lack of macro import/export, macro hygiene at the item level, etc. Code will still not work if you have extern crate core somewhere different than the root, or if you rename it to something else.

I think we can land this for now (hopefully with no API extensions), but I'm very wary of continuing down this path which was not meant to be taken of shoehorning hygiene under the facade where it doesn't exist.

@kmcallister
Copy link
Contributor Author

I'm working on a different band-aid (see #17103) that makes some of this obsolete and should merge first.

@kmcallister
Copy link
Contributor Author

(also, e17977f now refers to a nonexistent commit in the message since you rebased)

Nope, it's still there. This was a fix to std's assert_eq! (but not core's) that made it upstream. That's part of what convinced me to address the code duplication issue instead of making it worse as in this PR.

@alexcrichton
Copy link
Member

@kmcallister do you think we can move forward with this without modifying public APIs? I would be ok with this otherwise I think.

@kmcallister
Copy link
Contributor Author

Slice syntax has a similar problem.

@alexcrichton
Copy link
Member

Closing due to inactivity, and it looks like this getting re-scrutinized in rust-lang/rfcs#453 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants