Don't discard attributes on string enums variants

[rrbutani]

The only important thing this PR does is make it so doc comments (and other attributes) on string enums actually make their way into the enums that get generated. In other words, it makes it so that the comments and deprecated attributes on the variants in this:

/// The type of the bell notification the terminal will use.
#[wasm_bindgen]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum BellStyle {
    /// No bell notification.
    None = "none",
    /// [Removed](https://github.com/xtermjs/xterm.js/issues/1155).
    #[deprecated(
        since = "3.0.0",
        note = "Removed!"
    )]
    /// A visual bell notification.
    Visual = "visual",
   <snipped>
}

don't get discarded.

I ended up actually changing ast::ImportEnum to store all the attrs for each variant in the enum and outputting all of them in the ToTokens impl for ImportEnum; this matches the behaviour for regular enums (though, for string enums, the attributes have to be stored in ast::ImportEnum because we can't just output all the tokens that are part of the enum right away like the parser for regular enums does).

I also changed ast::ImportEnum to use Variant like regular enums do because I was hoping to also emit the doc comments in the generated typescript bindings. I didn't actually get as far as doing this (string enums seem like a special case; since they're treated like exports, afaict, they don't have typescript bindings with their variants) and wasn't sure what the best way to do so would be. If this seems useful and anyone has suggestions about how to implement this, I'm happy to update the PR; otherwise I can switch back to just storing the variant name and value and roll back the changes to ast::Variant.

Finally, I also added an impl of FromStr for string enums; if this would be considered a breaking change or is undesirable I can remove it.

Apologies for the mishmash of changes; I can break things up into separate PRs if needed.

[alexcrichton]

Thanks! Could some tests be added which show how the new preserved strings show up in JS?

[alexcrichton]

These look like unrelated changes?

[rrbutani]

They definitely are; I added a few allows (missing_docs and deprecated for the from_str/to_str functions and unsafe_code wherever unsafe was used) but I can spin these off into their own PR or just remove them if they're problematic.

[alexcrichton]

Ah yeah I think it's best to have a dedicated PR for things like this if that's ok

[rrbutani]

Sure, np. I'll make another PR

[alexcrichton]

A type parameter here seems a bit like overkill, is this used in multiple locations to warrant that?

[rrbutani]

It's used by ast::ImportEnum where Val = String, but this isn't actually used for preserving attributes. As mentioned, I made this change so that TypeScript bindings for string enums could have the docs, but I wasn't sure if this would be considered useful/how to best do this.

[alexcrichton]

Would it be possible to either update this structure to store a u32 or String? Is it still needed in two different contexts at the same time?

[alexcrichton]

I'd personally prefer that if we auto-generate this impl that a more appropriate Err type is selected for use here.

[rrbutani]

Sure; I can make this change.

What would an appropriate Err type be? Something like:

struct StringEnumUnknownVariant<'s> {
  // (with functions instead of public fields)
  pub valid_variants: &'static [&'static str],
  pub received: &'s str,
}

or just a str with the invalid variant string or something else?

[alexcrichton]

I haven't really thought about what the best error type would be, but designing good error types is unfortunately not trivial. I would prefer if we didn't auto-generate this for now and left it to users to implement if they need it.

[rrbutani]

👍 I'll remove it from this PR

[rrbutani]

@alexcrichton Thanks for the review!

So, the doc strings (for string enum variants) don't currently show up anywhere in JS. If I understand correctly, the variant names for string enums themselves don't show up on the JS side at all.

I wanted to have the doc strings show up in the TypeScript bindings which is why I made Variant generic and used it in ast::ImportEnum.

If this is something that seems useful, I can give it another go (pointers as to how to best do this would be appreciated if you have time but if not I can try to figure it out). If not, I can remove the changes to Variant and just not use it in ast::ImportEnum; the only changes left then would be the bits that copy over string enum variant attributes to the generated Rust enum.

[alexcrichton]

Ah sorry I'm mostly just looking for tests of some form, not necessarily a particular kind of test. There's reference output tests in crates/cli/tests which may be a good place to add this.