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

fix: use internal QuotedString wrapper to quote Quil strings correctly #317

Merged
merged 1 commit into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
6 changes: 3 additions & 3 deletions quil-rs/src/instruction/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{collections::HashMap, str::FromStr};

use nom_locate::LocatedSpan;

use super::{MemoryReference, Qubit, WaveformInvocation};
use super::{MemoryReference, Qubit, QuotedString, WaveformInvocation};
use crate::{
expression::Expression,
parser::{common::parse_frame_identifier, lex, ParseError},
Expand All @@ -24,7 +24,7 @@ impl Quil for AttributeValue {
) -> crate::quil::ToQuilResult<()> {
use AttributeValue::*;
match self {
String(value) => write!(f, "{value:?}").map_err(Into::into),
String(value) => write!(f, "{}", QuotedString(value)).map_err(Into::into),
Expression(value) => value.write(f, fall_back_to_debug),
}
}
Expand Down Expand Up @@ -87,7 +87,7 @@ impl Quil for FrameIdentifier {
qubit.write(writer, fall_back_to_debug)?;
write!(writer, " ")?;
}
write!(writer, "{:?}", self.name).map_err(Into::into)
write!(writer, "{}", QuotedString(&self.name)).map_err(Into::into)
}
}

Expand Down
19 changes: 19 additions & 0 deletions quil-rs/src/instruction/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,25 @@ impl Quil for Instruction {
}
}

pub(crate) struct QuotedString<S>(pub(crate) S);

impl<S> fmt::Display for QuotedString<S>
where
S: AsRef<str>,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "\"")?;
for c in self.0.as_ref().chars() {
match c {
'"' => write!(f, "\\\"")?,
'\\' => write!(f, "\\\\")?,
c => write!(f, "{}", c)?,
}
}
Comment on lines +330 to +336
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect String::replace would be faster than this, but we'd need to benchmark it. I don't know how much the performance of this conversion matters, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is specifically inside of Display, so I think this might actually be faster. String::replace would be creating a new owned String and then calling its Display, which probably looks similar to this loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, fair point. 👍🏻

write!(f, "\"")
}
}

#[cfg(test)]
mod test_instruction_display {
use crate::{instruction::PragmaArgument, quil::Quil};
Expand Down
6 changes: 4 additions & 2 deletions quil-rs/src/instruction/pragma.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use crate::quil::Quil;

use super::QuotedString;

#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct Pragma {
pub name: String,
Expand Down Expand Up @@ -29,7 +31,7 @@ impl Quil for Pragma {
arg.write(f, fall_back_to_debug)?;
}
if let Some(data) = &self.data {
write!(f, " {data:?}")?;
write!(f, " {}", QuotedString(data))?;
}
Ok(())
}
Expand Down Expand Up @@ -66,7 +68,7 @@ impl Quil for Include {
f: &mut impl std::fmt::Write,
_fall_back_to_debug: bool,
) -> crate::quil::ToQuilResult<()> {
write!(f, r#"INCLUDE {:?}"#, self.filename).map_err(Into::into)
write!(f, r#"INCLUDE {}"#, QuotedString(&self.filename)).map_err(Into::into)
}
}

Expand Down
8 changes: 7 additions & 1 deletion quil-rs/src/parser/lexer/quoted_strings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ fn surrounded(
#[cfg(test)]
mod tests {
use super::*;
use crate::instruction::QuotedString;
use nom::Finish;
use nom_locate::LocatedSpan;
use rstest::rstest;
Expand All @@ -107,12 +108,17 @@ mod tests {
#[case("\"foo bar (baz) 123\" after", "foo bar (baz) 123", " after")]
#[case(r#""{\"name\": \"quoted json\"}""#, r#"{"name": "quoted json"}"#, "")]
#[case(r#""hello"\n"world""#, "hello", "\\n\"world\"")]
#[case(
"\"a \\\"string\\\" \n with newlines\"",
"a \"string\" \n with newlines",
""
)]
fn string_parser(#[case] input: &str, #[case] output: &str, #[case] leftover: &str) {
let span = LocatedSpan::new(input);
let (remaining, parsed) = unescaped_quoted_string(span).finish().unwrap();
assert_eq!(parsed, output);
assert_eq!(remaining.fragment(), &leftover);
let round_tripped = format!("{:?}", parsed);
let round_tripped = QuotedString(&parsed).to_string();
assert!(
input.starts_with(&round_tripped),
"expected `{}` to start with `{}`",
Expand Down
3 changes: 2 additions & 1 deletion quil-rs/src/parser/token.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::instruction::QuotedString;
use crate::parser::lexer::{Command, DataType, LexInput, LexResult, Modifier, Operator};
use std::fmt;
use std::fmt::Formatter;
Expand Down Expand Up @@ -125,7 +126,7 @@ impl fmt::Display for Token {
Token::RParenthesis => write!(f, ")"),
Token::Semicolon => write!(f, ";"),
Token::Sharing => write!(f, "SHARING"),
Token::String(s) => write!(f, "{s:?}"),
Token::String(s) => write!(f, "{}", QuotedString(s)),
Token::Variable(v) => write!(f, "{v}"),
}
}
Expand Down
Loading