impl Add<char> and AddAssign<char> for String #66504
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Oh, I forgot to mention that, sorry. This is easily fixed by Another meta-hint on the side: if in question about what to do, just ask (as you did) and give us a day or two to answer. From personal experience I know very well that it can often be frustrating trying to figure everything out on your own, especially given the long compile times and complex test system. And since others can often tell you what you have to do, that avoids a lot of that frustration. Regarding the test failures found in CI: those are UI tests. You might fix them all by just looking at the output that @rust-highfive posted. But if you want to test locally, try this |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@Centril @KodrAus @jonas-schievink Can we move this forward please? |
|
r? @dtolnay |
Centril
added
relnotes
|
21 hours and still no review. @Centril What to do? |
|
@Ruster-a11y Don't worry, this is completely normal. It usually takes a few days (sometimes weeks) until the reviewer gets active. Also, just as a hint: I'm sure that is not your intention at all, but some people might interpret your comments as trying to rush people or to tell people they are too slow. And we wouldn't want people to feel bad/angry because of that. As an open source contributor you have to have some patience. So let's just wait :) |
|
☔ The latest upstream changes (presumably #66389) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
I expect that this would still break a large amount of code even with the extra impls for &String and &&String. :(
This sort of thing compiles today and applies the right chain of derefs to invoke impl Add<&str> for String:
let lhs = String::new();
let rhs = String::new();
let _ = lhs + &&&&&&&rhs;Nobody would write it exactly like this, but I have found that especially beginners and especially with "match ergonomics" it's very easy to end up with a surprising number of & without realizing.
fn f(input: Option<&str>) {
let mut string = String::new();
match &input {
None => {}
Some(ref input) => {
string += &input; // already dealing with &&&str
}
}
}Beginners often end up with this type of code because every part of this locally looks like something they have seen before or copied from somewhere.
Option<&str>-- commonmatch &input {...}-- commonSome(ref input)-- commonstring += &input-- common
Is there anything like impl<T> Add<&&T> for String we could do to keep existing code working regardless of the number of references?
src/test/ui/span/issue-39018.stderr
Outdated
| LL | let _ = a + b; | ||
| | ^ | ||
| | | | ||
| | expected &str, found struct `std::string::String` | ||
| | help: consider borrowing here: `&b` | ||
| | ^ no implementation for `std::string::String + std::string::String` |
There was a problem hiding this comment.
This would be an unfortunate error message regression. It doesn't necessarily mean we can't add the impl, but we would definitely want to follow up with a dedicated diagnostic to show that the user should write a + &b.
There was a problem hiding this comment.
I talked to Ruster-a11y about this and suggested they ignore it for now as a better error message can still be added later. Or do you think we should have a better error message in this PR already? My reasoning was that we first have to decide on the whole "breakage" situation anyway. Implementing the error message beforehand might be a waste of time.
I could have a go at implementing the better error message, if Ruster-a11y does not want to/does not have the time to.
There was a problem hiding this comment.
It doesn't need to be in this PR but it would be good to have something underway, so that we don't end up with the worse message for years. The code that the original message is explaining exactly how to fix is one of the things that many beginners struggle with.
There was a problem hiding this comment.
Absolutely! I'd say that the new impl and the better error message should hit stable at the same time. So absolutely not "years" in between.
Great idea! It indeed seems to work. I implemented it with While it's good to know that we could add this impl to avoid all breakage, I still prefer not to add it, I think. @dtolnay do you think we can simply run crater on this PR? |
|
@bors try |
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
|
Looks like this will need a rebase to get a crater going. Rather than building on AsRef, would it maybe be viable to do it this way?: impl<'a, T> Add<&&'a T> for String
where
String: Add<&'a T>This seems safer because the only impls it could conflict with are ones that we would never want to behave a different way, i.e. we wouldn't want a String + &&T impl that isn't just the same as String + &T. I tried it briefly in your playground but the trait solver goes nuts, though naively it seems like the sort of thing that should be possible to make work. |
…s per what compiler produces now.
per previous upstream changes.
Ruster-a11y
force-pushed
the
impl-char-add-operation-for-String2
branch
from
77e1329
to
b7b8eed
Compare
That's what I tried first, and not so briefly. But I also always got the recursion limit error from the trait solver. No idea why it's happening as it seems like we are never adding a But maybe crater results will be nice, so we don't have to worry about that impl ;-) |
craterbot
added
the
S-waiting-on-review
|
There are ~60 root regressions based on crater-generate-report: https://gist.github.com/Mark-Simulacrum/3139e9307eaa63eb37cea965465de384#file-roots-pr-66504-md. I think given the scope of the failures here we probably can't land this -- it poses too large a breaking change for very little benefit. My earlier comment I think may provide a plausible path forward, but it may make sense to just open a different PR for that approach. |
@Mark-Simulacrum Alright, this sounds like a good idea. I'll start working on this, and let's see how that plays out. Hopefully, it should work out, it makes a lot more sense than the approach taken in this PR. |
|
I'm not a big fan of the inherent |
|
@LukasKalbertodt Yep, makes sense. I'll add a working commit at the earliest, might do it using If further additions don't play well in the Crater tool test, we can then move on to the |
Added another layer of & prefixed impls.
|
Is this ready for another try + crater? @LukasKalbertodt I agree that It's also interesting to note that at least hypothetically, |
I think so, yes. For reference:
Testing the 1180 regression should only take an hour at most, I guess. So we can probably test all of them for different configurations as well (i.e. with the |
We can spawn a shorter crater run with that, but I think we should actually do a full crater run, right? In particular, the changes may uncover breakage in crates that weren't detected as broken by this crater run. In any case though, @bors try -- that way we can get to a crater run more quickly regardless of which type we choose. |
|
⌛ Trying commit fca8180 with merge 1a608893c03d7722effeb0058e82770bd36b4b3c... |
|
☀️ Try build successful - checks-azure |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
removed
the
S-waiting-on-review
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
the
S-waiting-on-crater
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Oof, that's still a lot of regressions. Skimming through the results shows a few patterns:
There are also some common root regressions ( The Unsatisfying situation :/ |
Dylan-DPC-zz
added
S-waiting-on-author
|
Ping from Triage: Any updates @Ruster-a11y? |
|
I think we've explored these Add impls pretty thoroughly and all the approaches have turned out to be too disruptive for not a great amount of benefit, so I'll go ahead and close the PR. Thanks anyway! I would recommend that instead of String + char, that people make a local variable for the String and then .push(...) chars into it. The idea of |
Centril
removed
the
relnotes
This is the new working code for the functionalities mentioned in the title. This also means the previous pull request ( #66215) can now be relegated to just as the implementation for
AddAssign<char> for Cow<'_, str>.This makes the following operations possible:
These further code modifications were needed due to the already open issue #51916. Since that issue is beyond the scope for this PR and feature-set, this commit only attempts at getting around that issue by further adding implementations for:
impl Add<&&str> for Stringimpl Add<&String> for Stringimpl Add<&&String> for Stringimpl AddAssign<&String> for StringWithout these
impladditions, the rust compiler complains about lacking the respective implementations forStringoperations sinceimpl Add<char> for Stringadds a layer of ambiguity for the compiler.@LukasKalbertodt, thanks for your suggestions! (#66215 (comment))
Note:
Build only tested on latest Windows 10.
Earlier PR #66490 has been closed since commits had unforeseen changes from submodules and all my rebase/cherry-pick attempts failed to fix that.