Skip to content

Rustdoc-Json: Always put enum fields in their own item #100762

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/etc/check_missing_items.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,7 @@ def check_type(ty):
set(item["inner"]["variants"]) | set(item["inner"]["impls"])
) - visited
elif item["kind"] == "variant":
if item["inner"]["variant_kind"] == "tuple":
for ty in item["inner"]["variant_inner"]:
check_type(ty)
elif item["inner"]["variant_kind"] == "struct":
work_list |= set(item["inner"]["variant_inner"]) - visited
work_list |= set(item["inner"]["fields"]) - visited
elif item["kind"] in ("function", "method"):
check_generics(item["inner"]["generics"])
check_decl(item["inner"]["decl"])
Expand Down
66 changes: 26 additions & 40 deletions src/librustdoc/json/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,12 +238,12 @@ fn from_clean_item(item: clean::Item, tcx: TyCtxt<'_>) -> ItemEnum {

match *item.kind {
ModuleItem(m) => {
ItemEnum::Module(Module { is_crate, items: ids(m.items, tcx), is_stripped: false })
ItemEnum::Module(Module { is_crate, items: ids(&m.items, tcx), is_stripped: false })
}
ImportItem(i) => ItemEnum::Import(i.into_tcx(tcx)),
StructItem(s) => ItemEnum::Struct(s.into_tcx(tcx)),
UnionItem(u) => ItemEnum::Union(u.into_tcx(tcx)),
StructFieldItem(f) => ItemEnum::StructField(f.into_tcx(tcx)),
StructFieldItem(f) => ItemEnum::Field(f.into_tcx(tcx)),
EnumItem(e) => ItemEnum::Enum(e.into_tcx(tcx)),
VariantItem(v) => ItemEnum::Variant(v.into_tcx(tcx)),
FunctionItem(f) => ItemEnum::Function(from_function(f, header.unwrap(), tcx)),
Expand Down Expand Up @@ -282,7 +282,7 @@ fn from_clean_item(item: clean::Item, tcx: TyCtxt<'_>) -> ItemEnum {
match *inner {
ModuleItem(m) => ItemEnum::Module(Module {
is_crate,
items: ids(m.items, tcx),
items: ids(&m.items, tcx),
is_stripped: true,
}),
// `convert_item` early returns `None` for stripped items we're not including
Expand All @@ -301,10 +301,10 @@ impl FromWithTcx<clean::Struct> for Struct {
let fields_stripped = struct_.has_stripped_entries();
let clean::Struct { struct_type, generics, fields } = struct_;
Struct {
struct_type: from_ctor_kind(struct_type),
kind: from_ctor_kind(struct_type),
generics: generics.into_tcx(tcx),
fields_stripped,
fields: ids(fields, tcx),
fields: ids(&fields, tcx),
impls: Vec::new(), // Added in JsonRenderer::item
}
}
Expand All @@ -317,17 +317,17 @@ impl FromWithTcx<clean::Union> for Union {
Union {
generics: generics.into_tcx(tcx),
fields_stripped,
fields: ids(fields, tcx),
fields: ids(&fields, tcx),
impls: Vec::new(), // Added in JsonRenderer::item
}
}
}

pub(crate) fn from_ctor_kind(struct_type: CtorKind) -> StructType {
pub(crate) fn from_ctor_kind(struct_type: CtorKind) -> StructKind {
match struct_type {
CtorKind::Fictive => StructType::Plain,
CtorKind::Fn => StructType::Tuple,
CtorKind::Const => StructType::Unit,
CtorKind::Fictive => StructKind::NamedFields,
CtorKind::Fn => StructKind::Tuple,
CtorKind::Const => StructKind::Unit,
}
}

Expand Down Expand Up @@ -551,7 +551,7 @@ impl FromWithTcx<clean::Trait> for Trait {
Trait {
is_auto,
is_unsafe,
items: ids(items, tcx),
items: ids(&items, tcx),
generics: generics.into_tcx(tcx),
bounds: bounds.into_tcx(tcx),
implementations: Vec::new(), // Added in JsonRenderer::item
Expand Down Expand Up @@ -591,7 +591,7 @@ impl FromWithTcx<clean::Impl> for Impl {
.collect(),
trait_: trait_.map(|path| path.into_tcx(tcx)),
for_: for_.into_tcx(tcx),
items: ids(items, tcx),
items: ids(&items, tcx),
negative: negative_polarity,
synthetic,
blanket_impl: blanket_impl.map(|x| x.into_tcx(tcx)),
Expand Down Expand Up @@ -634,42 +634,28 @@ impl FromWithTcx<clean::Enum> for Enum {
Enum {
generics: generics.into_tcx(tcx),
variants_stripped,
variants: ids(variants, tcx),
variants: ids(&variants, tcx),
impls: Vec::new(), // Added in JsonRenderer::item
}
}
}

impl FromWithTcx<clean::VariantStruct> for Struct {
fn from_tcx(struct_: clean::VariantStruct, tcx: TyCtxt<'_>) -> Self {
let fields_stripped = struct_.has_stripped_entries();
let clean::VariantStruct { struct_type, fields } = struct_;
Struct {
struct_type: from_ctor_kind(struct_type),
generics: Generics { params: vec![], where_predicates: vec![] },
fields_stripped,
fields: ids(fields, tcx),
impls: Vec::new(),
}
}
}

impl FromWithTcx<clean::Variant> for Variant {
fn from_tcx(variant: clean::Variant, tcx: TyCtxt<'_>) -> Self {
use clean::Variant::*;

match variant {
CLike => Variant::Plain,
Tuple(fields) => Variant::Tuple(
fields
.into_iter()
.filter_map(|f| match *f.kind {
clean::StructFieldItem(ty) => Some(ty.into_tcx(tcx)),
clean::StrippedItem(_) => None,
_ => unreachable!(),
})
.collect(),
),
Struct(s) => Variant::Struct(ids(s.fields, tcx)),
CLike => Variant { kind: StructKind::Unit, fields: Vec::new(), fields_stripped: false },
Tuple(fields) => Variant {
kind: StructKind::Tuple,
fields: ids(&fields, tcx),
fields_stripped: fields.iter().any(|i| i.is_stripped()),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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, _),
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

},
Struct(s) => Variant {
kind: StructKind::NamedFields,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

fields: ids(&s.fields, tcx),
fields_stripped: s.has_stripped_entries(),
},
}
}
}
Expand Down Expand Up @@ -773,7 +759,7 @@ impl FromWithTcx<ItemType> for ItemKind {
}
}

fn ids(items: impl IntoIterator<Item = clean::Item>, tcx: TyCtxt<'_>) -> Vec<Id> {
fn ids<'a>(items: impl IntoIterator<Item = &'a clean::Item>, tcx: TyCtxt<'_>) -> Vec<Id> {
items
.into_iter()
.filter(|x| !x.is_stripped() && !x.is_keyword())
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/json/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
| types::ItemEnum::PrimitiveType(_) => true,
types::ItemEnum::ExternCrate { .. }
| types::ItemEnum::Import(_)
| types::ItemEnum::StructField(_)
| types::ItemEnum::Field(_)
| types::ItemEnum::Variant(_)
| types::ItemEnum::Function(_)
| types::ItemEnum::TraitAlias(_)
Expand Down
69 changes: 58 additions & 11 deletions src/rustdoc-json-types/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::path::PathBuf;
use serde::{Deserialize, Serialize};

/// rustdoc format-version.
pub const FORMAT_VERSION: u32 = 18;
pub const FORMAT_VERSION: u32 = 19;

/// A `Crate` is the root of the emitted JSON blob. It contains all type/documentation information
/// about the language items in the local crate, as well as info about external items to allow
Expand Down Expand Up @@ -230,7 +230,8 @@ pub enum ItemEnum {

Union(Union),
Struct(Struct),
StructField(Type),
/// The name of the field is given in the enclosing [`Item`]
Field(Type),
Enum(Enum),
Variant(Variant),

Expand Down Expand Up @@ -283,15 +284,17 @@ pub struct Module {
pub struct Union {
pub generics: Generics,
pub fields_stripped: bool,
/// Will all be [`ItemEnum::Field`]
pub fields: Vec<Id>,
pub impls: Vec<Id>,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub struct Struct {
pub struct_type: StructType,
pub kind: StructKind,
pub generics: Generics,
pub fields_stripped: bool,
/// Will all be [`ItemEnum::Field`]
pub fields: Vec<Id>,
pub impls: Vec<Id>,
}
Expand All @@ -300,24 +303,68 @@ pub struct Struct {
pub struct Enum {
pub generics: Generics,
pub variants_stripped: bool,
/// Will all be [`ItemEnum::Variant`]
pub variants: Vec<Id>,
pub impls: Vec<Id>,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
#[serde(tag = "variant_kind", content = "variant_inner")]
pub enum Variant {
Plain,
Tuple(Vec<Type>),
Struct(Vec<Id>),
pub struct Variant {
pub kind: StructKind,
Copy link
Member Author

Choose a reason for hiding this comment

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

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

/// Will all be [`ItemEnum::Field`]
pub fields: Vec<Id>,
pub fields_stripped: bool,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
pub enum StructType {
Plain,
/// The kind used for literals of a struct or enum.
pub enum StructKind {
/// A "normal" struct, with named fields.
///
/// Eg:
///
/// ```rust
/// pub struct A {
/// x: i32,
/// }
/// pub struct B {}
/// pub enum C {
/// Variant { x: i32 },
/// }
/// pub enum D {
/// Variant {},
/// }
/// ```
NamedFields,
/// Unnamed fields, accessed with a number.
///
/// Eg:
///
/// ```rust
/// pub struct A(i32);
/// pub struct B();
Copy link
Member

Choose a reason for hiding this comment

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

I actually didn't know it was a thing. :o

/// pub enum C {
/// Variant(i32),
/// }
/// pub enum D {
/// Variant(),
/// }
/// ```
Tuple,
/// No fields, and no parentheses.
///
/// Note: Cases without fields but with parentheses will have a different
/// kind, as seen in the examples above.
///
/// Eg:
///
/// ```rust
/// pub struct A;
/// pub enum B {
/// Variant,
/// }
/// ```
Unit,
}

Expand Down
2 changes: 1 addition & 1 deletion src/rustdoc-json-types/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use super::*;
#[test]
fn test_struct_info_roundtrip() {
let s = ItemEnum::Struct(Struct {
struct_type: StructType::Plain,
kind: StructKind::NamedFields,
generics: Generics { params: vec![], where_predicates: vec![] },
fields_stripped: false,
fields: vec![],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@

// @has "$.index[*][?(@.name=='ParseError')]"
// @has "$.index[*][?(@.name=='UnexpectedEndTag')]"
// @is "$.index[*][?(@.name=='UnexpectedEndTag')].inner.variant_kind" '"tuple"'
// @is "$.index[*][?(@.name=='UnexpectedEndTag')].inner.variant_inner" []
// @is "$.index[*][?(@.name=='UnexpectedEndTag')].inner.kind" '"tuple"'
// @is "$.index[*][?(@.name=='UnexpectedEndTag')].inner.fields" []
// @is "$.index[*][?(@.name=='UnexpectedEndTag')].inner.fields_stripped" true

pub enum ParseError {
UnexpectedEndTag(#[doc(hidden)] u32),
Expand Down
34 changes: 34 additions & 0 deletions src/test/rustdoc-json/enums/kind.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#![feature(no_core)]
#![no_core]

pub enum Foo {
// @is "$.index[*][?(@.name=='Unit')].inner.kind" '"unit"'
// @set Unit = "$.index[*][?(@.name=='Unit')].id"
// @is "$.index[*][?(@.name=='Unit')].inner.fields" []
Unit,
// @is "$.index[*][?(@.name=='Named')].inner.kind" '"named_fields"'
// @set Named = "$.index[*][?(@.name=='Named')].id"
// @is "$.index[*][?(@.name=='Named')].inner.fields" []
Named {},
// @is "$.index[*][?(@.name=='Tuple')].inner.kind" '"tuple"'
// @set Tuple = "$.index[*][?(@.name=='Tuple')].id"
// @is "$.index[*][?(@.name=='Tuple')].inner.fields" []
Tuple(),
// @is "$.index[*][?(@.name=='NamedField')].inner.kind" '"named_fields"'
// @set NamedField = "$.index[*][?(@.name=='NamedField')].id"
// @set x = "$.index[*][?(@.name=='x' && @.kind=='field')].id"
// @is "$.index[*][?(@.name=='NamedField')].inner.fields[*]" $x
NamedField { x: i32 },
// @is "$.index[*][?(@.name=='TupleField')].inner.kind" '"tuple"'
// @set TupleField = "$.index[*][?(@.name=='TupleField')].id"
// @set tup_field = "$.index[*][?(@.name=='0' && @.kind=='field')].id"
// @is "$.index[*][?(@.name=='TupleField')].inner.fields[*]" $tup_field
TupleField(i32),
}

// @is "$.index[*][?(@.name=='Foo')].inner.variants[0]" $Unit
// @is "$.index[*][?(@.name=='Foo')].inner.variants[1]" $Named
// @is "$.index[*][?(@.name=='Foo')].inner.variants[2]" $Tuple
// @is "$.index[*][?(@.name=='Foo')].inner.variants[3]" $NamedField
// @is "$.index[*][?(@.name=='Foo')].inner.variants[4]" $TupleField
// @count "$.index[*][?(@.name=='Foo')].inner.variants[*]" 5
22 changes: 22 additions & 0 deletions src/test/rustdoc-json/enums/some_fields_hidden.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#![feature(no_core)]
#![no_core]

pub enum Foo {
// @set i8 = "$.index[*][?(@.docs=='i8' && @.kind=='field')].id"
// @is "$.index[*][?(@.docs=='i8' && @.kind=='field')].name" '"0"'
// @is "$.index[*][?(@.name=='V1')].inner.fields[*]" $i8
// @is "$.index[*][?(@.name=='V1')].inner.fields_stripped" false
V1(
/// i8
i8,
),
// @set u8 = "$.index[*][?(@.docs=='u8' && @.kind=='field')].id"
// @is "$.index[*][?(@.docs=='u8' && @.kind=='field')].name" '"1"'
// @is "$.index[*][?(@.name=='V2')].inner.fields[*]" $u8
// @is "$.index[*][?(@.name=='V2')].inner.fields_stripped" true
V2(
#[doc(hidden)] u8,
/// u8
u8,
),
}
15 changes: 6 additions & 9 deletions src/test/rustdoc-json/enums/variant_struct.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
// @has "$.index[*][?(@.name=='EnumStruct')].visibility" \"public\"
// @has "$.index[*][?(@.name=='EnumStruct')].kind" \"enum\"
// @is "$.index[*][?(@.name=='EnumStruct')].visibility" \"public\"
// @is "$.index[*][?(@.name=='EnumStruct')].kind" \"enum\"
pub enum EnumStruct {
// @has "$.index[*][?(@.name=='VariantS')].inner.variant_kind" \"struct\"
// @has "$.index[*][?(@.name=='x')].kind" \"struct_field\"
// @has "$.index[*][?(@.name=='y')].kind" \"struct_field\"
VariantS {
x: u32,
y: String,
},
// @is "$.index[*][?(@.name=='VariantS')].inner.kind" \"named_fields\"
// @is "$.index[*][?(@.name=='x')].kind" \"field\"
// @is "$.index[*][?(@.name=='y')].kind" \"field\"
VariantS { x: u32, y: String },
}
10 changes: 5 additions & 5 deletions src/test/rustdoc-json/enums/variant_tuple_struct.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// @has "$.index[*][?(@.name=='EnumTupleStruct')].visibility" \"public\"
// @has "$.index[*][?(@.name=='EnumTupleStruct')].kind" \"enum\"
// @is "$.index[*][?(@.name=='EnumTupleStruct')].visibility" \"public\"
// @is "$.index[*][?(@.name=='EnumTupleStruct')].kind" \"enum\"
pub enum EnumTupleStruct {
// @has "$.index[*][?(@.name=='VariantA')].inner.variant_kind" \"tuple\"
// @has "$.index[*][?(@.name=='0')].kind" \"struct_field\"
// @has "$.index[*][?(@.name=='1')].kind" \"struct_field\"
// @is "$.index[*][?(@.name=='VariantA')].inner.kind" \"tuple\"
// @is "$.index[*][?(@.name=='0')].kind" \"field\"
// @is "$.index[*][?(@.name=='1')].kind" \"field\"
VariantA(u32, String),
}
2 changes: 1 addition & 1 deletion src/test/rustdoc-json/nested.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub mod l1 {
pub mod l3 {

// @is "$.index[*][?(@.name=='L4')].kind" \"struct\"
// @is "$.index[*][?(@.name=='L4')].inner.struct_type" \"unit\"
// @is "$.index[*][?(@.name=='L4')].inner.kind" \"unit\"
// @set l4_id = "$.index[*][?(@.name=='L4')].id"
// @ismany "$.index[*][?(@.name=='l3')].inner.items[*]" $l4_id
pub struct L4;
Expand Down
Loading