Skip to content
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

[rustdoc-json] Make header a vec of modifiers, and FunctionPointer consistent #81891

Merged
merged 5 commits into from
Feb 15, 2021
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 1 addition & 1 deletion src/etc/check_missing_items.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def check_type(ty):
elif ty["kind"] == "function_pointer":
for param in ty["inner"]["generic_params"]:
check_generic_param(param)
check_decl(ty["inner"]["inner"])
check_decl(ty["inner"]["decl"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous implementation looks wrong, other call(s) of check_decl are ["inner"]["decl"], and it doesn't look like there should ever be an ["inner"]["inner"] in the output.

elif ty["kind"] == "qualified_path":
check_type(ty["inner"]["self_type"])
check_type(ty["inner"]["trait"])
Expand Down
36 changes: 26 additions & 10 deletions src/librustdoc/json/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
//! the `clean` types but with some fields removed or stringified to simplify the output and not
//! expose unstable compiler internals.

#![allow(rustc::default_hash_types)]

use std::convert::From;

use rustc_ast::ast;
Expand All @@ -14,6 +16,7 @@ use rustdoc_json_types::*;
use crate::clean;
use crate::formats::item_type::ItemType;
use crate::json::JsonRenderer;
use std::collections::HashSet;

impl JsonRenderer<'_> {
pub(super) fn convert_item(&self, item: clean::Item) -> Option<Item> {
Expand Down Expand Up @@ -225,15 +228,22 @@ crate fn from_ctor_kind(struct_type: CtorKind) -> StructType {
}
}

fn stringify_header(header: &rustc_hir::FnHeader) -> String {
let mut s = String::from(header.unsafety.prefix_str());
if header.asyncness == rustc_hir::IsAsync::Async {
s.push_str("async ")
crate fn from_fn_header(header: &rustc_hir::FnHeader) -> HashSet<Modifiers> {
let mut v = HashSet::new();

if let rustc_hir::Unsafety::Unsafe = header.unsafety {
v.insert(Modifiers::Unsafe);
}

if let rustc_hir::IsAsync::Async = header.asyncness {
v.insert(Modifiers::Async);
}
if header.constness == rustc_hir::Constness::Const {
s.push_str("const ")

if let rustc_hir::Constness::Const = header.constness {
v.insert(Modifiers::Const);
}
s

v
}

impl From<clean::Function> for Function {
Expand All @@ -242,7 +252,7 @@ impl From<clean::Function> for Function {
Function {
decl: decl.into(),
generics: generics.into(),
header: stringify_header(&header),
header: from_fn_header(&header),
abi: header.abi.to_string(),
}
}
Expand Down Expand Up @@ -364,7 +374,13 @@ impl From<clean::BareFunctionDecl> for FunctionPointer {
fn from(bare_decl: clean::BareFunctionDecl) -> Self {
let clean::BareFunctionDecl { unsafety, generic_params, decl, abi } = bare_decl;
FunctionPointer {
is_unsafe: unsafety == rustc_hir::Unsafety::Unsafe,
header: if let rustc_hir::Unsafety::Unsafe = unsafety {
let mut hs = HashSet::new();
hs.insert(Modifiers::Unsafe);
hs
} else {
HashSet::new()
},
generic_params: generic_params.into_iter().map(Into::into).collect(),
decl: decl.into(),
abi: abi.to_string(),
Expand Down Expand Up @@ -439,7 +455,7 @@ crate fn from_function_method(function: clean::Function, has_body: bool) -> Meth
Method {
decl: decl.into(),
generics: generics.into(),
header: stringify_header(&header),
header: from_fn_header(&header),
abi: header.abi.to_string(),
has_body,
}
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 @@ -243,7 +243,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
)
})
.collect(),
format_version: 3,
format_version: 4,
};
let mut p = self.out_path.clone();
p.push(output.index.get(&output.root).unwrap().name.clone().unwrap());
Expand Down
19 changes: 14 additions & 5 deletions src/rustdoc-json-types/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! These types are the public API exposed through the `--output-format json` flag. The [`Crate`]
//! struct is the root of the JSON blob and all other items are contained within.

use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::path::PathBuf;

use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -281,19 +281,28 @@ pub enum StructType {
Unit,
}

#[non_exhaustive]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other things may be added as modifiers in the language, or default on methods could be interpreted as a modifier. This way things can be added without it being breaking

Copy link
Contributor

@HeroicKatora HeroicKatora Feb 9, 2021

Choose a reason for hiding this comment

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

One detail with the addition of new variants would be that the order is not quite arbitrary. The grammar apppears to be currently:

AsyncConstQualifiers? unsafe? (extern Abi?)?

So while technically permitting the forward compatible addition of new qualifiers, a pretty-printer might still be confused by their order and not produce syntactically valid method/type declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Joshua said that we should use sets over vecs specifically so order is not guaranteed as part of the contract. I think that plus non_exhaustive means it's at least fairly obvious that we're not guaranteeing anything (all matches will require a _ part), the only issue is if people think we're implying something about the syntax.

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
#[serde(rename_all = "snake_case")]
pub enum Modifiers {
CraftSpider marked this conversation as resolved.
Show resolved Hide resolved
Const,
Unsafe,
Async,
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
pub struct Function {
pub decl: FnDecl,
pub generics: Generics,
pub header: String,
pub header: HashSet<Modifiers>,
pub abi: String,
}

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
pub struct Method {
pub decl: FnDecl,
pub generics: Generics,
pub header: String,
pub header: HashSet<Modifiers>,
pub abi: String,
pub has_body: bool,
}
Expand Down Expand Up @@ -404,9 +413,9 @@ pub enum Type {

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
pub struct FunctionPointer {
pub is_unsafe: bool,
pub generic_params: Vec<GenericParamDef>,
pub decl: FnDecl,
pub generic_params: Vec<GenericParamDef>,
pub header: HashSet<Modifiers>,
pub abi: String,
}

Expand Down
5 changes: 5 additions & 0 deletions src/test/rustdoc-json/fn_pointer/header.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// @has header.json "$.index[*][?(@.name=='FnPointer')].inner.type.inner.header" "[]"
pub type FnPointer = fn();

// @has - "$.index[*][?(@.name=='UnsafePointer')].inner.type.inner.header" '["unsafe"]'
pub type UnsafePointer = unsafe fn();
22 changes: 22 additions & 0 deletions src/test/rustdoc-json/fns/header.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// edition:2018

// @has header.json "$.index[*][?(@.name=='nothing_fn')].inner.header" "[]"
pub fn nothing_fn() {}

// @has - "$.index[*][?(@.name=='const_fn')].inner.header" '["const"]'
pub const fn const_fn() {}

// @has - "$.index[*][?(@.name=='async_fn')].inner.header" '["async"]'
pub async fn async_fn() {}

// @count - "$.index[*][?(@.name=='async_unsafe_fn')].inner.header[*]" 2
// @has - "$.index[*][?(@.name=='async_unsafe_fn')].inner.header[*]" '"async"'
// @has - "$.index[*][?(@.name=='async_unsafe_fn')].inner.header[*]" '"unsafe"'
pub async unsafe fn async_unsafe_fn() {}

// @count - "$.index[*][?(@.name=='const_unsafe_fn')].inner.header[*]" 2
// @has - "$.index[*][?(@.name=='const_unsafe_fn')].inner.header[*]" '"const"'
// @has - "$.index[*][?(@.name=='const_unsafe_fn')].inner.header[*]" '"unsafe"'
pub const unsafe fn const_unsafe_fn() {}
CraftSpider marked this conversation as resolved.
Show resolved Hide resolved

// It's impossible for a function to be both const and async, so no test for that
26 changes: 26 additions & 0 deletions src/test/rustdoc-json/methods/header.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// edition:2018

pub struct Foo;

impl Foo {
// @has header.json "$.index[*][?(@.name=='nothing_meth')].inner.header" "[]"
pub fn nothing_meth() {}

// @has - "$.index[*][?(@.name=='const_meth')].inner.header" '["const"]'
pub const fn const_meth() {}

// @has - "$.index[*][?(@.name=='async_meth')].inner.header" '["async"]'
pub async fn async_meth() {}

// @count - "$.index[*][?(@.name=='async_unsafe_meth')].inner.header[*]" 2
// @has - "$.index[*][?(@.name=='async_unsafe_meth')].inner.header[*]" '"async"'
// @has - "$.index[*][?(@.name=='async_unsafe_meth')].inner.header[*]" '"unsafe"'
pub async unsafe fn async_unsafe_meth() {}

// @count - "$.index[*][?(@.name=='const_unsafe_meth')].inner.header[*]" 2
// @has - "$.index[*][?(@.name=='const_unsafe_meth')].inner.header[*]" '"const"'
// @has - "$.index[*][?(@.name=='const_unsafe_meth')].inner.header[*]" '"unsafe"'
pub const unsafe fn const_unsafe_meth() {}

// It's impossible for a method to be both const and async, so no test for that
}