Rustdoc-Json: Always put enum fields in their own item #100762
Conversation
rustbot
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
|
@rustbot modify labels: +A-rustdoc-json |
aDotInTheVoid
force-pushed
the
rdj-enum-field-item
branch
from
2f43769
to
c328910
Compare
| /// | ||
| /// ```rust | ||
| /// pub struct A(i32); | ||
| /// pub struct B(); |
There was a problem hiding this comment.
I actually didn't know it was a thing. :o
aDotInTheVoid
force-pushed
the
rdj-enum-field-item
branch
2 times, most recently
from
e680554
to
ed9b778
Compare
| fields_stripped: fields.iter().any(|i| i.is_stripped()), | ||
| }, | ||
| Struct(s) => Variant { | ||
| kind: StructKind::NamedFields, |
There was a problem hiding this comment.
Just thought about it: NamedFields sounds a bit weird. Wouldn't it be better to have something similar to what already exists? For example VariantData.
There was a problem hiding this comment.
The problem with that is having struct.kind == StructKind::Struct isn't informative, as all 3 kinds can be "structs".
Another option is to use StructKind::Normal, as most structs use {}, but thats not the "normal" option for enum variants, which are mostly tuple or unit.
While it's not the name used in compiler internals, we haven't done that for other parts of the output (eg using type_ instead of ty.
There was a problem hiding this comment.
Multiplying the number of terms isn't the best idea either but I'm not great for naming... Does it seem good to you @Manishearth ?
| Tuple(fields) => Variant { | ||
| kind: StructKind::Tuple, | ||
| fields: ids(&fields, tcx), | ||
| fields_stripped: fields.iter().any(|i| i.is_stripped()), |
There was a problem hiding this comment.
I've been experimenting with this code (see cargo-public-api/cargo-public-api#99) and have the following question/concern: What is the value of having a bool to specify if some field is stripped? Wouldn't it be better to remove fields_stripped and instead add info on a per-field level if it is stripped or not? Later today I think I will have time to develop a concrete proposal on how it could look.
There was a problem hiding this comment.
I think the idea is to not show stripped fields and just say more fields exist but they're not part of the public API.
There was a problem hiding this comment.
I think it would be good to have higher fidelity than "more fields exist" and also be able to easily say which fields have been stripped. In other words, be on par with rustdoc HTML output, which looks like this:
pub enum EnumWithStrippedTupleVariants {
Double(bool, bool),
DoubleFirstHidden(_, bool),
DoubleSecondHidden(bool, _),
}There was a problem hiding this comment.
Some options for this:
1: Also give the number of fields. The relative position of fields can be determined by the number in the name for tuple, and not at all for structs.
2: Instead of having a Vec<Id>, have a Vec<Option>, and use None` for stripped fields.
There was a problem hiding this comment.
2 sounds good! I haven't had time to experiment but that should work well.
Not a fan of 1 because names having special meaning seems too fragile for an API.
|
☔ The latest upstream changes (presumably #100678) made this pull request unmergeable. Please resolve the merge conflicts. |
aDotInTheVoid
force-pushed
the
rdj-enum-field-item
branch
from
ed9b778
to
6ca1ee3
Compare
|
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing |
| Tuple(Vec<Type>), | ||
| Struct(Vec<Id>), | ||
| pub struct Variant { | ||
| pub kind: StructKind, |
There was a problem hiding this comment.
I think we need a separate enum for variant kinds and struct kinds, as unit variants may have a discriminant value, which currently we don't document but should. See cargo-public-api/cargo-public-api#121 and zulip discussion
|
☔ The latest upstream changes (presumably #101386) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
…GuillaumeGomez Rustdoc-Json: Store Variant Fields as their own item. Closes rust-lang#100587 Closes rust-lang#92945 Successor to rust-lang#100762 Unlike that one, we don't have merge `StructType` and `Variant`, as after rust-lang#101386 `Variant` stores enum specific information (discriminant). Resolves the naming discussion (rust-lang#100762 (comment)) by having seperate enums for struct and enum kinds Resolves `#[doc(hidden)]` on tuple structs (rust-lang#100762 (comment)) by storing as a `Vec<Option<Id>>` r? `@GuillaumeGomez`
|
Since #101462 that succeeded this PR has been merged, I think this PR can be flat-out closed now? |
Closes #100587
Closes #92945
See those issues for motivation
r? @GuillaumeGomez