Stream: rustdoc

Topic: JSON Format


view this post on Zulip CraftSpider (Jan 20 2021 at 18:30):

Attempting to collect thoughts about JSON format improvements. List below is a mix of things from the original review and my additions.

view this post on Zulip CraftSpider (Jan 20 2021 at 18:30):

view this post on Zulip CraftSpider (Jan 20 2021 at 18:30):

Several of these changes would be very easy to implement, but any thoughts on which would be useful, or any that opinions on have changed?

view this post on Zulip Joshua Nelson (Jan 20 2021 at 18:44):

include_privates may be unnecessary, whether private things are there will be obvious to the user as they passed the flag | Or see the 'private' items in the output, if passed

I think this should be fixed first

view this post on Zulip Joshua Nelson (Jan 20 2021 at 18:44):

other than that I don't remember exactly my comments, but this all looks vaguely in the right direction

view this post on Zulip Joshua Nelson (Jan 20 2021 at 18:44):

would be great to hear from @HeroicKatora when they get back from lunch :)

view this post on Zulip HeroicKatora (Jan 20 2021 at 19:20):

view this post on Zulip HeroicKatora (Jan 20 2021 at 19:24):

In particular, since index and paths are not meant to be mutable, I have a very slight preference for a sorted array of sorts. But it's not a critical requirement by any means and a map is cleaner in the serde interaction/type interface.

view this post on Zulip CraftSpider (Jan 20 2021 at 20:24):

So, given Heroic's comment, I'd leave include_privates alone for the moment, but doc nullability should be easy long-hanging fruit, as it's just threading Option and removing some unwraps mostly

view this post on Zulip CraftSpider (Jan 20 2021 at 21:09):

Interesting how clean::Union has a StructType, as both Unit and Tuple unions are rejected by the parser (expected `where` or `{` after union name, found `;` )

view this post on Zulip CraftSpider (Jan 20 2021 at 21:10):

clean::Union and clean::Struct are the same, aside from their name. It would almost make more sense to consider 'Union' a StructType

view this post on Zulip CraftSpider (Jan 20 2021 at 21:19):

On the other hand, it makes sense to keep unions as distinct items. I'm thinking about this because I really want to fix the whole 'structs and unions are indistinguishable in JSON' thing

view this post on Zulip Joshua Nelson (Jan 20 2021 at 21:20):

could you make them different but leave out StructType from union?

view this post on Zulip CraftSpider (Jan 20 2021 at 21:25):

Yeah, that works. Mind if I do that as part of a general move of StructType into clean, as that's now the only place with types that store it (rendering uses it)?

view this post on Zulip Joshua Nelson (Jan 20 2021 at 21:25):

sure, SGTM

view this post on Zulip CraftSpider (Jan 20 2021 at 21:52):

PR for that up. I hope you don't mind that I keep requesting your review, I can let high-five pick if you'd rather not get the pings

view this post on Zulip Joshua Nelson (Jan 20 2021 at 21:52):

haha, nah this is fine

view this post on Zulip Joshua Nelson (Jan 20 2021 at 21:52):

JSON PRs are easy to review because I don't have to worry about breaking things :laughing:

view this post on Zulip Joshua Nelson (Jan 20 2021 at 21:53):

if something goes wrong HeroicKatora can just ping me lol

view this post on Zulip Joshua Nelson (Jan 20 2021 at 21:58):

https://github.com/rust-lang/rust/pull/81227/files#r561334410

view this post on Zulip CraftSpider (Jan 22 2021 at 19:18):

I like the idea of the json types being published as their own crate. Personally, I don't even think the rest of it is necessary (Or as reasonable, EG the conversion needs rustc internals), but the types with their serde impls is great.

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:30):

@simulacrum who would I talk to about autopublishing src/librustdoc/json/types.rs as a crate? Is that something that makes sense to do? Happy to do the work for it.

view this post on Zulip simulacrum (Jan 22 2021 at 19:35):

I would prefer to not do that personally

view this post on Zulip simulacrum (Jan 22 2021 at 19:35):

what is your use case I guess?

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:35):

the use case is someone who wants to use the JSON API without rewriting the deserialization

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:36):

if we don't publish this someone will, and it won't be as accurate or up to date

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:36):

case in point: https://docs.rs/rustdoc-types/

view this post on Zulip simulacrum (Jan 22 2021 at 19:36):

then, autopublishing seems like the wrong approach to me

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:36):

what would you suggest instead?

view this post on Zulip simulacrum (Jan 22 2021 at 19:36):

