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

Implement From<&'a &'static str> for Arguments<'a> #257

Closed
EFanZh opened this issue Aug 6, 2023 · 7 comments
Closed

Implement From<&'a &'static str> for Arguments<'a> #257

EFanZh opened this issue Aug 6, 2023 · 7 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@EFanZh
Copy link

EFanZh commented Aug 6, 2023

Proposal

Problem statement

Implement From<&'a &'static str> for Arguments<'a>.

Motivating examples or use cases

The log! macro from the log crate once had an optimization for calls with a string literal, but later the optimization was removed to support capturing variables in log messages (see rust-lang/log#446).

The idea is that constructing an Arguments object is more costly than constructing an &str object, so in order to reduce the binary size, the log! macro could dispatch the call to two different functions based on whether the message is a string literal:

fn __private_api_log_1(args: &'static str, ...) { ... }
fn __private_api_log_2(args: Arguments, ...) { ... }

macro_rules! log {
    // If the argument is a string literal,
    // call `__private_api_log_1`,
    // otherwise call `__private_api_log_2`.
}

I intend to bring that optimization back, the problem is that although we can construct an Arguments object from an &str object using format_args!("{s}"), the result Arguments object won’t has its as_str method returning Some(&'static str), which essentially prevents logging backend from utilizing as_str.

Solution sketch

The standard library provides a way to construct an Arguments object that returns Some(&'static str) if called on its as_str() method from an &'static str object. I think implementing From<&'a &'static str> for Arguments<'a> seems to be a reasonable thing to do this.

Alternatives

The only thing I think of is utilizing private API Arguments::new_const, but it’s not a good idea.

Links and related work

Implementation PR: rust-lang/rust#114531

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@EFanZh EFanZh added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Aug 6, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Aug 22, 2023

I'd go further and go for a conversion or constructor from &'static str, without the extra reference. That's impossible with the current implementation of fmt::Arguments, but with some tweaks we could make that work. I think it's worth supporting that.

(I haven't made any progress on rust-lang/rust#99012 lately, but I will look into making conversion from &'static str possible when I find some time to work on fmt::Arguments, hopefully soon.)

@m-ou-se
Copy link
Member

m-ou-se commented Aug 24, 2023

I will look into making conversion from &'static str possible when I find some time to work on fmt::Arguments, hopefully soon.

Did exactly that in rust-lang/rust#115129

I'm still tweaking things a bit for performance, but it's looking good. :)

@m-ou-se
Copy link
Member

m-ou-se commented Aug 24, 2023

The idea is that constructing an Arguments object is more costly than constructing an &str object

Ideally, that wouldn't be the case. I think we can, eventually, make fmt::Arguments and &str the same size (two pointers/usizes). (The highest bit of the string length is never set, because slices have a maximum length of isize::MAX. So, we can use one bit to differentiate between the trivial case (just a &'static str) and the case with placeholders and arguments. But that's still something that needs experimentation.)

@Zoxc
Copy link

Zoxc commented Sep 6, 2023

Both this API and Arguments::as_str are unreasonable restrictive and I haven't yet seen a reason to have them. There's an assumption that &str is fundamentally better for code size, which is not true.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 21, 2023

@Zoxc What do you mean? You don't see a reason for these APIs to exist, or you don't see a reason for certain restrictions? Which restrictions do you mean?

@Zoxc
Copy link

Zoxc commented Sep 22, 2023

I mean the APIs place restrictions on the implementation of Arguments for no reason.

This proposal seems want to pass a &str instead of Arguments, but that's exactly the reason Arguments::as_str was added. There's no new API needed for that.

The existence for Arguments::as_str is without (presented) motivation as there's no reason to favor &str over Arguments to reduce code size if Arguments is further optimized. This function wouldn't be as bad if it could always return None, but currently it requires a pointer to a constant str to be accessible from Arguments.

@Amanieu Amanieu added the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label Sep 17, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Sep 27, 2024

We discussed this in the libs-api meeting. We don't feel comfortable adding a stable From implementation like this while the internals of fmt::Arguments are still in flux. However, we're happy to see this added as an unstable method, so we can get some experience and feedback. (E.g. fmt::Arguments::from_str().)

Feel free to open a tracking issue and open a PR to rust-lang/rust to add this as an unstable function on fmt::Arguments.

@m-ou-se m-ou-se closed this as completed Sep 27, 2024
@m-ou-se m-ou-se removed the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants