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

rustc suggests calling to_string() on &&str but this generates inefficient code. #128690

Closed
ianloic opened this issue Aug 5, 2024 · 2 comments · Fixed by #128759
Closed

rustc suggests calling to_string() on &&str but this generates inefficient code. #128690

ianloic opened this issue Aug 5, 2024 · 2 comments · Fixed by #128759
Labels
C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ianloic
Copy link

ianloic commented Aug 5, 2024

If I write some code like:

fn stringify(s: &&str) -> String {
    s
}

then rustc will say something like:

1 | fn stringify(s: &&str) -> String {
  |                           ------ expected `String` because of return type
2 |     s
  |     ^- help: try using a conversion method: `.to_string()`
  |     |
  |     expected `String`, found `&&str`

(see: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c01bf377e97a0b1c90bd374ba90663a6)

But ToString::to_string() for &&str uses the default implementation which goes via fmt.

Even with optimization this is far less efficient than dereferencing the value first (eg: https://godbolt.org/z/rEoo1qv6x)

Ideally we'd have some better combination of an efficient to_string() override and better advice in error messages.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 5, 2024
@the8472
Copy link
Member

the8472 commented Aug 5, 2024

ToString already uses specialization (but it's not a specialization trait), it might be possible to add one for that case, though probably not a recursive one.

@workingjubilee
Copy link
Member

It seems to me that rustc's suggestions are nonetheless an optimization over the status quo of the proposed original source code, as

fn stringify(s: &&str) -> String {
    s
}

generates this assembly

and a compiler error.

I am surprised that there is no other codegen issue with which this is a duplicate. I do not think we are going to predicate our diagnostics on the resulting codegen, either, as we cannot predict such, because we expose codegen settings that alter how well something will perform.

@workingjubilee workingjubilee added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-heavy Issue: Problems and improvements with respect to binary size of generated code. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 14, 2024
…, r=workingjubilee,compiler-errors

alloc: add ToString specialization for `&&str`

Fixes rust-lang#128690
@bors bors closed this as completed in 1b587a6 Aug 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 14, 2024
Rollup merge of rust-lang#128759 - notriddle:notriddle/spec-to-string, r=workingjubilee,compiler-errors

alloc: add ToString specialization for `&&str`

Fixes rust-lang#128690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants