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

Statically evaluate constants referenced by macros #9487

Merged
merged 8 commits into from
Jan 21, 2024
2 changes: 1 addition & 1 deletion crates/node-bindings/src/transformer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ pub fn transform(opts: JsObject, env: Env) -> napi::Result<JsUnknown> {
mod native_only {
use super::*;
use crossbeam_channel::{Receiver, Sender};
use indexmap::IndexMap;
use napi::{
threadsafe_function::{ThreadSafeCallContext, ThreadsafeFunctionCallMode},
JsBoolean, JsFunction, JsNumber, JsString, ValueType,
};
use parcel_js_swc_core::{JsValue, SourceLocation};
use std::sync::Arc;
use indexmap::IndexMap;

// Allocate a single channel per thread to communicate with the JS thread.
thread_local! {
Expand Down
116 changes: 116 additions & 0 deletions packages/core/integration-tests/test/macros.js
Original file line number Diff line number Diff line change
Expand Up @@ -604,4 +604,120 @@ describe('macros', function () {
assert(res.includes(`output${i}="2a2300bbd7ea6e9a"`));
}
});

it('should throw a diagnostic when a constant is mutated', async function () {
await fsFixture(overlayFS, dir)`
index.js:
import { hashString } from "@parcel/rust" with { type: "macro" };
const object = {foo: 'bar'};
object.foo = 'test';
output = hashString(object.foo);
`;

try {
await bundle(path.join(dir, '/index.js'), {
inputFS: overlayFS,
mode: 'production',
});
} catch (err) {
assert.deepEqual(err.diagnostics, [
{
message: 'Could not statically evaluate macro argument',
origin: '@parcel/transformer-js',
codeFrames: [
{
filePath: path.join(dir, 'index.js'),
codeHighlights: [
{
message: undefined,
start: {
line: 3,
column: 1,
},
end: {
line: 3,
column: 19,
},
},
],
},
],
hints: null,
},
]);
}
});

it('should throw a diagnostic when a constant object is passed to a function', async function () {
await fsFixture(overlayFS, dir)`
index.js:
import { hashString } from "@parcel/rust" with { type: "macro" };
const bar = 'bar';
const object = {foo: bar};
doSomething(bar); // ok (string)
doSomething(object.foo); // ok (evaluates to a string)
doSomething(object); // error (object could be mutated)
output = hashString(object.foo);

const object2 = {foo: bar, obj: {}};
doSomething(object2.obj); // error (object could be mutated)
output2 = hashString(object2);
Copy link
Member Author

Choose a reason for hiding this comment

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

The above could potentially be fine if object2.obj is not accessed in a macro, e.g. if object2.foo were passed here. But tracking mutations for individual values would be a bit more complicated than only tracking it for constants as a whole.

`;

try {
await bundle(path.join(dir, '/index.js'), {
inputFS: overlayFS,
mode: 'production',
});
} catch (err) {
assert.deepEqual(err.diagnostics, [
{
message: 'Could not statically evaluate macro argument',
origin: '@parcel/transformer-js',
codeFrames: [
{
filePath: path.join(dir, 'index.js'),
codeHighlights: [
{
message: undefined,
start: {
line: 6,
column: 13,
},
end: {
line: 6,
column: 18,
},
},
],
},
],
hints: null,
},
{
message: 'Could not statically evaluate macro argument',
origin: '@parcel/transformer-js',
codeFrames: [
{
filePath: path.join(dir, 'index.js'),
codeHighlights: [
{
message: undefined,
start: {
line: 10,
column: 13,
},
end: {
line: 10,
column: 19,
},
},
],
},
],
hints: null,
},
]);
}
});
});
83 changes: 74 additions & 9 deletions packages/transformers/js/core/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ pub struct Macros<'a> {
callback: MacroCallback,
source_map: &'a SourceMap,
diagnostics: &'a mut Vec<Diagnostic>,
assignment_span: Option<Span>,
in_call: bool,
}

struct MacroImport {
Expand All @@ -48,6 +50,8 @@ impl<'a> Macros<'a> {
callback,
source_map,
diagnostics,
assignment_span: None,
in_call: false,
}
}

Expand Down Expand Up @@ -89,7 +93,7 @@ impl<'a> Macros<'a> {
}
}

