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

Copy more doc comments to JS/TS files, unescape comments #2070

Merged
merged 2 commits into from
Apr 13, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions crates/backend/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ pub struct Enum {
pub struct Variant {
pub name: Ident,
pub value: u32,
pub comments: Vec<String>,
}

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
Expand Down
1 change: 1 addition & 0 deletions crates/backend/src/encode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ fn shared_variant<'a>(v: &'a ast::Variant, intern: &'a Interner) -> EnumVariant<
EnumVariant {
name: intern.intern(&v.name),
value: v.value,
comments: v.comments.iter().map(|s| &**s).collect(),
}
}

Expand Down
47 changes: 36 additions & 11 deletions crates/cli-support/src/js/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ pub struct ExportedClass {
is_inspectable: bool,
/// All readable properties of the class
readable_properties: Vec<String>,
/// Map from field name to type as a string plus whether it has a setter
/// Map from field name to type as a string, docs plus whether it has a setter
/// and it is optional
typescript_fields: HashMap<String, (String, bool, bool)>,
typescript_fields: HashMap<String, (String, String, bool, bool)>,
}

const INITIAL_HEAP_VALUES: &[&str] = &["undefined", "null", "true", "false"];
Expand Down Expand Up @@ -798,7 +798,8 @@ impl<'a> Context<'a> {
let mut fields = class.typescript_fields.keys().collect::<Vec<_>>();
fields.sort(); // make sure we have deterministic output
for name in fields {
let (ty, has_setter, is_optional) = &class.typescript_fields[name];
let (ty, docs, has_setter, is_optional) = &class.typescript_fields[name];
ts_dst.push_str(docs);
ts_dst.push_str(" ");
if !has_setter {
ts_dst.push_str("readonly ");
Expand All @@ -816,6 +817,7 @@ impl<'a> Context<'a> {
ts_dst.push_str("}\n");

self.export(&name, &dst, Some(&class.comments))?;
self.typescript.push_str(&class.comments);
self.typescript.push_str(&ts_dst);

Ok(())
Expand Down Expand Up @@ -2901,10 +2903,23 @@ impl<'a> Context<'a> {
self.typescript
.push_str(&format!("export enum {} {{", enum_.name));
}
for (name, value) in enum_.variants.iter() {
for (name, value, comments) in enum_.variants.iter() {
let variant_docs = if comments.is_empty() {
String::new()
} else {
format_doc_comments(&comments, None)
};
if !variant_docs.is_empty() {
variants.push_str("\n");
variants.push_str(&variant_docs);
}
variants.push_str(&format!("{}:{},", name, value));
if enum_.generate_typescript {
self.typescript.push_str(&format!("\n {},", name));
self.typescript.push_str("\n");
if !variant_docs.is_empty() {
self.typescript.push_str(&variant_docs);
}
self.typescript.push_str(&format!(" {},", name));
}
}
if enum_.generate_typescript {
Expand Down Expand Up @@ -3227,7 +3242,7 @@ impl ExportedClass {
fn push_getter(&mut self, docs: &str, field: &str, js: &str, ret_ty: Option<&str>) {
self.push_accessor(docs, field, js, "get ");
if let Some(ret_ty) = ret_ty {
self.push_accessor_ts(field, ret_ty);
self.push_accessor_ts(docs, field, ret_ty, false);
}
self.readable_properties.push(field.to_string());
}
Expand All @@ -3244,20 +3259,30 @@ impl ExportedClass {
) {
self.push_accessor(docs, field, js, "set ");
if let Some(ret_ty) = ret_ty {
let (has_setter, is_optional) = self.push_accessor_ts(field, ret_ty);
*has_setter = true;
let is_optional = self.push_accessor_ts(docs, field, ret_ty, true);
*is_optional = might_be_optional_field;
}
}

fn push_accessor_ts(&mut self, field: &str, ret_ty: &str) -> (&mut bool, &mut bool) {
let (ty, has_setter, is_optional) = self
fn push_accessor_ts(
&mut self,
docs: &str,
field: &str,
ret_ty: &str,
is_setter: bool,
) -> &mut bool {
let (ty, accessor_docs, has_setter, is_optional) = self
.typescript_fields
.entry(field.to_string())
.or_insert_with(Default::default);

*ty = ret_ty.to_string();
(has_setter, is_optional)
// Deterministic output: always use the getter's docs if available
if !docs.is_empty() && (accessor_docs.is_empty() || !is_setter) {
*accessor_docs = docs.to_owned();
}
*has_setter |= is_setter;
is_optional
}

fn push_accessor(&mut self, docs: &str, field: &str, js: &str, prefix: &str) {
Expand Down
3 changes: 2 additions & 1 deletion crates/cli-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,8 @@ fn reset_indentation(s: &str) -> String {
dst.push_str(line);
}
dst.push_str("\n");
if line.ends_with('{') {
// Ignore { inside of comments and if it's an exported enum
if line.ends_with('{') && !line.starts_with('*') && !line.ends_with("Object.freeze({") {
indent += 1;
}
}
Expand Down
83 changes: 80 additions & 3 deletions crates/cli-support/src/wit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ use crate::descriptors::WasmBindgenDescriptorsSection;
use crate::intrinsic::Intrinsic;
use crate::{decode, PLACEHOLDER_MODULE};
use anyhow::{anyhow, bail, Error};
use std::char;
use std::collections::{HashMap, HashSet};
use std::str;
use std::str::{self, Chars};
use walrus::MemoryId;
use walrus::{ExportId, FunctionId, ImportId, Module};
use wasm_bindgen_shared::struct_function_export_name;
Expand Down Expand Up @@ -766,7 +767,13 @@ impl<'a> Context<'a> {
variants: enum_
.variants
.iter()
.map(|v| (v.name.to_string(), v.value))
.map(|v| {
(
v.name.to_string(),
v.value,
concatenate_comments(&v.comments),
)
})
.collect(),
generate_typescript: enum_.generate_typescript,
};
Expand Down Expand Up @@ -1513,7 +1520,77 @@ fn verify_schema_matches<'a>(data: &'a [u8]) -> Result<Option<&'a str>, Error> {
fn concatenate_comments(comments: &[&str]) -> String {
comments
.iter()
.map(|s| s.trim_matches('"'))
.map(|&s| try_unescape(s).unwrap_or_else(|| s.to_owned()))
.collect::<Vec<_>>()
.join("\n")
}

// Unescapes a quoted string. char::escape_debug() was used to escape the text.
fn try_unescape(s: &str) -> Option<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally a bit confused on why we need a method like this, can you help me understand where this is coming from? Where do we acquire an escaped string from, and would it be possible to acquire an unescaped string instead?

Copy link
Author

Choose a reason for hiding this comment

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

TokenTree::Literal(lit) => {
// this will always return the quoted string, we deal with
// that in the cli when we read in the comments
Some(lit.to_string())
}

That's the code that gets a quoted + escaped string from proc_macro2. I couldn't find an unescape function anywhere. If you know of one, I'll replace this func with that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the un-quoting/etc happen around there? I don't think it should be happening in the CLI but rather at the source here if possible.

Copy link
Author

Choose a reason for hiding this comment

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

It's been moved now. The old code used to unquote the string, but it's also been moved. Let me know if that's considered a breaking change and I'll restore it.

if s.is_empty() {
return Some(String::new());
}
let mut result = String::with_capacity(s.len());
let mut chars = s.chars();
for i in 0.. {
let c = match chars.next() {
Some(c) => c,
None => {
if result.ends_with('"') {
result.pop();
}
return Some(result);
}
};
if i == 0 && c == '"' {
// ignore it
} else if c == '\\' {
let c = chars.next()?;
match c {
't' => result.push('\t'),
'r' => result.push('\r'),
'n' => result.push('\n'),
'\\' | '\'' | '"' => result.push(c),
'u' => {
if chars.next() != Some('{') {
return None;
}
let (c, next) = unescape_unicode(&mut chars)?;
result.push(c);
if next != '}' {
return None;
}
}
_ => return None,
}
} else {
result.push(c);
}
}
None
}

fn unescape_unicode(chars: &mut Chars) -> Option<(char, char)> {
let mut value = 0;
for i in 0..7 {
let c = chars.next()?;
let num = if c >= '0' && c <= '9' {
c as u32 - '0' as u32
} else if c >= 'a' && c <= 'f' {
c as u32 - 'a' as u32 + 10
} else if c >= 'A' && c <= 'F' {
c as u32 - 'A' as u32 + 10
} else {
if i == 0 {
return None;
}
let decoded = char::from_u32(value)?;
return Some((decoded, c));
};
if i >= 6 {
return None;
}
value = (value << 4) | num;
}
None
}
4 changes: 2 additions & 2 deletions crates/cli-support/src/wit/nonstandard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@ pub struct AuxEnum {
pub name: String,
/// The copied Rust comments to forward to JS
pub comments: String,
/// A list of variants with their name and value
/// A list of variants with their name, value and comments
/// and whether typescript bindings should be generated for each variant
pub variants: Vec<(String, u32)>,
pub variants: Vec<(String, u32, String)>,
/// Whether typescript bindings should be generated for this enum.
pub generate_typescript: bool,
}
Expand Down
2 changes: 2 additions & 0 deletions crates/macro-support/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1116,9 +1116,11 @@ impl<'a> MacroParse<(&'a mut TokenStream, BindgenAttrs)> for syn::ItemEnum {
),
};

let comments = extract_doc_comments(&v.attrs);
Ok(ast::Variant {
name: v.ident.clone(),
value,
comments,
})
})
.collect::<Result<Vec<_>, Diagnostic>>()?;
Expand Down
1 change: 1 addition & 0 deletions crates/shared/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ macro_rules! shared_api {
struct EnumVariant<'a> {
name: &'a str,
value: u32,
comments: Vec<&'a str>,
}

struct Function<'a> {
Expand Down
12 changes: 10 additions & 2 deletions tests/wasm/comments.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@ const assert = require('assert');
exports.assert_comments_exist = function() {
const bindings_file = require.resolve('wasm-bindgen-test');
const contents = fs.readFileSync(bindings_file);
assert.ok(contents.includes("* annotated function"));
assert.ok(contents.includes("* annotated function ✔️ \" \\ ' {"));
assert.ok(contents.includes("* annotated struct type"));
assert.ok(contents.includes("* annotated struct field"));
assert.ok(contents.includes("* annotated struct field b"));
assert.ok(contents.includes("* annotated struct field c"));
assert.ok(contents.includes("* annotated struct constructor"));
assert.ok(contents.includes("* annotated struct method"));
assert.ok(contents.includes("* annotated struct getter"));
assert.ok(contents.includes("* annotated struct setter"));
assert.ok(contents.includes("* annotated struct static method"));
assert.ok(contents.includes("* annotated enum type"));
assert.ok(contents.includes("* annotated enum variant 1"));
assert.ok(contents.includes("* annotated enum variant 2"));
};
45 changes: 42 additions & 3 deletions tests/wasm/comments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,65 @@ extern "C" {
fn assert_comments_exist();
}

/// annotated function ✔️ " \ ' {
#[wasm_bindgen]
/// annotated function
pub fn annotated() -> String {
String::new()
}

#[wasm_bindgen]
/// annotated struct type
#[wasm_bindgen]
pub struct Annotated {
a: String,
/// annotated struct field
/// annotated struct field b
pub b: u32,
/// annotated struct field c
#[wasm_bindgen(readonly)]
pub c: u32,
d: u32,
}

#[wasm_bindgen]
impl Annotated {
/// annotated struct constructor
#[wasm_bindgen(constructor)]
pub fn new() -> Self {
Self {
a: String::new(),
b: 0,
c: 0,
d: 0,
}
}

/// annotated struct method
pub fn get_a(&self) -> String {
self.a.clone()
}

/// annotated struct getter
#[wasm_bindgen(getter)]
pub fn d(&self) -> u32 {
self.d
}

/// annotated struct setter
#[wasm_bindgen(setter)]
pub fn set_d(&mut self, value: u32) {
self.d = value
}

/// annotated struct static method
pub fn static_method() {}
}

/// annotated enum type
#[wasm_bindgen]
pub enum AnnotatedEnum {
/// annotated enum variant 1
Variant1,
/// annotated enum variant 2
Variant2,
}

#[wasm_bindgen_test]
Expand Down