webidl: Keep track of data discarded in type conversion #2387

chrysn · 2020-12-03T14:22:48Z

... and report it during documentation

Closes: #2381

This adds a concept of "confessions" -- statements the to_syn_type can emit when it knows it does a suboptimal conversion (eg. because typed promises would need GATs, or for compatibility, or any other reasons).

That data is passed along with the converted argument types, and during documentation converted into text that helps the user find what to do with that type. #2381 provides an example case where I was left to reimplement something there was already a helper for, simply because that helper was not discoverable. The reader will still need to use the search function until I can get markupped (possibly intra-doc) links from syn::Type (see dtolnay/syn#937).

It's still a bit rough around the edges, so please consider this as WIP; concretely, I'd like to overhaul the Confession structure to reduce redundancies between its variants. A high-level review at this stage would make sense already IMO, just don't get hung up on details like "why are there parallel vecs for the args and the confessions" because they're the type of thing still to enhance.

(On the "confessions" name: I'm open for any color of the bikeshed, but I prefer names that may sound a bit dramatic and maybe leave the reader thinking but give the gist over bland names like "additional type data" that leave the reader equally puzzled and could really mean anything else as well).

... and report it during documentation Closes: rustwasm#2381

Refactor Confession

chrysn · 2020-12-03T15:15:00Z

(If it's OK with you I'll run cargo fmt only once I've squashed the commits that build up; that should interfere less with the review process.)

alexcrichton

Awesome, thanks for this! I like the name "confession" as well :)

This is looking great so far, I'm eager to see this land!

crates/web-sys/src/features/gen_AnimationPropertyDetails.rs

alexcrichton · 2020-12-03T15:51:27Z

crates/webidl/src/generator.rs

@@ -38,6 +38,22 @@ fn comment(mut comment: String, features: &Option<String>) -> TokenStream {
    }
 }