fn call_macro(&self, src: &JsWord, export: &JsWord, call: &CallExpr) -> Result<Expr, Diagnostic> {
fn call_macro(&self, src: String, export: String, call: CallExpr) -> Result<Expr, Diagnostic> {
// Try to statically evaluate all of the function arguments.
let mut args = Vec::with_capacity(call.args.len());
for arg in &call.args {
Expand All @@ -111,7 +115,7 @@ impl<'a> Macros<'a> {

// If that was successful, call the function callback (on the JS thread).
let loc = SourceLocation::from(self.source_map, call.span);
match (self.callback)(src.to_string(), export.to_string(), args, loc.clone()) {
match (self.callback)(src, export, args, loc.clone()) {
Ok(val) => Ok(self.value_to_expr(val)?),
Err(err) => Err(Diagnostic {
message: format!("Error evaluating macro: {}", err),
Expand Down Expand Up @@ -163,17 +167,18 @@ impl<'a> Fold for Macros<'a> {
node
}

fn fold_expr(&mut self, mut node: Expr) -> Expr {
node = node.fold_children_with(self);

if let Expr::Call(call) = &node {
fn fold_expr(&mut self, node: Expr) -> Expr {
if let Expr::Call(call) = node {
if let Callee::Expr(expr) = &call.callee {
match &**expr {
Expr::Ident(ident) => {
if let Some(specifier) = self.macros.get(&ident.to_id()) {
if let Some(imported) = &specifier.imported {
let specifier = specifier.src.to_string();
let imported = imported.to_string();
let call = call.fold_with(self);
return handle_error(
self.call_macro(&specifier.src, imported, call),
self.call_macro(specifier, imported, call),
&mut self.diagnostics,
);
}
Expand All @@ -188,8 +193,11 @@ impl<'a> Fold for Macros<'a> {
) {
// Check that this is a namespace import.
if specifier.imported.is_none() {
let specifier = specifier.src.to_string();
let imported = prop.0.to_string();
let call = call.fold_with(self);
return handle_error(
self.call_macro(&specifier.src, &prop.0, call),
self.call_macro(specifier, imported, call),
&mut self.diagnostics,
);
}
Expand All @@ -199,9 +207,16 @@ impl<'a> Fold for Macros<'a> {
_ => {}
}
}

// Not a macro. Track if we're in a call so we can error if constant
// objects are referenced that might be mutated.
self.in_call = true;
let call = call.fold_with(self);
self.in_call = false;
return Expr::Call(call);
}

node
node.fold_children_with(self)
}

fn fold_var_decl(&mut self, mut node: VarDecl) -> VarDecl {
Expand All @@ -218,6 +233,56 @@ impl<'a> Fold for Macros<'a> {

node
}

fn fold_assign_expr(&mut self, mut node: AssignExpr) -> AssignExpr {
self.assignment_span = Some(node.span.clone());
node.left = node.left.fold_with(self);
self.assignment_span = None;

node.right = node.right.fold_with(self);
node
}

fn fold_member_expr(&mut self, node: MemberExpr) -> MemberExpr {
if let Some(assignment_span) = self.assignment_span {
// Error when re-assigning a property of a constant that's used in a macro.
let node = node.fold_children_with(self);
if let Expr::Ident(id) = &*node.obj {
if let Some(constant) = self.constants.get_mut(&id.to_id()) {
if constant.is_ok() {
*constant = Err(assignment_span.clone());
}
}
}

return node;
} else if self.in_call {
// We need to error when passing a constant object into a non-macro call, since it might be mutated.
// If the member expression evaluates to an object, continue traversing so we error in fold_ident.
// Otherwise, return early to allow other properties to be accessed without error.
let value = self
.eval(&*node.obj)
.and_then(|obj| self.eval_member_prop(obj, &node));
if !matches!(value, Ok(JsValue::Object(..))) {
return node;
}
}

node.fold_children_with(self)
}

fn fold_ident(&mut self, node: Ident) -> Ident {
if self.in_call {
if let Some(constant) = self.constants.get_mut(&node.to_id()) {
if matches!(constant, Ok(JsValue::Object(..))) {
// Mark access to constant object inside a call as an error since it could potentially be mutated.
*constant = Err(node.span.clone());
}
}
}

node
}
}

/// Checks if an object literal (from import attributes) has type: 'macro'.
Expand Down