well, I guess, what kind of stability guarantees are we offering here?

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:37):

not tied to releases, just a normal crate with semver

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:37):

and we can document which version of the crate correponds to a Rust release

view this post on Zulip simulacrum (Jan 22 2021 at 19:37):

but you want nice semver? i.e. you expect to bump major versions rarely?

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:38):

that's what I was imagining, yes

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:38):

there's more discussion in the RFC, one sec

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:38):

https://github.com/rust-lang/rfcs/pull/2963#issuecomment-667342922, https://github.com/rust-lang/rfcs/pull/2963#issuecomment-669466678

view this post on Zulip simulacrum (Jan 22 2021 at 19:40):

ok. so what I would do, I think, is move that file to a separate crate, and put it under, say, rust-lang/rustdoc-json github or something like that.

view this post on Zulip simulacrum (Jan 22 2021 at 19:41):

then we can publish that to crates.io and use it from rustdoc via normal cargo.toml dep.

view this post on Zulip simulacrum (Jan 22 2021 at 19:41):

that said, I would personally prefer that we hold off on doing so until JSON in rustdoc moves out of erfc phase

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:41):

hmm, I see - the idea is that the conversions might break based on changes to rustc, but changing the types should be an intentional semver break

view this post on Zulip CraftSpider (Jan 22 2021 at 19:41):

Just to double check, would that trigger foreign impl rules on impl From<clean::Module> for /* rustdoc_json:: */ Module?

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:42):

CraftSpider said:

Just to double check, would that trigger foreign impl rules on impl From<clean::Module> for /* rustdoc_json:: */ Module?

we can change it to free functions if need be, I'm not worried about that

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:42):

rustdoc_json shouldn't depend on librustdoc anyway

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:42):

simulacrum said:

that said, I would personally prefer that we hold off on doing so until JSON in rustdoc moves out of erfc phase

I think it would be nice to have it as a separate crate (but not published to crates.io) in-tree during the eRFC

view this post on Zulip CraftSpider (Jan 22 2021 at 19:43):

Makes sense, I was worried about conversion difficulty, but thinking about it more changing to free functions is basically a renaming thing

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:43):

and people can use a git dependency

view this post on Zulip simulacrum (Jan 22 2021 at 19:43):

ok, well, that's not hard and doesn't require any checkin with anyone :)

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:43):

haha, I'll start with that then

view this post on Zulip simulacrum (Jan 22 2021 at 19:43):

if we want a crate on crates.io that we expect sort of "end users" to use, then I would like to see that go through an RFC.

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:43):

hey @CraftSpider ;)

view this post on Zulip simulacrum (Jan 22 2021 at 19:44):

(likely with T-core approval as a public facing artifact, in some sense)

view this post on Zulip CraftSpider (Jan 22 2021 at 19:45):

Oh my. Well, I can take a look :P

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:46):

FWIW I would expect actually adding the new crate to be easy and the "hard" part to be rewriting things as free functions

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:46):

but it's hard as in a lot of time, not as in difficult

view this post on Zulip CraftSpider (Jan 22 2021 at 19:47):

Yeah. Where would the new crate go, just in src? As it's not really a 'tool'

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:48):

I would put it in src/librustdoc/json-types maybe

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:48):

definitely in librustdoc somewhere

view this post on Zulip CraftSpider (Jan 22 2021 at 19:49):

Yeah, that makes sense, assuming Cargo doesn't get mad because it's the same level as the lib.rs file? It wouldn't be a mod so I don't think so

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:49):

nah the module tree is independent of the file tree

view this post on Zulip CraftSpider (Jan 22 2021 at 19:51):

Cool. I have managed to hit that point where like, I know a lot about some weird things, yet fairly little about other things. Yay self-teaching and diving into things head-first :P

view this post on Zulip CraftSpider (Jan 22 2021 at 19:57):

Hm. If we want to keep this untied to rustc, should I remove the FxHashMap usage?

view this post on Zulip CraftSpider (Jan 22 2021 at 19:57):

Replace it with just the std::collections::HashMap

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:57):

yes, please do

view this post on Zulip Joshua Nelson (Jan 22 2021 at 19:58):

you're just splitting out types.rs, right? conversions.rs needs to stay in rustdoc

view this post on Zulip CraftSpider (Jan 22 2021 at 19:58):

Yeah

view this post on Zulip CraftSpider (Jan 22 2021 at 20:04):

The types use FxHashMap in... 4 places

view this post on Zulip CraftSpider (Jan 22 2021 at 20:04):

