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

Reference to compiler warning is slightly outdated in chapter 15.5 #4140

Closed
matthias-t opened this issue Dec 5, 2024 · 2 comments · Fixed by #4142
Closed

Reference to compiler warning is slightly outdated in chapter 15.5 #4140

matthias-t opened this issue Dec 5, 2024 · 2 comments · Fixed by #4142

Comments

@matthias-t
Copy link

  • I have searched open and closed issues and pull requests for duplicates, using these search terms:
    • milestone:ch15
  • I have checked the latest main branch to see if this has already been fixed, in this file:
    • src/ch15-05-interior-mutability.md

URL to the section(s) of the book with this problem:

We can’t modify the `MockMessenger` to keep track of the messages, because the
`send` method takes an immutable reference to `self`. We also can’t take the
suggestion from the error text to use `&mut self` instead, because then the
signature of `send` wouldn’t match the signature in the `Messenger` trait
definition (feel free to try and see what error message you get).

Description of the problem: Rust 1.82.0 (and possibly earlier) suggests changing the trait definition as well as the impl method, as below. This still would not compile, but the reason now becomes that messenger is contained as an immutable reference in LimitTracker, and so cannot be passed to send.

error[E0596]: cannot borrow `self.sent_messages` as mutable, as it is behind a `&` reference
  --> src/sec_5.rs:62:13
   |
62 |             self.sent_messages.push(message.to_string());
   |             ^^^^^^^^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable
   |
help: consider changing this to be a mutable reference in the `impl` method and the `trait` definition
   |
6  ~     fn send(&mut self, message: &str);
7  | }
...
60 |     impl Messenger for MockMessenger {
61 ~         fn send(&mut self, message: &str) {
   |

For more information about this error, try `rustc --explain E0596`.

Suggested fix: Perhaps change the second sentence to: "We also can’t take the suggestion from the error text to use &mut self instead, because our LimitTracker only holds an immutable reference to a Messenger instance (feel free to try and see what error message you get)." Or, to adress the concern that this might be changed in turn: "We could follow the compiler's suggestions to work with mutable references instead, but here we'll assume that we want the send method in our library to accept immutable references."

@chriskrycho
Copy link
Contributor

It looks like this change landed in 1.79. I think the sentence could indeed use a tiny bit of tweaking, but I think in keeping with the actual flow of the text here, the point should be that we shouldn't be changing the trait definition for the sake of test code. I’ll open a small PR to that effect!

chriskrycho added a commit that referenced this issue Dec 6, 2024
Previously, the error message from the compiler only suggested changing
the `impl` to use `&mut self`, but a (very helpful!) update to the error
message suggests changing both the `impl` and the `trait` definitions,
which is much better in the general case since changing one without the
other will just produce a new error message: a point the original text
points to with its suggestion to try and see what happens.

Account for the new error message by keeping the framing of the reason
as avoiding breaking the trait contract, but now explicitly in terms of
good testing habits.

Fixes #4140
@matthias-t
Copy link
Author

Thanks! 🙂 Looks good to me.

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 a pull request may close this issue.

2 participants