Skip to content

Conversation

LukeMathWalker
Copy link
Contributor

@LukeMathWalker LukeMathWalker commented Jul 9, 2025

No description provided.

@LukeMathWalker LukeMathWalker changed the base branch from main to idiomatic-rust July 9, 2025 17:07
@LukeMathWalker LukeMathWalker changed the base branch from idiomatic-rust to main July 9, 2025 19:03
@LukeMathWalker LukeMathWalker force-pushed the extension-traits branch 2 times, most recently from 9ad9fa7 to 9b50d50 Compare July 9, 2025 19:12
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

looking good!

@LukeMathWalker LukeMathWalker marked this pull request as ready for review July 14, 2025 15:37
@LukeMathWalker LukeMathWalker requested a review from djmitche July 14, 2025 17:20
Copy link
Collaborator

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

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

This looks good! I only have a couple of minor comments, but on the whole I like it :)

LukeMathWalker and others added 4 commits July 15, 2025 14:38
…nding-foreign-types.md

Co-authored-by: Dmitri Gribenko <[email protected]>
…od-resolution-conflicts.md

Co-authored-by: Dmitri Gribenko <[email protected]>
…od-resolution-conflicts.md

Co-authored-by: Dmitri Gribenko <[email protected]>
…od-resolution-conflicts.md

Co-authored-by: Dmitri Gribenko <[email protected]>
Comment on lines 43 to 44
- The extended trait may, in a newer version, add a new trait method with the
same name of our extension method.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The extended trait may, in a newer version, add a new trait method with the
same name of our extension method.
- Another extension trait may, in a newer version, add a new trait method with the
same name as our extension method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both cases are actually possible, so I reworded the paragraph to account for both in 17ba065

@LukeMathWalker LukeMathWalker requested a review from gribozavr July 31, 2025 15:17
Copy link
Collaborator

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

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

I think we might want to change the examples in this section to not be on &str, since implementing a trait on a reference leads to confusing behavior around method resolution (which I talk about in more detail in one of the comments here). I think things would be less confusing if we used a non-reference type like i32 or struct Foo.

Comment on lines +53 to +55
Now `StrExt::trim_ascii` is invoked, rather than the inherent method, since
`&mut self` has a higher priority than `&self`, the one used by the inherent
method.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is an accurate explanation of what's happening here. The reference explicitly states (in the info box in that section) that &self methods have higher priority than &mut self methods.

I think the reason why the &mut self version gets higher priority here is that the receiver expression is &mut &str. If I'm understanding the reference's explanation of method resolution correctly, this means that when it builds the list of candidate receiver types, &mut &str is the first candidate type in the list. It's then choosing between the inherent &str method and the &mut &str method coming from the trait, and the latter wins because it's the actual type of the expression &mut " dad ".

I think the confusion here is because we're implementing the trait on on &str, which is already a reference type. If I change the trait to be implemented on str directly (i.e. impl StrExt for str), when when I change the method to take &mut self the inherent method still gets called (exmple in the playground). Part of the reason for this is because when we do (&mut " dad ") we're not getting a &mut str, we're getting a &mut &str.

I think things would be a lot less ambiguous if we were demonstrating this on a regular, non-reference type such as i32 or struct Foo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking at it closely!
Re-reading through it, and cross-referencing with the RFC, I agree with your interpretation as to why things play out as they do in terms of precedence. I'll rework the example to something simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, I'd be happy if we explained a basic rule that people can remember, and simply mention that while the Rust language has rules to disambiguate in other cases, the rules are quite complex to remember and apply, and we'd rather not write code that depends on them.

Comment on lines 52 to 60
- The compiler rejects the code because it cannot determine which method to
invoke. Neither `Ext1` nor `Ext2` has a higher priority than the other.

To resolve this conflict, you must specify which trait you want to use. For
example, you can call `Ext1::is_palindrome("dad")` or
`Ext2::is_palindrome("dad")`.

For methods with more complex signatures, you may need to use a more explicit
[fully-qualified syntax][1].
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably worth showing the full syntax to students, since sometimes Trait::method(foo) isn't enough. Specifically, if the compiler can't infer the type of foo then it won't be able to resolve which type's trait implementation to use. In those cases you'd have to write <Type as Trait>::method(foo). That syntax can be surprising for people new to the language (I know I was confused the first time I saw it), so I think showing it here (and explaining when it's necessary) would be good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, that would be a good thing for the instructor to demonstrate. Please add a bullet point with the prompt for the instructor.

Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

This looks great to me. The note below is stylistic and relates only to the speaker notes, so feel free to treat it as a mild preference at most.

Comment on lines 52 to 60
- The compiler rejects the code because it cannot determine which method to
invoke. Neither `Ext1` nor `Ext2` has a higher priority than the other.

To resolve this conflict, you must specify which trait you want to use. For
example, you can call `Ext1::is_palindrome("dad")` or
`Ext2::is_palindrome("dad")`.

For methods with more complex signatures, you may need to use a more explicit
[fully-qualified syntax][1].
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, that would be a good thing for the instructor to demonstrate. Please add a bullet point with the prompt for the instructor.

@gribozavr
Copy link
Collaborator

@LukeMathWalker I left another round of comments, but please don't be discouraged - this section looks very good and the comments are mostly minor. Please work through resolving the comments, rebase the PR to resolve conflicts with main, and merge. You have my LGTM.

@gribozavr gribozavr added the waiting for author Waiting on the issue author to reply label Sep 21, 2025
@tall-vase tall-vase self-assigned this Oct 13, 2025
Co-authored-by: Dmitri Gribenko <[email protected]>
Co-authored-by: Nicole L <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deep_dives/idiomatic waiting for author Waiting on the issue author to reply

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants