-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Document fully-qualified syntax in as
' keyword doc
#142670
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
Document fully-qualified syntax in as
' keyword doc
#142670
Conversation
r? @ibraheemdev
rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r?
to explicitly pick a reviewer
7cfd0dd
to
a2d247b
Compare
library/std/src/keyword_docs.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other std traits where this is commonly needed? A small downside with this example is that this is something that should just be written u32::from(my_thing)
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be a thing with Debug vs Display::fmt. That said, <Self as fmt::Debug>::fmt(self, f)
is semantically identical, longer, and weirder than fmt::Debug::fmt(self, f)
. io::Write::write_fmt
and fmt::Write::write_fmt
have the same problem.
Into<T>
is nice since it's reasonably likely that users will encounter a message from rustc that they should use the fully-qualified path as in these docs (though by all rights, it should suggest that you should use T::from(self)
).
An other option would be to make a contrived exampled like is done in the Rust book. E.g.,
struct Person; trait MetalHead { fn head_bang(); } trait SalaryWorker { fn head_bang(); } impl MetalHead for Person { ... } impl SalaryWorker for Person { ... } // 9 to 5 <Person as SalaryWorker>::head_bang(); // 5 to 9 <Person as MetalHead>::head_bang();
But of course coming up with a more useful example is hard since associated items don't usually overlap and there's usually sufficient type information to remove the ambiguity this doc is trying to highlight. The optimal example would be either a pair of trait methods that return a generic type like Into::<T>::into
, or a pair of static trait methods. Short of what's here already, I can't find anything like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it so there's a footnote saying that it'd be better to use T::from(my_thing)
and that the example is imperfect.
library/std/src/keyword_docs.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make this change, but isn't this a bit less useful? As it was, it tells you exactly which syntactic objects this syntax can be used with, whereas the change demands that you know about the associated item abstraction. Realistically, if I'm someone who's reading the keyword docs, I'm probably a rust beginner and probably not familiar with this abstraction.
Perhaps "a means of clarifying ambiguous associated items, i.e. methods, constants, and types. For example," would be a good compromise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm not the primary reviewer) if you end up listing the three kinds of associated item, please substitute method with function or with function (includes methods). Methods are only a subset of associated functions.
r=me after the nit.
054ee36
to
15f73ab
Compare
@bors r+ rollup
library/std/src/keyword_docs.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Revisiting this after seeing #143098)
This paragraph is placed in the middle of the as
casts explainer. E.g., below it continues to talk about as _
which is confusing and disrupts the flow. Please move it to the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, I think we should quickly mention associated items in the synopsis / introductory paragraph to give this more structure.
library/std/src/keyword_docs.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
library/std/src/keyword_docs.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bors r-
15f73ab
to
c7fd35f
Compare
c7fd35f
to
0de43e0
Compare
r? fmease
fmease
is currently at their maximum review capacity.
They may take a while to respond.
@fmease Hey I just want to politely check in on when this can get a review. No problem if you're strapped for time.
Sorry about that, I'll take a look later tonight!
No description provided.