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.

Sign up for GitHub

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

Jump to bottom

very dynamic requests will only lead to a warning #7640

Merged
merged 1 commit into from Mar 6, 2024
Merged

very dynamic requests will only lead to a warning #7640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/turbopack-ecmascript/src/lib.rs
View file
Open in desktop
Expand Up @@ -131,6 +131,10 @@ pub struct EcmascriptOptions {
/// External imports should used `__turbopack_import__` instead of
/// `__turbopack_require__` and become async module references.
pub import_externals: bool,
/// Ignore very dynamic requests which doesn't have any static known part.
/// If false, they will reference the whole directory. If true, they won't
/// reference anything and lead to an runtime error instead.
pub ignore_dynamic_requests: bool,
}

#[turbo_tasks::value(serialization = "auto_for_input")]
Expand Down
71 changes: 71 additions & 0 deletions crates/turbopack-ecmascript/src/references/dynamic_expression.rs
View file
Open in desktop
@@ -0,0 +1,71 @@
use anyhow::Result;
use serde::{Deserialize, Serialize};
use swc_core::quote;
use turbo_tasks::{debug::ValueDebugFormat, trace::TraceRawVcs, Vc};

use super::AstPath;
use crate::{
chunk::EcmascriptChunkingContext,
code_gen::{CodeGenerateable, CodeGeneration},
create_visitor,
};

#[derive(PartialEq, Eq, TraceRawVcs, Serialize, Deserialize, ValueDebugFormat)]
enum DynamicExpressionType {
Promise,
Normal,
}

#[turbo_tasks::value]
pub struct DynamicExpression {
path: Vc<AstPath>,
ty: DynamicExpressionType,
}

#[turbo_tasks::value_impl]
impl DynamicExpression {
#[turbo_tasks::function]
pub fn new(path: Vc<AstPath>) -> Vc<Self> {
Self::cell(DynamicExpression {
path,
ty: DynamicExpressionType::Normal,
})
}

#[turbo_tasks::function]
pub fn new_promise(path: Vc<AstPath>) -> Vc<Self> {
Self::cell(DynamicExpression {
path,
ty: DynamicExpressionType::Promise,
})
}
}

