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

Add quote integration as optional printing feature #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
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
8 changes: 5 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "litrs"
version = "0.4.0"
version = "0.4.1"
Copy link
Owner

Choose a reason for hiding this comment

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

I will bump the version as part of my release process. At least in my workflow, this doesn't belong into feature commits.

authors = ["Lukas Kalbertodt <[email protected]>"]
edition = "2018"
rust-version = "1.54"
Expand All @@ -25,9 +25,11 @@ exclude = [".github"]


[features]
default = ["proc-macro2"]
default = ["proc-macro2", "printing"]
Copy link
Owner

Choose a reason for hiding this comment

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

Don't make it a default feature. In fact, I consider removing proc-macro2 from the default features as well.

check_suffix = ["unicode-xid"]
printing = ["dep:quote"]
Copy link
Owner

Choose a reason for hiding this comment

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

Mh this feature requires Rust 1.60, but currently this is targetting 1.54. So if we want this, I would only release that with a major version bump. While I dislike implicit features and do think dep: is a step in the right direction, I think this isn't worth it. So please change this to just ["quote"].

Also, independently: I guess this should also mention "proc-macro2" as that's required to implement the printing feature.


[dependencies]
proc-macro2 = { version = "1", optional = true }
proc-macro2 = { version = "1", optional = true }
unicode-xid = { version = "0.2.4", optional = true }
quote = { version = "1", optional = true, no-default-features = true }
Comment on lines -32 to +35
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't align it like that. Just one space after the comma. and before the =.

3 changes: 3 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ mod test_util;
#[cfg(test)]
mod tests;

#[cfg(feature = "printing")]
mod printing;

mod bool;
mod byte;
mod bytestr;
Expand Down
44 changes: 44 additions & 0 deletions src/printing/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use quote::{ToTokens, TokenStreamExt};

use crate::Buffer;

macro_rules! to_tokens_simple {
($id:ident) => {
impl<B: Buffer + Clone> ToTokens for crate::$id<B> {
fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) {
tokens.append(<Self as Into<proc_macro2::Literal>>::into(self.clone()))
Copy link
Owner

Choose a reason for hiding this comment

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

That shouldn't require a clone. I suppose there should be From<&Literal> for proc_macro2::Literal impls hu? The only thing the current impl (for non references) does is call parse on the raw_input(). That works with a reference as well. Can you add these impls in a separate commit? And then you can use them here and remove the Clone bound. If you prefer I can also add those impls.

Also, tokens.append(proc_macro2::Literal::from(self)) should work as well?

}

fn into_token_stream(self) -> proc_macro2::TokenStream
where
Self: Sized,
{
proc_macro2::TokenStream::from(proc_macro2::TokenTree::from(<Self as Into<
proc_macro2::Literal,
>>::into(self)))
}
Comment on lines +12 to +19
Copy link
Owner

Choose a reason for hiding this comment

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

And I suppose if we remove the Clone bound, this also can be removed.

}
};
}
to_tokens_simple!(ByteLit);
to_tokens_simple!(ByteStringLit);
to_tokens_simple!(CharLit);
to_tokens_simple!(FloatLit);
to_tokens_simple!(IntegerLit);
to_tokens_simple!(StringLit);

impl ToTokens for crate::BoolLit {
fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) {
use crate::BoolLit::*;
tokens.append(proc_macro2::Ident::new(
match self {
True => "true",
False => "false",
},
proc_macro2::Span::call_site(),
))
Comment on lines +32 to +39
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
use crate::BoolLit::*;
tokens.append(proc_macro2::Ident::new(
match self {
True => "true",
False => "false",
},
proc_macro2::Span::call_site(),
))
tokens.append(proc_macro2::Ident::from(self));

Should work?

}
}

#[cfg(test)]
mod tests;
82 changes: 82 additions & 0 deletions src/printing/tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
use proc_macro2::{Literal, TokenTree};
use quote::ToTokens;

use crate::{ByteLit, ByteStringLit, CharLit, FloatLit, IntegerLit, StringLit};

#[test]
fn it_preserves_bool_true() {
let other = crate::BoolLit::True
.to_token_stream()
.into_iter()
.next()
.and_then(|v| {
if let TokenTree::Ident(v) = v {
Some(v)
} else {
None
}
})
.unwrap();
assert_eq!(other, "true")
Comment on lines +8 to +20
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let other = crate::BoolLit::True
.to_token_stream()
.into_iter()
.next()
.and_then(|v| {
if let TokenTree::Ident(v) = v {
Some(v)
} else {
None
}
})
.unwrap();
assert_eq!(other, "true")
let v = crate::BoolLit::True.to_token_stream().into_iter().collect::<Vec<_>>();
assert_eq!(v.len(), 1);
let TokenTree::Ident(ident) = v[0] else { panic!("not an ident") };
assert_eq!(ident, "true");

How about this? This also checks that there are no extra token trees in the stream.

}
#[test]
fn it_preserves_bool_false() {
let other = crate::BoolLit::False
.to_token_stream()
.into_iter()
.next()
.and_then(|v| {
if let TokenTree::Ident(v) = v {
Some(v)
} else {
None
}
})
.unwrap();
assert_eq!(other, "false")
}

// NOTE: sometimes the round-trip to quote simplifies literals (for example float literals)
// this is something that has to be taken into consideration when writing tests
macro_rules! preservation {
($name:ident : $ty:ident : $($content:tt)+) => {
#[test]
fn $name() {
use std::convert::TryFrom;

let lit = Literal::$($content)+;

let lhs = $ty::try_from(lit).unwrap();

let rhs = $ty::try_from(
lhs.to_token_stream()
.into_iter()
.next()
.and_then(|v| {
if let TokenTree::Literal(v) = v {
Some(v)
} else {
None
}
})
Comment on lines +55 to +61
Copy link
Owner

Choose a reason for hiding this comment

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

That shouldn't be necessary as a TryFrom impl also exists for TokenTree, not only Literal, right?

.unwrap(),
)
.unwrap();

assert_eq!(lhs, rhs);
}
}
}

preservation!(it_preserves_u8_suffixed : IntegerLit : u8_suffixed(10));
preservation!(it_preserves_u8_unsuffixed : IntegerLit : u8_unsuffixed(10));

preservation!(it_preserves_f64_suffixed : FloatLit : f64_suffixed(1.1));
preservation!(it_preserves_f64_unsuffixed : FloatLit : f64_unsuffixed(1.1));

preservation!(it_preserves_strings : StringLit : string("hello world"));
preservation!(it_preserves_raw_strings : StringLit : string(r#"hello world"#));

preservation!(it_preserves_char : CharLit : character('1'));

preservation!(it_preserves_bytestring : ByteStringLit : byte_string(b"hello world"));