From 68c173a1c6923c865b64bbea78f358969b71cc96 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Fri, 19 Jan 2024 13:00:55 -0500 Subject: [PATCH 1/2] Improve macro error handling --- crates/node-bindings/src/transformer.rs | 23 +++-- packages/core/core/src/Transformation.js | 52 ++++++----- .../core/core/src/requests/DevDepRequest.js | 7 +- .../core/integration-tests/test/macros.js | 87 +++++++++++++++++-- packages/transformers/js/core/src/lib.rs | 2 +- packages/transformers/js/core/src/macros.rs | 73 ++++++++++++---- packages/transformers/js/src/JSTransformer.js | 18 ++-- 7 files changed, 202 insertions(+), 60 deletions(-) diff --git a/crates/node-bindings/src/transformer.rs b/crates/node-bindings/src/transformer.rs index 19fbcb605b1..36d1a0fc0d8 100644 --- a/crates/node-bindings/src/transformer.rs +++ b/crates/node-bindings/src/transformer.rs @@ -18,12 +18,12 @@ mod native_only { threadsafe_function::{ThreadSafeCallContext, ThreadsafeFunctionCallMode}, JsBoolean, JsFunction, JsNumber, JsString, ValueType, }; - use parcel_js_swc_core::{JsValue, SourceLocation}; + use parcel_js_swc_core::{JsValue, MacroError, SourceLocation}; use std::sync::Arc; // Allocate a single channel per thread to communicate with the JS thread. thread_local! { - static CHANNEL: (Sender>, Receiver>) = crossbeam_channel::unbounded(); + static CHANNEL: (Sender>, Receiver>) = crossbeam_channel::unbounded(); } struct CallMacroMessage { @@ -33,6 +33,12 @@ mod native_only { loc: SourceLocation, } + #[napi(object)] + struct JsMacroError { + pub kind: u32, + pub message: String, + } + #[napi] pub fn transform_async(opts: JsObject, env: Env) -> napi::Result { let call_macro_tsfn = if opts.has_named_property("callMacro")? { @@ -203,7 +209,7 @@ mod native_only { fn await_promise( env: Env, result: JsUnknown, - tx: Sender>, + tx: Sender>, ) -> napi::Result<()> { // If the result is a promise, wait for it to resolve, and send the result to the channel. // Otherwise, send the result immediately. @@ -217,12 +223,13 @@ mod native_only { ctx.env.get_undefined() })?; let eb = env.create_function_from_closure("error_callback", move |ctx| { - let res = ctx.get::(0)?; - let message = match napi_to_js_value(res, env)? { - JsValue::String(s) => s, - _ => "Unknown error".into(), + let res = ctx.get::(0)?; + let err = match res.kind { + 1 => MacroError::LoadError(res.message), + 2 => MacroError::ExecutionError(res.message), + _ => MacroError::LoadError("Invalid error kind".into()), }; - tx2.send(Err(message)).expect("send failure"); + tx2.send(Err(err)).expect("send failure"); ctx.env.get_undefined() })?; then.call(Some(&result), &[cb, eb])?; diff --git a/packages/core/core/src/Transformation.js b/packages/core/core/src/Transformation.js index b27edded41f..ca25f703d85 100644 --- a/packages/core/core/src/Transformation.js +++ b/packages/core/core/src/Transformation.js @@ -264,24 +264,24 @@ export default class Transformation { initialAsset: UncommittedAsset, ): Promise> { let initialType = initialAsset.value.type; - let assets: Array = await this.runPipeline( - pipeline, - initialAsset, - ); - - // Add dev dep requests for each transformer - for (let transformer of pipeline.transformers) { - await this.addDevDependency({ - specifier: transformer.name, - resolveFrom: transformer.resolveFrom, - range: transformer.range, - }); - } + let assets: Array; + try { + assets = await this.runPipeline(pipeline, initialAsset); + } finally { + // Add dev dep requests for each transformer + for (let transformer of pipeline.transformers) { + await this.addDevDependency({ + specifier: transformer.name, + resolveFrom: transformer.resolveFrom, + range: transformer.range, + }); + } - // Add dev dep requests for dependencies of transformer plugins - // (via proxied packageManager.require calls). - for (let devDep of this.pluginDevDeps) { - await this.addDevDependency(devDep); + // Add dev dep requests for dependencies of transformer plugins + // (via proxied packageManager.require calls). + for (let devDep of this.pluginDevDeps) { + await this.addDevDependency(devDep); + } } let finalAssets: Array = []; @@ -324,13 +324,17 @@ export default class Transformation { } // Ensure that the package manager has an entry for this resolution. - await this.options.packageManager.resolve( - specifier, - fromProjectPath(this.options.projectRoot, opts.resolveFrom), - { - range, - }, - ); + try { + await this.options.packageManager.resolve( + specifier, + fromProjectPath(this.options.projectRoot, opts.resolveFrom), + { + range, + }, + ); + } catch (err) { + // ignore + } let devDepRequest = await createDevDependency( opts, diff --git a/packages/core/core/src/requests/DevDepRequest.js b/packages/core/core/src/requests/DevDepRequest.js index 8eb51124069..eb1d93d5aa4 100644 --- a/packages/core/core/src/requests/DevDepRequest.js +++ b/packages/core/core/src/requests/DevDepRequest.js @@ -49,7 +49,12 @@ export async function createDevDependency( let resolveFromAbsolute = fromProjectPath(options.projectRoot, resolveFrom); // Ensure that the package manager has an entry for this resolution. - await options.packageManager.resolve(specifier, resolveFromAbsolute); + try { + await options.packageManager.resolve(specifier, resolveFromAbsolute); + } catch (err) { + // ignore + } + let invalidations = options.packageManager.getInvalidations( specifier, resolveFromAbsolute, diff --git a/packages/core/integration-tests/test/macros.js b/packages/core/integration-tests/test/macros.js index 487a34584c5..c5f27cf4f89 100644 --- a/packages/core/integration-tests/test/macros.js +++ b/packages/core/integration-tests/test/macros.js @@ -1,7 +1,14 @@ // @flow strict-local import assert from 'assert'; import path from 'path'; -import {bundle, run, overlayFS, fsFixture} from '@parcel/test-utils'; +import { + bundle, + bundler, + run, + overlayFS, + fsFixture, + getNextBuild, +} from '@parcel/test-utils'; describe('macros', function () { let count = 0; @@ -368,7 +375,7 @@ describe('macros', function () { } catch (err) { assert.deepEqual(err.diagnostics, [ { - message: `Error evaluating macro: Could not resolve module "./macro.js" from "${path.join( + message: `Error loading macro: Could not resolve module "./macro.js" from "${path.join( dir, 'index.js', )}"`, @@ -380,12 +387,12 @@ describe('macros', function () { { message: undefined, start: { - line: 2, - column: 10, + line: 1, + column: 1, }, end: { - line: 2, - column: 19, + line: 1, + column: 57, }, }, ], @@ -561,6 +568,74 @@ describe('macros', function () { assert.notEqual(match[1], match2[1]); }); + it('should only error once if a macro errors during loading', async function () { + await fsFixture(overlayFS, dir)` + index.js: + import { test } from "./macro.js" with { type: "macro" }; + output = test(1, 2); + output2 = test(1, 3); + + macro.js: + export function test() { + return Date.now( + } + `; + + try { + await bundle(path.join(dir, '/index.js'), { + inputFS: overlayFS, + mode: 'production', + }); + } catch (err) { + assert.equal(err.diagnostics.length, 1); + } + }); + + it('should rebuild in watch mode after fixing an error', async function () { + await fsFixture(overlayFS, dir)` + index.js: + import { test } from "./macro.ts" with { type: "macro" }; + output = test('test.txt'); + + macro.ts: + export function test() { + return Date.now( + } + `; + + let b = await bundler(path.join(dir, '/index.js'), { + inputFS: overlayFS, + mode: 'production', + shouldDisableCache: false, + }); + + let subscription; + try { + subscription = await b.watch(); + let buildEvent = await getNextBuild(b); + assert.equal(buildEvent.type, 'buildFailure'); + + await fsFixture(overlayFS, dir)` + macro.ts: + export function test() { + return Date.now(); + } + `; + + buildEvent = await getNextBuild(b); + assert.equal(buildEvent.type, 'buildSuccess'); + + let res = await overlayFS.readFile( + buildEvent.bundleGraph.getBundles()[0].filePath, + 'utf8', + ); + let match = res.match(/output=(\d+)/); + assert(match); + } finally { + await subscription.unsubscribe(); + } + }); + it('should support evaluating constants', async function () { await fsFixture(overlayFS, dir)` index.js: diff --git a/packages/transformers/js/core/src/lib.rs b/packages/transformers/js/core/src/lib.rs index 9ab67c2179a..cbaad41506f 100644 --- a/packages/transformers/js/core/src/lib.rs +++ b/packages/transformers/js/core/src/lib.rs @@ -53,8 +53,8 @@ use node_replacer::NodeReplacer; use typeof_replacer::*; use utils::{error_buffer_to_diagnostics, Diagnostic, DiagnosticSeverity, ErrorBuffer, SourceType}; -pub use crate::macros::JsValue; use crate::macros::Macros; +pub use crate::macros::{JsValue, MacroError}; pub use utils::SourceLocation; type SourceMapBuffer = Vec<(swc_core::common::BytePos, swc_core::common::LineCol)>; diff --git a/packages/transformers/js/core/src/macros.rs b/packages/transformers/js/core/src/macros.rs index 87e3fa5617f..d374a131921 100644 --- a/packages/transformers/js/core/src/macros.rs +++ b/packages/transformers/js/core/src/macros.rs @@ -16,8 +16,15 @@ use crate::utils::{ ErrorBuffer, SourceLocation, }; +pub enum MacroError { + /// An error occurred loading a macro (e.g. resolution or syntax error). + LoadError(String), + /// An error was thrown when executing a macro. + ExecutionError(String), +} + pub type MacroCallback = Arc< - dyn Fn(String, String, Vec, SourceLocation) -> Result + Send + Sync, + dyn Fn(String, String, Vec, SourceLocation) -> Result + Send + Sync, >; pub struct Macros<'a> { @@ -27,6 +34,7 @@ pub struct Macros<'a> { callback: MacroCallback, source_map: &'a SourceMap, diagnostics: &'a mut Vec, + load_errors: HashSet, assignment_span: Option, in_call: bool, } @@ -36,6 +44,8 @@ struct MacroImport { src: JsWord, /// The imported identifier. None if this is a namespace import. imported: Option, + /// The location of the import specifier. + span: Span, } impl<'a> Macros<'a> { @@ -47,6 +57,7 @@ impl<'a> Macros<'a> { Macros { macros: HashMap::new(), constants: HashMap::new(), + load_errors: HashSet::new(), callback, source_map, diagnostics, @@ -68,6 +79,7 @@ impl<'a> Macros<'a> { MacroImport { src: import.src.value.clone(), imported: Some(imported), + span: import.span, }, ); } @@ -77,6 +89,7 @@ impl<'a> Macros<'a> { MacroImport { src: import.src.value.clone(), imported: Some(js_word!("default")), + span: import.span, }, ); } @@ -86,6 +99,7 @@ impl<'a> Macros<'a> { MacroImport { src: import.src.value.clone(), imported: None, + span: import.span, }, ); } @@ -93,7 +107,18 @@ impl<'a> Macros<'a> { } } - fn call_macro(&self, src: String, export: String, call: CallExpr) -> Result { + fn call_macro( + &mut self, + src: String, + export: String, + call: CallExpr, + import_span: Span, + ) -> Result { + // If a macro already errorered during loading, don't try callinbg it again. + if self.load_errors.contains(&src) { + return Ok(Expr::Lit(Lit::Null(Null::dummy()))); + } + // Try to statically evaluate all of the function arguments. let mut args = Vec::with_capacity(call.args.len()); for arg in &call.args { @@ -115,16 +140,32 @@ 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, export, args, loc.clone()) { + match (self.callback)(src.clone(), export, args, loc.clone()) { Ok(val) => Ok(self.value_to_expr(val)?), - Err(err) => Err(Diagnostic { - message: format!("Error evaluating macro: {}", err), - code_highlights: Some(vec![CodeHighlight { message: None, loc }]), - hints: None, - show_environment: false, - severity: crate::utils::DiagnosticSeverity::Error, - documentation_url: None, - }), + Err(err) => match err { + MacroError::LoadError(err) => { + self.load_errors.insert(src); + Err(Diagnostic { + message: format!("Error loading macro: {}", err), + code_highlights: Some(vec![CodeHighlight { + message: None, + loc: SourceLocation::from(self.source_map, import_span), + }]), + hints: None, + show_environment: false, + severity: crate::utils::DiagnosticSeverity::Error, + documentation_url: None, + }) + } + MacroError::ExecutionError(err) => Err(Diagnostic { + message: format!("Error evaluating macro: {}", err), + code_highlights: Some(vec![CodeHighlight { message: None, loc }]), + hints: None, + show_environment: false, + severity: crate::utils::DiagnosticSeverity::Error, + documentation_url: None, + }), + }, } } @@ -174,11 +215,12 @@ impl<'a> Fold for Macros<'a> { 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 src = specifier.src.to_string(); let imported = imported.to_string(); + let span = specifier.span; let call = call.fold_with(self); return handle_error( - self.call_macro(specifier, imported, call), + self.call_macro(src, imported, call, span), &mut self.diagnostics, ); } @@ -193,11 +235,12 @@ 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 src = specifier.src.to_string(); let imported = prop.0.to_string(); + let span = specifier.span; let call = call.fold_with(self); return handle_error( - self.call_macro(specifier, imported, call), + self.call_macro(src, imported, call, span), &mut self.diagnostics, ); } diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index df943837700..edc88bd52df 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -474,15 +474,20 @@ export default (new Transformer({ inline_constants: config.inlineConstants, callMacro: asset.isSource ? async (err, src, exportName, args, loc) => { + let mod; try { - let mod = await options.packageManager.require( - src, - asset.filePath, - ); + mod = await options.packageManager.require(src, asset.filePath); if (!Object.hasOwnProperty.call(mod, exportName)) { throw new Error(`"${src}" does not export "${exportName}".`); } + } catch (err) { + throw { + kind: 1, + message: err.message, + }; + } + try { if (typeof mod[exportName] === 'function') { let ctx: MacroContext = { // Allows macros to emit additional assets to add as dependencies (e.g. css). @@ -564,7 +569,10 @@ export default (new Transformer({ } message += '\n' + line; } - throw message; + throw { + kind: 2, + message, + }; } } : null, From c933eedcbc3daa8ad238031fc4f6b79d00f745c8 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Fri, 19 Jan 2024 13:19:43 -0500 Subject: [PATCH 2/2] flow --- packages/core/integration-tests/test/macros.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/integration-tests/test/macros.js b/packages/core/integration-tests/test/macros.js index c5f27cf4f89..9345c1552b7 100644 --- a/packages/core/integration-tests/test/macros.js +++ b/packages/core/integration-tests/test/macros.js @@ -1,5 +1,6 @@ // @flow strict-local import assert from 'assert'; +import invariant from 'assert'; import path from 'path'; import { bundle, @@ -624,6 +625,7 @@ describe('macros', function () { buildEvent = await getNextBuild(b); assert.equal(buildEvent.type, 'buildSuccess'); + invariant(buildEvent.type === 'buildSuccess'); // flow let res = await overlayFS.readFile( buildEvent.bundleGraph.getBundles()[0].filePath, @@ -632,7 +634,7 @@ describe('macros', function () { let match = res.match(/output=(\d+)/); assert(match); } finally { - await subscription.unsubscribe(); + await subscription?.unsubscribe(); } });