#[turbo_tasks::value_impl]
impl CodeGenerateable for DynamicExpression {
#[turbo_tasks::function]
async fn code_generation(
&self,
_context: Vc<Box<dyn EcmascriptChunkingContext>>,
) -> Result<Vc<CodeGeneration>> {
let path = &self.path.await?;

let visitor = match self.ty {
DynamicExpressionType::Normal => {
create_visitor!(path, visit_mut_expr(expr: &mut Expr) {
*expr = quote!("(() => { const e = new Error(\"Cannot find module as expression is too dynamic\"); e.code = 'MODULE_NOT_FOUND'; throw e; })()" as Expr);
Copy link
Member

Choose a reason for hiding this comment

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

this will completely throw away information about what would have been called

can we add something back so it shows up in the stack trace?

Copy link
Member

Choose a reason for hiding this comment

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

Giving the inline function a name would probably work

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to source mapping the error will point to the actual user code.

Copy link
Member

Choose a reason for hiding this comment

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

But the stack trace still won't contain a require/import

})
}
DynamicExpressionType::Promise => {
create_visitor!(path, visit_mut_expr(expr: &mut Expr) {
*expr = quote!("Promise.resolve().then(() => { const e = new Error(\"Cannot find module as expression is too dynamic\"); e.code = 'MODULE_NOT_FOUND'; throw e; })" as Expr);
})
}
};

Ok(CodeGeneration {
visitors: vec![visitor],
}
.cell())
}
}
101 changes: 78 additions & 23 deletions crates/turbopack-ecmascript/src/references/mod.rs
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@ pub mod async_module;
pub mod cjs;
pub mod constant_condition;
pub mod constant_value;
pub mod dynamic_expression;
pub mod esm;
pub mod node;
pub mod pattern_mapping;
Expand Down Expand Up @@ -123,6 +124,7 @@ use crate::{
references::{
async_module::{AsyncModule, OptionAsyncModule},
cjs::{CjsRequireAssetReference, CjsRequireCacheAccess, CjsRequireResolveAssetReference},
dynamic_expression::DynamicExpression,
esm::{module_id::EsmModuleIdAssetReference, EsmBinding, UrlRewriteBehavior},
node::PackageJsonReference,
require_context::{RequireContextAssetReference, RequireContextMap},
Expand Down Expand Up @@ -337,6 +339,7 @@ struct AnalysisState<'a> {
first_import_meta: bool,
tree_shaking_mode: Option<TreeShakingMode>,
import_externals: bool,
ignore_dynamic_requests: bool,
}

impl<'a> AnalysisState<'a> {
Expand Down Expand Up @@ -774,6 +777,7 @@ pub(crate) async fn analyse_ecmascript_module_internal(
first_import_meta: true,
tree_shaking_mode: options.tree_shaking_mode,
import_externals: options.import_externals,
ignore_dynamic_requests: options.ignore_dynamic_requests,
};

enum Action {
Expand Down Expand Up @@ -1063,7 +1067,10 @@ pub(crate) async fn analyse_ecmascript_module_internal(
DiagnosticId::Lint(
errors::failed_to_analyse::ecmascript::NEW_URL_IMPORT_META.to_string(),
),
)
);
sokra marked this conversation as resolved.
Show resolved Hide resolved
if options.ignore_dynamic_requests {
continue;
}
}
analysis.add_reference(UrlAssetReference::new(
origin,
Expand Down Expand Up @@ -1131,6 +1138,7 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
origin,
source,
compile_time_info,
ignore_dynamic_requests,
..
} = state;
fn explain_args(args: &[JsValue]) -> (String, String) {
Expand Down Expand Up @@ -1186,7 +1194,13 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
DiagnosticId::Lint(
errors::failed_to_analyse::ecmascript::DYNAMIC_IMPORT.to_string(),
),
)
);
if ignore_dynamic_requests {
analysis.add_code_gen(DynamicExpression::new_promise(Vc::cell(
ast_path.to_vec(),
)));
return Ok(());
}
}
analysis.add_reference(EsmAsyncAssetReference::new(
origin,
Expand Down Expand Up @@ -1219,7 +1233,11 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
DiagnosticId::Lint(
errors::failed_to_analyse::ecmascript::REQUIRE.to_string(),
),
)
);
if ignore_dynamic_requests {
analysis.add_code_gen(DynamicExpression::new(Vc::cell(ast_path.to_vec())));
return Ok(());
}
}
analysis.add_reference(CjsRequireAssetReference::new(
origin,
Expand Down Expand Up @@ -1262,7 +1280,11 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
DiagnosticId::Lint(
errors::failed_to_analyse::ecmascript::REQUIRE_RESOLVE.to_string(),
),
)
);
if ignore_dynamic_requests {
analysis.add_code_gen(DynamicExpression::new(Vc::cell(ast_path.to_vec())));
return Ok(());
}
}
analysis.add_reference(CjsRequireResolveAssetReference::new(
origin,
Expand Down Expand Up @@ -1327,7 +1349,10 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
DiagnosticId::Lint(
errors::failed_to_analyse::ecmascript::FS_METHOD.to_string(),
),
)
);
if ignore_dynamic_requests {
return Ok(());
}
}
analysis.add_reference(FileSourceReference::new(source, Pattern::new(pat)));
return Ok(());
Expand Down Expand Up @@ -1367,7 +1392,10 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
DiagnosticId::Lint(
errors::failed_to_analyse::ecmascript::PATH_METHOD.to_string(),
),
)
);
if ignore_dynamic_requests {
return Ok(());
}
}
analysis.add_reference(FileSourceReference::new(source, Pattern::new(pat)));
return Ok(());
Expand Down Expand Up @@ -1398,7 +1426,10 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
DiagnosticId::Lint(
errors::failed_to_analyse::ecmascript::PATH_METHOD.to_string(),
),
)
);
if ignore_dynamic_requests {
return Ok(());
}
}
analysis.add_reference(DirAssetReference::new(source, Pattern::new(pat)));
return Ok(());
Expand All @@ -1419,17 +1450,27 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
JsValue::member(Box::new(args[1].clone()), Box::new(0_f64.into()));
let first_arg = state.link_value(first_arg, in_try).await?;
let pat = js_value_to_pattern(&first_arg);
if !pat.has_constant_parts() {
let dynamic = !pat.has_constant_parts();
if dynamic {
show_dynamic_warning = true;
}
analysis.add_reference(CjsAssetReference::new(
origin,
Request::parse(Value::new(pat)),
issue_source(source, span),
in_try,
));
if !dynamic || !ignore_dynamic_requests {
analysis.add_reference(CjsAssetReference::new(
origin,
Request::parse(Value::new(pat)),
issue_source(source, span),
in_try,
));
}
}
let dynamic = !pat.has_constant_parts();
if dynamic {
show_dynamic_warning = true;
}
if !dynamic || !ignore_dynamic_requests {
analysis.add_reference(FileSourceReference::new(source, Pattern::new(pat)));
}
if show_dynamic_warning || !pat.has_constant_parts() {
if show_dynamic_warning {
let (args, hints) = explain_args(&args);
handler.span_warn_with_code(
span,
Expand All @@ -1439,7 +1480,6 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
),
);
}
analysis.add_reference(FileSourceReference::new(source, Pattern::new(pat)));
return Ok(());
}
let (args, hints) = explain_args(&args);
Expand All @@ -1465,6 +1505,9 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
errors::failed_to_analyse::ecmascript::CHILD_PROCESS_SPAWN.to_string(),
),
);
if ignore_dynamic_requests {
return Ok(());
}
}
analysis.add_reference(CjsAssetReference::new(
origin,
Expand Down Expand Up @@ -1499,6 +1542,7 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
errors::failed_to_analyse::ecmascript::NODE_PRE_GYP_FIND.to_string(),
),
);
// Always ignore this dynamic request
return Ok(());
sokra marked this conversation as resolved.
Show resolved Hide resolved
}
analysis.add_reference(NodePreGypConfigReference::new(
Expand Down Expand Up @@ -1588,6 +1632,7 @@ async fn handle_call<G: Fn(Vec<Effect>) + Send + Sync>(
errors::failed_to_analyse::ecmascript::NODE_EXPRESS.to_string(),
),
);
// Always ignore this dynamic request
return Ok(());
}
match s {
Expand Down Expand Up @@ -2187,19 +2232,29 @@ async fn value_visitor_inner(
if *compile_time_info.environment().node_externals().await? {
// TODO check externals
match &**name {
"path" => JsValue::WellKnownObject(WellKnownObjectKind::PathModule),
"fs/promises" => JsValue::WellKnownObject(WellKnownObjectKind::FsModule),
"fs" => JsValue::WellKnownObject(WellKnownObjectKind::FsModule),
"child_process" => JsValue::WellKnownObject(WellKnownObjectKind::ChildProcess),
"os" => JsValue::WellKnownObject(WellKnownObjectKind::OsModule),
"process" => JsValue::WellKnownObject(WellKnownObjectKind::NodeProcess),
"node:path" | "path" => {
JsValue::WellKnownObject(WellKnownObjectKind::PathModule)
}
"node:fs/promises" | "fs/promises" => {
JsValue::WellKnownObject(WellKnownObjectKind::FsModule)
}
"node:fs" | "fs" => JsValue::WellKnownObject(WellKnownObjectKind::FsModule),
"node:child_process" | "child_process" => {
JsValue::WellKnownObject(WellKnownObjectKind::ChildProcess)
}
"node:os" | "os" => JsValue::WellKnownObject(WellKnownObjectKind::OsModule),
"node:process" | "process" => {
JsValue::WellKnownObject(WellKnownObjectKind::NodeProcess)
}
"@mapbox/node-pre-gyp" => {
JsValue::WellKnownObject(WellKnownObjectKind::NodePreGyp)
}
"node-gyp-build" => {
JsValue::WellKnownFunction(WellKnownFunctionKind::NodeGypBuild)
}
"bindings" => JsValue::WellKnownFunction(WellKnownFunctionKind::NodeBindings),
"node:bindings" | "bindings" => {
JsValue::WellKnownFunction(WellKnownFunctionKind::NodeBindings)
}
"express" => JsValue::WellKnownFunction(WellKnownFunctionKind::NodeExpress),
"strong-globalize" => {
JsValue::WellKnownFunction(WellKnownFunctionKind::NodeStrongGlobalize)
Expand Down
1 change: 1 addition & 0 deletions crates/turbopack-tests/tests/snapshot.rs
View file
Open in desktop
Expand Up @@ -261,6 +261,7 @@ async fn run_test(resource: String) -> Result<Vc<FileSystemPath>> {
..Default::default()
})),
preset_env_versions: Some(env),
ignore_dynamic_requests: true,
rules: vec![(
ContextCondition::InDirectory("node_modules".to_string()),
ModuleOptionsContext {
Expand Down
17 changes: 17 additions & 0 deletions crates/turbopack-tests/tests/snapshot/dynamic-request/very-dynamic/input/index.js
View file
Open in desktop
@@ -0,0 +1,17 @@
import child_process from "node:child_process";
import fs, { readFileSync } from "node:fs";

const unknown = Math.random();

child_process.spawnSync(unknown);
child_process.spawnSync("node", unknown);
child_process.spawnSync("node", [unknown, unknown]);

require(unknown);

import(unknown);

fs.readFileSync(unknown);
readFileSync(unknown);

new URL(unknown, import.meta.url);
13 changes: 13 additions & 0 deletions ...request/very-dynamic/issues/__l___lint TP1001__ import(FreeVar(Math)[__quo__ra-fcce98.txt
View file
Open in desktop
@@ -0,0 +1,13 @@
warning - [analysis] [project]/crates/turbopack-tests/tests/snapshot/dynamic-request/very-dynamic/input/index.js /crates/turbopack-tests/tests/snapshot/dynamic-request/very-dynamic/input/index.js:12:0 lint TP1001 import(FreeVar(Math)["random"]()) is very dynamic

8 | child_process.spawnSync("node", [unknown, unknown]);
9 |
10 | require(unknown);
11 |
+ v-------------v
12 + import(unknown);
+ ^-------------^
13 |
14 | fs.readFileSync(unknown);
15 | readFileSync(unknown);
16 |
13 changes: 13 additions & 0 deletions ...request/very-dynamic/issues/__l___lint TP1002__ require(FreeVar(Math)[__quo__r-fa7bb4.txt
View file
Open in desktop
@@ -0,0 +1,13 @@
warning - [analysis] [project]/crates/turbopack-tests/tests/snapshot/dynamic-request/very-dynamic/input/index.js /crates/turbopack-tests/tests/snapshot/dynamic-request/very-dynamic/input/index.js:10:0 lint TP1002 require(FreeVar(Math)["random"]()) is very dynamic

6 | child_process.spawnSync(unknown);
7 | child_process.spawnSync("node", unknown);
8 | child_process.spawnSync("node", [unknown, unknown]);
9 |
+ v--------------v
10 + require(unknown);
+ ^--------------^
11 |
12 | import(unknown);
13 |
14 | fs.readFileSync(unknown);
12 changes: 12 additions & 0 deletions ...request/very-dynamic/issues/__l___lint TP1004__ fs.readFileSync(FreeVar(Math)[-45dc7a.txt
View file
Open in desktop
@@ -0,0 +1,12 @@
warning - [analysis] [project]/crates/turbopack-tests/tests/snapshot/dynamic-request/very-dynamic/input/index.js /crates/turbopack-tests/tests/snapshot/dynamic-request/very-dynamic/input/index.js:15:0 lint TP1004 fs.readFileSync(FreeVar(Math)["random"]()) is very dynamic

11 |
12 | import(unknown);
13 |
14 | fs.readFileSync(unknown);
+ v-------------------v
15 + readFileSync(unknown);
+ ^-------------------^
16 |
17 | new URL(unknown, import.meta.url);
18 |