And at least 2 of those have been proposed to change to a Vec, so that number might go down in the future

view this post on Zulip HeroicKatora (Jan 23 2021 at 11:12):

Admittedly I had just copied the types over as well, changing FxHashMap to std:: in the process.

view this post on Zulip GuillaumeGomez (Jan 23 2021 at 11:18):

Generally, we tend to use FxHashMap in the compiler, so why making the switch?

view this post on Zulip HeroicKatora (Jan 23 2021 at 11:20):

When publishing as a crate, I also wouldn't have changed it. It was just slightly more consistent with the local code that I already had and didn't take an extra dependency. It wouldn't really matter for consumption.

view this post on Zulip HeroicKatora (Jan 23 2021 at 11:46):

One more thing to consider for a crate would be adding non_exhaustive attributes although that comes very close to sort-of stabilizing Rust syntax, which was the argument against including syn in std for proc-macors.

view this post on Zulip GuillaumeGomez (Jan 23 2021 at 20:21):

That would mean to convert between the two types... Instead of exposing FxHashMap, can't we expose a trait (or multiple ones) that both HashMap and FxHashMap have so we can manipulate them both without caring about the actual type?

view this post on Zulip Joshua Nelson (Jan 23 2021 at 20:22):

what would be the point of doing that?

view this post on Zulip Joshua Nelson (Jan 23 2021 at 20:22):

HashMap is not particularly complicated

view this post on Zulip Léo Lanteri Thauvin (Jan 23 2021 at 20:23):

FxHashMap is a HashMap IIRC

view this post on Zulip Léo Lanteri Thauvin (Jan 23 2021 at 20:23):

It's just a type alias with a different hasher than the default

view this post on Zulip Léo Lanteri Thauvin (Jan 23 2021 at 20:24):

See the docs

view this post on Zulip Léo Lanteri Thauvin (Jan 23 2021 at 20:29):

Wait how does that work?

view this post on Zulip Léo Lanteri Thauvin (Jan 23 2021 at 20:29):

Oh the usual HashMap methods are defined only for S = RandomState

view this post on Zulip Léo Lanteri Thauvin (Jan 23 2021 at 20:30):

Nevermind me then :p

view this post on Zulip CraftSpider (Jan 24 2021 at 00:24):

There was a concern marked on the PR about the new crate going in src/etc. So, should it be moved to src/rustdoc-json-types (Mark Simulacrum's suggestion), or is there some other place to put it?

view this post on Zulip CraftSpider (Jan 24 2021 at 00:38):

New issue, raised by HeroicKatora: the Method kind does not possess an ABI field, but (trait) methods may have an ABI

view this post on Zulip HeroicKatora (Jan 24 2021 at 00:41):

Add is_unsafe to the list missing from Function and Method as well. It's present on FunctionPointer.

view this post on Zulip CraftSpider (Jan 24 2021 at 00:41):

Currently, unsafe should appear as part of the header field. If it doesn't, that's a bug

view this post on Zulip CraftSpider (Jan 24 2021 at 00:42):

fn pointers are handled differently, I suspect because they currently cannot be async or const, the other things that appear in header

view this post on Zulip HeroicKatora (Jan 24 2021 at 00:43):

I see! That's fine then, unless it makes recombining the declaration harder.

view this post on Zulip CraftSpider (Jan 24 2021 at 00:47):

I'd think it would be fine. Max forward-compat would actually make function pointers drop is_unsafe and use header, so that if say const fn pointers ever come to be then it can be non-breaking to add them (assuming we mark that adding new values to header might happen at will)

view this post on Zulip Joshua Nelson (Jan 24 2021 at 13:26):

CraftSpider said:

There was a concern marked on the PR about the new crate going in src/etc. So, should it be moved to src/rustdoc-json-types (Mark Simulacrum's suggestion), or is there some other place to put it?

@CraftSpider yes, rustdoc-json-types is fine. Next time can you please ask this on the PR? I had trouble finding your comment and I wasn't sure why you didn't make the change in the PR, I almost marked it waiting-on-author.

view this post on Zulip CraftSpider (Jan 24 2021 at 23:44):

Current format changes I have in mind:

view this post on Zulip CraftSpider (Jan 30 2021 at 01:31):

Another thing I just noticed, Import calls the full path 'span', I feel there could be a better name?

view this post on Zulip Noah Lev (Jan 30 2021 at 04:01):

What about path :upside_down:


Last updated: Oct 21 2021 at 21:46 UTC