[rustdoc] Detect and resolve ambiguities in fn parameters type names#122872
[rustdoc] Detect and resolve ambiguities in fn parameters type names#122872krtab wants to merge 6 commits intorust-lang:masterfrom
Conversation
rustbot
added
S-waiting-on-review
src/librustdoc/html/format.rs
Outdated
| .map(|rcb| rcb.get()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
| .map(|rcb| rcb.get()) | |
| .unwrap_or(false); | |
| .is_some_and(|rcb| rcb.get()); |
src/librustdoc/html/format.rs
Outdated
| ) -> FxHashMap<DefId, Rc<Cell<bool>>> { | ||
| fn inner( | ||
| ty: &clean::Type, | ||
| res_map: &mut FxHashMap<DefId, Rc<Cell<bool>>>, |
There was a problem hiding this comment.
Why do you need to have Rc<Cell<bool>> instead of just a bool?
fmease
changed the title
|
☔ The latest upstream changes (presumably #123725) made this pull request unmergeable. Please resolve the merge conflicts. |
| self.add_fndecl(&f.decl); | ||
| } | ||
|
|
||
| pub fn finnish(self) -> AmbiguityTable<'a> { |
There was a problem hiding this comment.
Northern countries are now part of rust. 😄 ("finnish" with two "n")
|
|
||
| pub fn build_fn(f: &'a Function) -> Self { | ||
| let mut builder = Self::build(); | ||
| builder.add_fn(f); |
There was a problem hiding this comment.
Please directly call add_fn_decl. That will remove one method.
| } | ||
|
|
||
| #[allow(rustc::pass_by_value)] | ||
| pub fn get(&self, x: &DefId) -> Option<&PathLike<'a>> { |
There was a problem hiding this comment.
Should really not be a reference (DefId).
| @@ -0,0 +1,202 @@ | |||
| use rustc_data_structures::fx::FxHashMap; | |||
There was a problem hiding this comment.
Might be worth adding a comment explaining how the path ambiguities are handled at the top of the file.
| indent: usize, | ||
| cx: &Context<'_>, | ||
| ) { | ||
| let at = AmbiguityTable::empty(); |
There was a problem hiding this comment.
A lot of AmbiguityTables are created way too "low" in the tree call. For example in here, an associated constant is part of an impl or a trait, so the AmbiguityTable should have been already created much earlier. I think you can simply create one AmbiguityTable in html/render/context.rs::fn render_item and then pass it down everywhere.
| } | ||
|
|
||
| // @has fn_param_ambiguities/fn.f.html //pre 'pub fn f(xa: X::A, ya: Y::A, yb: B, xc: C)' | ||
| pub fn f(xa: X::A, ya: Y::A, yb : Y::B, xc: X::C) {} |
There was a problem hiding this comment.
Please add struct and other types with impls and do the same for their associated items (like constants, methods and types).
Dylan-DPC
added
S-waiting-on-author
|
@krtab any updates on this? thanks |
|
Closing this based on the comment #42066 (comment) |
Dylan-DPC
removed
the
S-waiting-on-author
5 participants
Possible improvements could be
A more general way of doing it would add a notion of context to the formatting which keeps track of what symbols are currently being rendered and need disambiguation, but this is a much more ambitious work. EDIT: actually, a simple notion of context is not enough. This needs to be done in two passes. The first one identifies for each symbol its shorter non ambiguous path suffix, and the second does the actual printing.
Closes: #122673
r? @GuillaumeGomez