Skip to content

Commit

Permalink
fix(compiler): assert doesn't work with string interpolation (#3038)
Browse files Browse the repository at this point in the history
Fixes #1833

The high level idea behind this fix is to avoid using backtick-strings in our generated JavaScript. These lead to a lot of issues that can be avoided by using other concatenation strategies that do not introduce grammar complexities. In this case, I decided to use `String.raw()`, but translating interpolated strings into a sequence of `+` operations probably could have worked too.

The other major change I did was improving the errors emitted by `assert()` so that it shows the original Wing source code that you asserted, not the emitted JavaScript. For example, now an error looks like:

```
assertion failed: b.list().length == 1
```





instead of:


```
assertion failed: '((await b.list()).length === 1)'
```

## Checklist

- [x] Title matches [Winglang's style guide](https://docs.winglang.io/contributors/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Monada Contribution License](https://docs.winglang.io/terms-and-policies/contribution-license.html)*.
  • Loading branch information
Chriscbr authored Jun 26, 2023
1 parent 95d4163 commit 1a83d65
Show file tree
Hide file tree
Showing 122 changed files with 1,325 additions and 927 deletions.
36 changes: 36 additions & 0 deletions examples/tests/valid/assert.w
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
let s1 = "foo";
let s2 = "bar";

assert("" == "");
assert("'" == "'");
assert("\"" == "\"");
assert("`" == "`");
assert("``" == "``");
assert("`s1`" == "`s1`");
assert(s1 == s1);
assert("${s1}" == "${s1}");
assert("${s1}" != "${s2}");
assert("a${s1}" == "a${s1}");
assert("a${s1}" != "b${s1}");
assert("${s1}a" == "${s1}a");
assert("${s1}a" != "${s1}b");
assert("`'${s1}" == "`'${s1}");
assert("a${s1}b${s2}c" == "a${s1}b${s2}c");

test "assert works inflight" {
assert("" == "");
assert("'" == "'");
assert("\"" == "\"");
assert("`" == "`");
assert("``" == "``");
assert("`s1`" == "`s1`");
assert(s1 == s1);
assert("${s1}" == "${s1}");
assert("${s1}" != "${s2}");
assert("a${s1}" == "a${s1}");
assert("a${s1}" != "b${s1}");
assert("${s1}a" == "${s1}a");
assert("${s1}a" != "${s1}b");
assert("`'${s1}" == "`'${s1}");
assert("a${s1}b${s2}c" == "a${s1}b${s2}c");
}
2 changes: 1 addition & 1 deletion examples/tests/valid/json_string_interpolation.w
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ let obj = Json {
let notStringifyStrValue = "string: ${obj.get("strValue")}";
assert(notStringifyStrValue == "string: test");
let stringifyNumValue = "number: ${obj.get("numValue")}";
assert(stringifyNumValue == "number: 1");
assert(stringifyNumValue == "number: 1");
4 changes: 3 additions & 1 deletion libs/wingc/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,13 +494,15 @@ impl Expr {
pub struct ArgList {
pub pos_args: Vec<Expr>,
pub named_args: IndexMap<Symbol, Expr>,
pub span: WingSpan,
}

impl ArgList {
pub fn new() -> Self {
pub fn new(span: WingSpan) -> Self {
ArgList {
pos_args: vec![],
named_args: IndexMap::new(),
span,
}
}
}
Expand Down
1 change: 1 addition & 0 deletions libs/wingc/src/closure_transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ impl Fold for ClosureTransformer {
arg_list: ArgList {
named_args: IndexMap::new(),
pos_args: vec![],
span: WingSpan::default(),
},
obj_id: None,
obj_scope: None,
Expand Down
5 changes: 5 additions & 0 deletions libs/wingc/src/jsify/files.rs → libs/wingc/src/files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ impl From<FilesError> for Diagnostic {

impl Error for FilesError {}

#[derive(Default)]
pub struct Files {
data: HashMap<PathBuf, String>,
}
Expand All @@ -53,6 +54,10 @@ impl Files {
Ok(())
}

pub fn get_file<S: AsRef<Path>>(&self, path: S) -> Option<&String> {
self.data.get(path.as_ref())
}

pub fn emit_files(&self, out_dir: &Path) -> Result<(), FilesError> {
for (path, content) in &self.data {
let full_path = out_dir.join(path);
Expand Down
1 change: 1 addition & 0 deletions libs/wingc/src/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ where
.into_iter()
.map(|(name, arg)| (f.fold_symbol(name), f.fold_expr(arg)))
.collect(),
span: node.span,
}
}

Expand Down
149 changes: 120 additions & 29 deletions libs/wingc/src/jsify.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
pub mod codemaker;
mod files;

use aho_corasick::AhoCorasick;
use const_format::formatcp;
Expand All @@ -24,16 +23,18 @@ use crate::{
comp_ctx::{CompilationContext, CompilationPhase},
dbg_panic, debug,
diagnostic::{report_diagnostic, Diagnostic, WingSpan},
files::Files,
type_check::{
resolve_user_defined_type,
symbol_env::{LookupResult, SymbolEnv, SymbolEnvRef},
ClassLike, SymbolKind, Type, TypeRef, Types, UnsafeRef, VariableInfo, CLASS_INFLIGHT_INIT_NAME, HANDLE_METHOD_NAME,
},
visit::{self, Visit},
MACRO_REPLACE_ARGS, MACRO_REPLACE_SELF, WINGSDK_ASSEMBLY_NAME, WINGSDK_RESOURCE, WINGSDK_STD_MODULE,
MACRO_REPLACE_ARGS, MACRO_REPLACE_ARGS_TEXT, MACRO_REPLACE_SELF, WINGSDK_ASSEMBLY_NAME, WINGSDK_RESOURCE,
WINGSDK_STD_MODULE,
};

use self::{codemaker::CodeMaker, files::Files};
use self::codemaker::CodeMaker;

const PREFLIGHT_FILE_NAME: &str = "preflight.js";

Expand Down Expand Up @@ -62,8 +63,8 @@ pub struct JSifyContext {

pub struct JSifier<'a> {
pub types: &'a Types,
/// Stores all generated JS files in memory.
files: Files,
source_files: &'a Files,
emitted_files: Files,
/// Root of the project, used for resolving extern modules
absolute_project_root: &'a Path,
shim: bool,
Expand All @@ -79,10 +80,17 @@ enum BindMethod {
}

impl<'a> JSifier<'a> {
pub fn new(types: &'a Types, app_name: &'a str, absolute_project_root: &'a Path, shim: bool) -> Self {
pub fn new(
types: &'a Types,
source_files: &'a Files,
app_name: &'a str,
absolute_project_root: &'a Path,
shim: bool,
) -> Self {
Self {
types,
files: Files::new(),
source_files,
emitted_files: Files::new(),
shim,
app_name,
absolute_project_root,
Expand Down Expand Up @@ -180,15 +188,15 @@ impl<'a> JSifier<'a> {
output.add_code(js);
}

match self.files.add_file(PREFLIGHT_FILE_NAME, output.to_string()) {
match self.emitted_files.add_file(PREFLIGHT_FILE_NAME, output.to_string()) {
Ok(()) => {}
Err(err) => report_diagnostic(err.into()),
}
}

/// Write all files to the output directory
pub fn emit_files(&mut self, out_dir: &Path) {
match self.files.emit_files(out_dir) {
match self.emitted_files.emit_files(out_dir) {
Ok(()) => {}
Err(err) => report_diagnostic(err.into()),
}
Expand Down Expand Up @@ -337,26 +345,34 @@ impl<'a> JSifier<'a> {
}
}
ExprKind::Literal(lit) => match lit {
Literal::Nil => "undefined".to_string(),
Literal::Nil => "undefined".to_string(),
Literal::String(s) => s.to_string(),
Literal::InterpolatedString(s) => format!(
"`{}`",
s.parts
Literal::InterpolatedString(s) => {
let comma_separated_statics = s
.parts
.iter()
.map(|p| match p {
InterpolatedStringPart::Static(l) => l.to_string(),
InterpolatedStringPart::Expr(e) => {
match *self.get_expr_type(e) {
Type::Json | Type::MutJson => {
format!("${{((e) => typeof e === 'string' ? e : JSON.stringify(e, null, 2))({})}}", self.jsify_expression(e, ctx))
}
_ => format!("${{{}}}", self.jsify_expression(e, ctx)),
.filter_map(|p| match p {
InterpolatedStringPart::Static(l) => Some(format!("\"{}\"", l.to_string())),
InterpolatedStringPart::Expr(_) => None,
})
.collect::<Vec<String>>()
.join(", ");
let comma_separated_exprs = s
.parts
.iter()
.filter_map(|p| match p {
InterpolatedStringPart::Static(_) => None,
InterpolatedStringPart::Expr(e) => Some(match *self.get_expr_type(e) {
Type::Json | Type::MutJson => {
format!("((e) => typeof e === 'string' ? e : JSON.stringify(e, null, 2))({})", self.jsify_expression(e, ctx))
}
}
_ => self.jsify_expression(e, ctx),
})
})
.collect::<Vec<String>>()
.join("")
),
.join(", ");
format!("String.raw({{ raw: [{}] }}, {})", comma_separated_statics, comma_separated_exprs)
},
Literal::Number(n) => format!("{}", n),
Literal::Boolean(b) => (if *b { "true" } else { "false" }).to_string(),
},
Expand Down Expand Up @@ -386,7 +402,13 @@ impl<'a> JSifier<'a> {
ExprKind::Reference(reference) => self.jsify_reference(reference, ctx),
_ => format!("({})", self.jsify_expression(callee, ctx)),
};
let arg_string = self.jsify_arg_list(&arg_list, None, None, ctx);
let args_string = self.jsify_arg_list(&arg_list, None, None, ctx);
let mut args_text_string = lookup_span(&arg_list.span, &self.source_files);
if args_text_string.len() > 0 {
// remove the parens
args_text_string = args_text_string[1..args_text_string.len() - 1].to_string();
}
let args_text_string = escape_javascript_string(&args_text_string);

if let Some(function_sig) = function_sig {
if let Some(js_override) = &function_sig.js_override {
Expand All @@ -399,16 +421,16 @@ impl<'a> JSifier<'a> {

_ => expr_string,
};
let patterns = &[MACRO_REPLACE_SELF, MACRO_REPLACE_ARGS];
let replace_with = &[self_string, &arg_string];
let patterns = &[MACRO_REPLACE_SELF, MACRO_REPLACE_ARGS, MACRO_REPLACE_ARGS_TEXT];
let replace_with = &[self_string, &args_string, &args_text_string];
let ac = AhoCorasick::new(patterns);
return ac.replace_all(js_override, replace_with);
}
}

// NOTE: if the expression is a "handle" class, the object itself is callable (see
// `jsify_class_inflight` below), so we can just call it as-is.
format!("({auto_await}{expr_string}({arg_string}))")
format!("({auto_await}{expr_string}({args_string}))")
}
ExprKind::Unary { op, exp } => {
let js_exp = self.jsify_expression(exp, ctx);
Expand Down Expand Up @@ -1312,7 +1334,7 @@ impl<'a> JSifier<'a> {
code.line(format!("return {name};"));
code.close("}");

match self.files.add_file(inflight_filename(class), code.to_string()) {
match self.emitted_files.add_file(inflight_filename(class), code.to_string()) {
Ok(()) => {}
Err(err) => report_diagnostic(err.into()),
}
Expand Down Expand Up @@ -1961,3 +1983,72 @@ fn jsify_type_name(t: &Vec<Symbol>, phase: Phase) -> String {
p.join(".")
}
}

fn lookup_span(span: &WingSpan, files: &Files) -> String {
let source = files
.get_file(&span.file_id)
.expect(&format!("failed to find source file with id {}", span.file_id));
let lines = source.lines().collect_vec();

let start_line = span.start.line as usize;
let end_line = span.end.line as usize;

let start_col = span.start.col as usize;
let end_col = span.end.col as usize;

let mut result = String::new();

if start_line == end_line {
result.push_str(&lines[start_line][start_col..end_col]);
} else {
result.push_str(&lines[start_line][start_col..]);
result.push('\n');

for line in lines[start_line + 1..end_line].iter() {
result.push_str(line);
result.push('\n');
}

result.push_str(&lines[end_line][..end_col]);
}

result
}

fn escape_javascript_string(s: &str) -> String {
let mut result = String::new();

// escape all escapable characters -- see the section "Escape sequences" in
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#literals
for c in s.chars() {
match c {
'\0' => result.push_str("\\0"),
'\'' => result.push_str("\\'"),
'"' => result.push_str("\\\""),
'\\' => result.push_str("\\\\"),
'\n' => result.push_str("\\n"),
'\r' => result.push_str("\\r"),
'\t' => result.push_str("\\t"),
_ => result.push(c),
}
}

result
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_escape_javascript_string() {
assert_eq!(escape_javascript_string("hello"), String::from("hello"));
assert_eq!(escape_javascript_string("hello\nworld"), String::from("hello\\nworld"));
assert_eq!(escape_javascript_string("hello\rworld"), String::from("hello\\rworld"));
assert_eq!(escape_javascript_string("hello\tworld"), String::from("hello\\tworld"));
assert_eq!(escape_javascript_string("hello\\world"), String::from("hello\\\\world"));
assert_eq!(escape_javascript_string("hello'world"), String::from("hello\\'world"));
assert_eq!(escape_javascript_string("hello\"world"), String::from("hello\\\"world"));
assert_eq!(escape_javascript_string("hello\0world"), String::from("hello\\0world"));
}
}
Loading

0 comments on commit 1a83d65

Please sign in to comment.