[rustdoc] Unify type aliases rendering with other ADT #140863
Conversation
rustbot
added
A-rustdoc-json
There was a problem hiding this comment.
I hope this is helpful, it's mostly a bunch of nitpicks, I didn't find any major issues (besides the witout typo), feel free to disregard things if you believe you have a good reason to do things your way, though I would appreciate a brief explanation so I can try to provide better feedback in the future.
src/librustdoc/clean/types.rs
Outdated
| @@ -759,22 +762,19 @@ impl Item { | |||
| Some(tcx.visibility(def_id)) | |||
| } | |||
|
|
|||
| pub(crate) fn attributes(&self, tcx: TyCtxt<'_>, cache: &Cache, is_json: bool) -> Vec<String> { | |||
| pub(crate) fn attributes_witout_repr(&self, tcx: TyCtxt<'_>, is_json: bool) -> Vec<String> { | |||
There was a problem hiding this comment.
| pub(crate) fn attributes_witout_repr(&self, tcx: TyCtxt<'_>, is_json: bool) -> Vec<String> { | |
| /// get a list of every attribute except `#[repr(...)]`. | |
| pub(crate) fn attributes_without_repr(&self, tcx: TyCtxt<'_>, is_json: bool) -> Vec<String> { |
Typo, also add a small doc comment to make things more immediately obvious.
There was a problem hiding this comment.
Thanks for the typo! In this case, the doc comment doesn't provide extra information in addition to the function name, so not sure it's worth adding.
There was a problem hiding this comment.
it's a niche clarification, making it clear that it excludes attributes that are repr, not that have repr, but there's no currently extant attribute forms i can think of where the latter interpretation would make sense, so it should be fine either way.
| @@ -2351,6 +2371,16 @@ pub(crate) enum TypeAliasInnerType { | |||
| Struct { ctor_kind: Option<CtorKind>, fields: Vec<Item> }, | |||
| } | |||
|
|
|||
| impl TypeAliasInnerType { | |||
| fn has_stripped_entries(&self) -> Option<bool> { | |||
There was a problem hiding this comment.
Wouldn't this make more sense as has_stripped_variants? Am I missing something?
Ofc if we're doing that we should rename the other has_stripped_entries for consistency, I think that seems pretty in line with all the other refactoring going on here.
There was a problem hiding this comment.
Maybe I'm the one who missed something. 😆
A type alias can be an alias of an enum, a union or a struct. Of these three, 2 have fields and one has variants. So in this case, neither "fields" nor "variants" can be used.
There was a problem hiding this comment.
ah, you're right. really it's the other has_stripped_entries that look odd, since in those cases we do actually know if its a field or variant. i guess it makes sense to keep the names the same, and i suppose the function body is simple enough it doesn't need a doc comment.
src/librustdoc/clean/types.rs
Outdated
| Self::Union { fields } => fields.iter().any(|f| f.is_stripped()), | ||
| Self::Struct { fields, .. } => fields.iter().any(|f| f.is_stripped()), |
There was a problem hiding this comment.
| Self::Union { fields } => fields.iter().any(|f| f.is_stripped()), | |
| Self::Struct { fields, .. } => fields.iter().any(|f| f.is_stripped()), | |
| Self::Union { fields } | | |
| Self::Struct { fields, .. } => fields.iter().any(|f| f.is_stripped()), |
unfortunately variants is a different type, but we can at least unify these two branches.
| let def_id = it.item_id.expect_def_id(); | ||
| let layout_def_id = if !is_type_alias { | ||
| write!(w, "{}", document(cx, it, None, HeadingOffset::H2))?; | ||
| def_id | ||
| } else { | ||
| self.def_id | ||
| }; |
There was a problem hiding this comment.
Nit: def_id could be raised into the scope of the if or inlined entirely, which would save a few branches and maybe be easier to read.
the ternary with side effect pattern is a bit difficult to read, especially with the negated condition, perhaps a brief comment explaining things would help (in particular, why are we using a different DefId in the type alias case).
There was a problem hiding this comment.
Gonna add a comment but for posterity: since we have extra generics information, the size of the type layout might change.
| struct DisplayEnum<'a> { | ||
| variants: &'a IndexVec<VariantIdx, clean::Item>, | ||
| generics: &'a clean::Generics, | ||
| is_non_exhaustive: bool, | ||
| def_id: DefId, | ||
| } | ||
|
|
||
| impl<'a> DisplayEnum<'a> { |
There was a problem hiding this comment.
Nit: could we use a more descriptive lifetime name, such as 'clean, since this lifetime seems to be bound to that of the cleaned ast ?
| variants: &e.variants, | ||
| generics: &e.generics, | ||
| is_non_exhaustive: it.is_non_exhaustive(), | ||
| def_id: it.def_id().unwrap(), |
There was a problem hiding this comment.
earlier you replaced it.def_id().unwrap() with e.def_id (or self.def_id), is there a reason you're not doing that again here?
There was a problem hiding this comment.
It's the original code behaviour. Keeping it as is, although it's curious indeed.
| } | ||
| fn render_attributes_in_pre(&self) -> impl fmt::Display { | ||
| fmt::from_fn(move |f| { | ||
| if !self.is_type_alias { |
There was a problem hiding this comment.
Is there a reason for all the inverted if conditions? if you're trying to do branch predictor hints based on what the "primary" branch is, I don't believe that's a thing in rust, and indeed with opt-level=3 I got identical asm no matter which way it was written.
There was a problem hiding this comment.
No, it's often because of how I originally wrote the code (ie no else yet) and I didn't update afterward. Gonna switch both branches.
| if !is_type_alias { | ||
| render_attributes_in_code(w, it, cx); | ||
| } else { | ||
| // For now we only render `repr` attributes for type aliases. |
There was a problem hiding this comment.
| // For now we only render `repr` attributes for type aliases. | |
| // For now the only attributes we render for type aliases are `repr` attributes. |
same nit again
GuillaumeGomez
force-pushed
the
cleanup-tyalias-render
branch
from
cb63d99 to
509bff1
Compare
|
Applied suggestions! |
|
☔ The latest upstream changes (presumably #141437) made this pull request unmergeable. Please resolve the merge conflicts. |
Fixes #140739.
Better reviewed one commit at a time.
Just one thing I'm wondering: should we also render non-
reprattributes? If so, I wonder if we shouldn't simply changeclean::TypeAliasto contain the other ADT directly (Struct,EnumandUnion) and remove theTypeAlias::genericsfield.Can be done in a follow-up too.
cc @camelid
r? @notriddle