+fn function_confession_docs(arguments_confessions: &Vec<(Ident, Option<crate::idl_type::Confession>)>, ret_confession: &Option<crate::idl_type::Confession>) -> proc_macro2::TokenStream {


FWIW &[T] is typically more idiomatic than &Vec<T>.

Also could Confession be imported into this module to avoid the long crate::idle_type::Confession type name?

And finally, like above Option<&T> is typically more idiomatic than &Option<T> where possible.

alexcrichton · 2020-12-03T15:53:50Z

crates/webidl/src/idl_type.rs

+            match (&self.kind, &self.actual_type) {
+                (ConfessionKind::Promise, Some(s)) => {
+                    write!(text, "While the Promise can produce any JsValue as far as the type system is \
+                            concerned, practically it is expected to contain a `{}`.", s)?;


I think it would be pretty cool actually if we could get the links to work here inside the {} bit. I think it should be pretty easy for this crate too? Everything lives in the root namespace so this may be able to just be:

[`{0}`](crate::{0})

perhaps?

Thing is, these can be more complex types as well (), and then this stops working; dtolnay/syn#937 is trying to get the full solution. An easy 80% solution for 5% of the effort would be to make them links if they're alphanumeric only and not attempt anything fancy otherwise. 15% more for 5% more by stripping any whitespace for those composed of alphanumerics or :: and making them links too. Unless dtolnay/syn#937 comes around quickly, I'll pick one of those.

Hm for the general case I could imagine that but for web-sys's use case we don't really have to deal with generics/lifetimes/etc, so I think the implementation should be pretty simple?

The three simple cases are covered now: plain identifiers, Option and &identifier. My go-to pages to look at the visualization are Directory::get_files, CredentialsContainer::create and U2f::register to check them.

dtolnay/syn#937 was closed as out-of-scope (which is OK, you might call the item a Zuständigkeitsfeststellungsfehlerbericht if you're into German composita); doc_prettyprint now goes as far as I consider it usable while documenting a bit of the general case this could be extended to if someone feels overly motivated.

alexcrichton · 2020-12-03T15:55:37Z

crates/webidl/src/idl_type.rs

+                (ConfessionKind::Promise, None) => {
+                    show_more = false;
+                    write!(text, "There is additional information in the IDL file about the content of \
+                           the promise, but it can not yet be explained any better.")?;


In a case like this I might insteada say that we should just sckip the additional docs. WebIDL is just an implementation detail and it's not exactly easy to look up the WebIDL source unfortunately. In that sense this may not be adding much value.

Can you list out the cases though where we don't know the type name?

Currently there's cases like AudioContext::close and suspend (where the promise is a void). I'll try to make better statements for the void case and see what's left there. (If nothing else, I'd prefer these statements to be left in to be searchable for whoever next wants to improve confessions).

Indeed I lacked the understanding that None in the IDL area just means void and indicates a unit type (changes in 4545b1f); this removes a whole class of "please look into the IDL" with "it just indicates completion" (for iterables, that doesn't happen in practice; in fd438fd).

This leaves "look into IDL" to union cases, the items in iterables inside promises, and iterables whose items have additional confessions.

alexcrichton · 2020-12-03T15:56:05Z

crates/webidl/src/idl_type.rs

+            }
+
+            if show_more {
+                write!(text, " More information is available in the source IDL file.")?;


Sort of like my comment above I wouldn't reference the IDL here since most users aren't aware of it or wouldn't know how to find it anyway.

Why not change that? I'd think the root of any generated crate is a good point to state what all these items were generated from; eg. like in 3952561. (If that stays, maybe I can make that statement linkable from the confessions, but not sure how well that works given it can be used with different -sys crates potentially).

I think unless this links to the WebIDL file in question it won't be too useful. Otherwise it's just saying "more info is probably somewhere" but it's not clear where to go to get more info.

crates/webidl/src/idl_type.rs

alexcrichton · 2020-12-03T15:59:45Z

crates/webidl/src/idl_type.rs

-            IdlType::Callback => Ok(js_sys("Function")),
+            IdlType::Any => Ok(js_value.honestly()),
+            IdlType::Void => Ok(None.honestly()), // FIXME what does it mean?
+            IdlType::Callback => Ok(js_sys("Function").honestly()), // FIXME can we give type?


This might be an issue with our parsing of WebIDl, there's probably data about the callback we're discarding somewhere along the way.

I checked around a bit but indeed, the information is long gone by this time. Added a generic confession anyway for consistency (as it's another case of "just because the signature says Function, there's actually more data but we don't express it") -- sure it'd be nice to have a better one here, but that's about as far as I can take it now.

(If you consider the added texts extraneous, I can make the confession not emit any text -- but for future expansion it's probably good to have it visible in the code).

See-Also: rustwasm#2387

Simplify ty_syn_type See-Also: rustwasm#2387 (comment)

Add indication for union (but no actual handling yet)

because generated docs will occasionally ask the user to look things up.

chrysn · 2020-12-04T11:25:56Z

I'm going through the remaining FIXMEs and refactoring artifacts (likely just changing the comment), but from my PoV this is getting ready; please let me know when you'd want it squashed.

See-Also: rustwasm#2387 (comment)

... by adding them to getters/setters, and complaining loudly where they are currently not used.

chrysn · 2020-12-04T13:57:09Z

All remaining FIXMEs resolved (fixing the treatment of those behind an indexed getter, and adding output to getters/setters).

If you're happy with this (or just prefer to review it as the 3-4 commits it'll eventually be), I'll squash it and run rustfmt over the commit tree.

crates/web-sys/src/features/gen_AudioContext.rs

crates/web-sys/src/features/gen_AnimationPropertyDetails.rs

alexcrichton · 2020-12-04T18:33:09Z

crates/web-sys/src/features/gen_AllowedBluetoothDevice.rs

@@ -39,6 +39,8 @@ impl AllowedBluetoothDevice {
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `AllowedBluetoothDevice`*"]
    #[doc = ""]
+    #[doc = "The type is actually a union over some types and  can not yet be explained any better."]


FWIW this doesn't seem too helpful IMO, it's not really adding much more than the documentation could already mention.

crates/web-sys/src/features/gen_AudioWorkletNodeOptions.rs

crates/web-sys/src/features/gen_AnimationPropertyDetails.rs

alexcrichton

I think there's still some FIXME annotations there about dropped confessions?

I think that the docs may still need some massaging to produce links. I can double check the rustdoc output locally before merging too.

alexcrichton · 2020-12-04T18:37:15Z

crates/webidl/src/idl_type.rs

+            .chars().all(|c| c.is_alphanumeric() || c == '_')
+    }
+
+    let quoted = format!("{}", quote! { #t });


I think it would be better to work over the structure of the syn::Type here rather than looking at the stringification of it. That feels sort of brittle and error-prone where we shouldn't really be dealing with too too fancy types here?

I've tried going that path -- but it isn't sufficient to superficially look at them, and in the end would require reimplementing most of quote (even to get to the "simple" cases). I had hoped to match on a few versions, but even just looking at Path means that for each path entry it needs to be understood whether that is just a literal or has some arguments that'd need special processing.

Hm I'd be surprised if we're getting all sorts of flavorful types here. What sort of syn::Type shapes are getting processed here?

alexcrichton · 2020-12-04T18:38:02Z

crates/webidl/src/idl_type.rs

+            }
+
+            if show_more {
+                write!(text, " More information is available in the source IDL file.")?;


I think unless this links to the WebIDL file in question it won't be too useful. Otherwise it's just saying "more info is probably somewhere" but it's not clear where to go to get more info.

chrysn · 2020-12-04T20:16:35Z

The FIXMEs are all gone in the pushed diff from master.

On the usefulness (of the "more is there" and "union" you mentioned, as well as probably the function signatures), I'll take one more dab at whether I can get by the WebIDL file name (to eventually construct a precise link, even if it's just into the GitHub tree) -- otherwise I'd silence them.

alexcrichton · 2020-12-09T22:42:40Z

Ok yeah seems reasonable to me, if we can't offer much more than "go look elsewhere" I think it's fine to omit the info for now.

chrysn added 3 commits December 3, 2020 15:15

webidl: Keep track of data discarded in type conversion

61f885b

... and report it during documentation Closes: rustwasm#2381

webidls rebuild of web-sys

14c5731

squash! webidl: Keep track of data discarded in type conversion

9e34455

Refactor Confession

alexcrichton reviewed Dec 3, 2020

View reviewed changes

chrysn added 11 commits December 3, 2020 18:21

Hide web-sys's generated file diffs by default

1331186

See-Also: rustwasm#2387

squash! webidl: Keep track of data discarded in type conversion

8a0e510

Simplify ty_syn_type See-Also: rustwasm#2387 (comment)

fixup! squash! webidl: Keep track of data discarded in type conversion

c8fe7ac

fixup! webidls rebuild of web-sys

f1085bd

squash! webidl: Keep track of data discarded in type conversion

ec36d84

Add indication for union (but no actual handling yet)

confession texts: Create better links

6bd733f

webidls rebuild of web-sys

ff47a27

web-sys: Point to WebIDLs

3952561

because generated docs will occasionally ask the user to look things up.

confessions: Add understanding that None IDL means unit type

4545b1f

webidls rebuild of web-sys

fd438fd

confessions: Minimally cover callbacks

c60ea63

chrysn added 6 commits December 4, 2020 12:26

webidls rebuild of web-sys

1bc0602

confessions: clean-up

6ed7020

See-Also: rustwasm#2387 (comment)

confessions: Clean-up, handling options like results earlier

0c1d469

confessions: Resolve remaining "FIXME dismissing confession"

98ff925

... by adding them to getters/setters, and complaining loudly where they are currently not used.

confessions: Handle indexed getters being put behind an Option

3e8171f

webidls rebuild of web-sys

8021960

chrysn mentioned this pull request Dec 4, 2020

wishlist: Formatting a syn::Type for rustdoc dtolnay/syn#937

Closed

alexcrichton reviewed Dec 4, 2020

View reviewed changes

fixup! some of the previous commits

d350c54

Uh oh!

webidl: Keep track of data discarded in type conversion #2387

Are you sure you want to change the base?

webidl: Keep track of data discarded in type conversion #2387

Uh oh!

Conversation

chrysn commented Dec 3, 2020

Uh oh!

chrysn commented Dec 3, 2020

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrysn commented Dec 4, 2020

Uh oh!

chrysn commented Dec 4, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrysn commented Dec 4, 2020

Uh oh!

alexcrichton commented Dec 9, 2020

Uh oh!

Uh oh!