From 8cdd5b0fd89a05d568d951853c13c791753eab2c Mon Sep 17 00:00:00 2001 From: Wang Wenzhe Date: Wed, 1 May 2024 19:46:40 +0800 Subject: [PATCH] feat(linter/tree_shaking): support LogicExpression and MemberExpression (#3148) MemberExpression's message is not accurate, I will update later. --- .../listener_map.rs | 202 +++++++++++++++++- .../no_side_effects_in_initialization/mod.rs | 96 +++++---- .../no_side_effects_in_initialization.snap | 120 ++++++++++- crates/oxc_linter/src/utils/tree_shaking.rs | 17 +- 4 files changed, 373 insertions(+), 62 deletions(-) diff --git a/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/listener_map.rs b/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/listener_map.rs index 8691328c2dc96..268393d74cca3 100644 --- a/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/listener_map.rs +++ b/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/listener_map.rs @@ -9,23 +9,24 @@ use oxc_ast::{ ConditionalExpression, Declaration, ExportSpecifier, Expression, ForStatementInit, FormalParameter, Function, IdentifierReference, JSXAttribute, JSXAttributeItem, JSXAttributeValue, JSXChild, JSXElement, JSXElementName, JSXExpression, - JSXExpressionContainer, JSXFragment, JSXIdentifier, JSXOpeningElement, MemberExpression, - ModuleExportName, NewExpression, ObjectExpression, ObjectPropertyKind, + JSXExpressionContainer, JSXFragment, JSXIdentifier, JSXOpeningElement, LogicalExpression, + MemberExpression, ModuleExportName, NewExpression, ObjectExpression, ObjectPropertyKind, ParenthesizedExpression, PrivateFieldExpression, Program, PropertyKey, - SimpleAssignmentTarget, Statement, StaticMemberExpression, ThisExpression, + SimpleAssignmentTarget, Statement, StaticMemberExpression, ThisExpression, UnaryExpression, VariableDeclarator, }, AstKind, }; use oxc_semantic::{AstNode, SymbolId}; use oxc_span::{GetSpan, Span}; +use oxc_syntax::operator::{LogicalOperator, UnaryOperator}; use rustc_hash::FxHashSet; use crate::{ ast_util::{get_declaration_of_variable, get_symbol_id_of_variable}, utils::{ - calculate_binary_operation, get_write_expr, has_comment_about_side_effect_check, - has_pure_notation, no_effects, Value, + calculate_binary_operation, calculate_logical_operation, get_write_expr, + has_comment_about_side_effect_check, has_pure_notation, no_effects, Value, }, LintContext, }; @@ -177,6 +178,9 @@ impl<'a> ListenerMap for Statement<'a> { stmt.right.report_effects(options); stmt.body.report_effects(options); } + Self::LabeledStatement(stmt) => { + stmt.body.report_effects(options); + } _ => {} } } @@ -265,6 +269,12 @@ impl<'a> ListenerMap for AstNode<'a> { options.ctx.diagnostic(NoSideEffectsDiagnostic::CallImport(span)); } } + AstKind::ImportNamespaceSpecifier(specifier) => { + let span = specifier.local.span; + if !has_comment_about_side_effect_check(span, options.ctx) { + options.ctx.diagnostic(NoSideEffectsDiagnostic::CallImport(span)); + } + } _ => {} } } @@ -503,6 +513,21 @@ impl<'a> ListenerMap for Expression<'a> { Self::ObjectExpression(expr) => { expr.report_effects(options); } + Self::LogicalExpression(expr) => { + expr.get_value_and_report_effects(options); + } + Self::StaticMemberExpression(expr) => { + expr.report_effects(options); + } + Self::ComputedMemberExpression(expr) => { + expr.report_effects(options); + } + Self::PrivateFieldExpression(expr) => { + expr.report_effects(options); + } + Self::UnaryExpression(expr) => { + expr.get_value_and_report_effects(options); + } Self::ArrowFunctionExpression(_) | Self::FunctionExpression(_) | Self::Identifier(_) @@ -558,6 +583,15 @@ impl<'a> ListenerMap for Expression<'a> { expr.report_effects_when_called(options); } Self::ConditionalExpression(expr) => expr.report_effects_when_called(options), + Self::StaticMemberExpression(expr) => { + expr.report_effects_when_called(options); + } + Self::ComputedMemberExpression(expr) => { + expr.report_effects_when_called(options); + } + Self::PrivateFieldExpression(expr) => { + expr.report_effects_when_called(options); + } _ => { // Default behavior options.ctx.diagnostic(NoSideEffectsDiagnostic::Call(self.span())); @@ -572,6 +606,7 @@ impl<'a> ListenerMap for Expression<'a> { | Self::TemplateLiteral(_) => Value::new(self), Self::BinaryExpression(expr) => expr.get_value_and_report_effects(options), Self::ConditionalExpression(expr) => expr.get_value_and_report_effects(options), + Self::LogicalExpression(expr) => expr.get_value_and_report_effects(options), _ => { self.report_effects(options); Value::Unknown @@ -596,6 +631,48 @@ fn defined_custom_report_effects_when_called(expr: &Expression) -> bool { ) } +impl<'a> ListenerMap for UnaryExpression<'a> { + fn get_value_and_report_effects(&self, options: &NodeListenerOptions) -> Value { + if self.operator == UnaryOperator::Delete { + match &self.argument { + Expression::StaticMemberExpression(expr) => { + expr.object.report_effects_when_mutated(options); + } + Expression::ComputedMemberExpression(expr) => { + expr.object.report_effects_when_mutated(options); + } + Expression::PrivateFieldExpression(expr) => { + expr.object.report_effects_when_mutated(options); + } + _ => {} + } + } + + // TODO + Value::Unknown + } +} + +impl<'a> ListenerMap for LogicalExpression<'a> { + fn get_value_and_report_effects(&self, options: &NodeListenerOptions) -> Value { + let left = self.left.get_value_and_report_effects(options); + // `false && foo` + if self.operator == LogicalOperator::And + && left.get_falsy_value().is_some_and(|is_falsy| is_falsy) + { + return left; + } + // `true || foo` + if self.operator == LogicalOperator::Or + && left.get_falsy_value().is_some_and(|is_falsy| !is_falsy) + { + return left; + } + let right = self.right.get_value_and_report_effects(options); + calculate_logical_operation(self.operator, left, right) + } +} + impl<'a> ListenerMap for ObjectExpression<'a> { fn report_effects(&self, options: &NodeListenerOptions) { self.properties.iter().for_each(|property| match property { @@ -769,6 +846,18 @@ impl<'a> ListenerMap for JSXExpression<'a> { Self::ObjectExpression(expr) => { expr.report_effects(options); } + Self::StaticMemberExpression(expr) => { + expr.report_effects(options); + } + Self::ComputedMemberExpression(expr) => { + expr.report_effects(options); + } + Self::PrivateFieldExpression(expr) => { + expr.report_effects(options); + } + Self::UnaryExpression(expr) => { + expr.get_value_and_report_effects(options); + } Self::ArrowFunctionExpression(_) | Self::EmptyExpression(_) | Self::FunctionExpression(_) @@ -1046,16 +1135,26 @@ impl<'a> ListenerMap for MemberExpression<'a> { fn report_effects_when_assigned(&self, options: &NodeListenerOptions) { match self { Self::ComputedMemberExpression(expr) => { - expr.report_effects(options); - expr.object.report_effects_when_mutated(options); + expr.report_effects_when_assigned(options); } Self::StaticMemberExpression(expr) => { - expr.report_effects(options); - expr.object.report_effects_when_mutated(options); + expr.report_effects_when_assigned(options); } Self::PrivateFieldExpression(expr) => { - expr.report_effects(options); - expr.object.report_effects_when_mutated(options); + expr.report_effects_when_assigned(options); + } + } + } + fn report_effects_when_called(&self, options: &NodeListenerOptions) { + match self { + Self::ComputedMemberExpression(expr) => { + expr.report_effects_when_called(options); + } + Self::StaticMemberExpression(expr) => { + expr.report_effects_when_called(options); + } + Self::PrivateFieldExpression(expr) => { + expr.report_effects_when_called(options); } } } @@ -1066,18 +1165,99 @@ impl<'a> ListenerMap for ComputedMemberExpression<'a> { self.expression.report_effects(options); self.object.report_effects(options); } + fn report_effects_when_called(&self, options: &NodeListenerOptions) { + self.report_effects(options); + + let mut node = &self.object; + loop { + match node { + Expression::ComputedMemberExpression(expr) => { + node = &expr.object; + } + Expression::StaticMemberExpression(expr) => node = &expr.object, + Expression::PrivateInExpression(expr) => node = &expr.right, + _ => { + break; + } + } + } + + if let Expression::Identifier(ident) = node { + ident.report_effects_when_called(options); + } else { + options.ctx.diagnostic(NoSideEffectsDiagnostic::CallMember(node.span())); + } + } + fn report_effects_when_assigned(&self, options: &NodeListenerOptions) { + self.report_effects(options); + self.object.report_effects_when_mutated(options); + } } impl<'a> ListenerMap for StaticMemberExpression<'a> { fn report_effects(&self, options: &NodeListenerOptions) { self.object.report_effects(options); } + fn report_effects_when_called(&self, options: &NodeListenerOptions) { + self.report_effects(options); + + let mut node = &self.object; + loop { + match node { + Expression::ComputedMemberExpression(expr) => { + node = &expr.object; + } + Expression::StaticMemberExpression(expr) => node = &expr.object, + Expression::PrivateInExpression(expr) => node = &expr.right, + _ => { + break; + } + } + } + + if let Expression::Identifier(ident) = node { + ident.report_effects_when_called(options); + } else { + options.ctx.diagnostic(NoSideEffectsDiagnostic::CallMember(node.span())); + } + } + fn report_effects_when_assigned(&self, options: &NodeListenerOptions) { + self.report_effects(options); + self.object.report_effects_when_mutated(options); + } } impl<'a> ListenerMap for PrivateFieldExpression<'a> { fn report_effects(&self, options: &NodeListenerOptions) { self.object.report_effects(options); } + fn report_effects_when_called(&self, options: &NodeListenerOptions) { + self.report_effects(options); + + let mut node = &self.object; + loop { + match node { + Expression::ComputedMemberExpression(expr) => { + node = &expr.object; + } + Expression::StaticMemberExpression(expr) => node = &expr.object, + Expression::PrivateInExpression(expr) => node = &expr.right, + _ => { + break; + } + } + } + + if let Expression::Identifier(ident) = node { + ident.report_effects_when_called(options); + } else { + options.ctx.diagnostic(NoSideEffectsDiagnostic::CallMember(node.span())); + } + } + fn report_effects_when_assigned(&self, options: &NodeListenerOptions) { + self.report_effects(options); + self.object.report_effects_when_mutated(options); + } } impl<'a> ListenerMap for ArrayExpressionElement<'a> { diff --git a/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/mod.rs b/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/mod.rs index cdc5e60da40b3..ee918b2779471 100644 --- a/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/mod.rs +++ b/crates/oxc_linter/src/rules/tree_shaking/no_side_effects_in_initialization/mod.rs @@ -62,6 +62,10 @@ enum NoSideEffectsDiagnostic { #[diagnostic(severity(warning))] CallImport(#[label] Span), + #[error("eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling member function")] + #[diagnostic(severity(warning))] + CallMember(#[label] Span), + #[error("eslint-plugin-tree-shaking(no-side-effects-in-initialization): Debugger statements are side-effects")] #[diagnostic(severity(warning))] Debugger(#[label] Span), @@ -280,29 +284,29 @@ fn test() { r#"class X {}; const x = "#, // JSXSpreadAttribute "class X {}; const x = ", - // // LabeledStatement - // "loop: for(;true;){continue loop}", - // // Literal - // "const x = 3", - // "if (false) ext()", - // r#""use strict""#, - // // LogicalExpression - // "const x = 3 || 4", - // "true || ext()", - // "false && ext()", - // "if (false && false) ext()", - // "if (true && false) ext()", - // "if (false && true) ext()", - // "if (false || false) ext()", - // // MemberExpression - // "const x = ext.y", - // r#"const x = ext["y"]"#, - // "let x = ()=>{}; x.y = 1", - // // MemberExpression when called + // LabeledStatement + "loop: for(;true;){continue loop}", + // Literal + "const x = 3", + "if (false) ext()", + r#""use strict""#, + // LogicalExpression + "const x = 3 || 4", + "true || ext()", + "false && ext()", + "if (false && false) ext()", + "if (true && false) ext()", + "if (false && true) ext()", + "if (false || false) ext()", + // MemberExpression + "const x = ext.y", + r#"const x = ext["y"]"#, + "let x = ()=>{}; x.y = 1", + // MemberExpression when called // "const x = Object.keys({})", - // // MemberExpression when mutated - // "const x = {};x.y = ext", - // "const x = {y: 1};delete x.y", + // MemberExpression when mutated + "const x = {};x.y = ext", + "const x = {y: 1};delete x.y", // // MetaProperty // "function x(){const y = new.target}; x()", // // MethodDefinition @@ -568,30 +572,30 @@ fn test() { "class X {}; const x = ", // JSXSpreadAttribute "class X {}; const x = ", - // // LabeledStatement - // "loop: for(;true;){ext()}", - // // Literal - // "if (true) ext()", - // // LogicalExpression - // "ext() && true", - // "true && ext()", - // "false || ext()", - // "if (true && true) ext()", - // "if (false || true) ext()", - // "if (true || false) ext()", - // "if (true || true) ext()", - // // MemberExpression - // "const x = {};const y = x[ext()]", - // // MemberExpression when called - // "ext.x()", - // "const x = {}; x.y()", - // "const x = ()=>{}; x().y()", - // "const Object = {}; const x = Object.keys({})", - // "const x = {}; x[ext()]()", - // // MemberExpression when mutated - // "const x = {y: ext};x.y.z = 1", - // "const x = {y:ext};const y = x.y; y.z = 1", - // "const x = {y: ext};delete x.y.z", + // LabeledStatement + "loop: for(;true;){ext()}", + // Literal + "if (true) ext()", + // LogicalExpression + "ext() && true", + "true && ext()", + "false || ext()", + "if (true && true) ext()", + "if (false || true) ext()", + "if (true || false) ext()", + "if (true || true) ext()", + // MemberExpression + "const x = {};const y = x[ext()]", + // MemberExpression when called + "ext.x()", + "const x = {}; x.y()", + "const x = ()=>{}; x().y()", + "const Object = {}; const x = Object.keys({})", + "const x = {}; x[ext()]()", + // MemberExpression when mutated + "const x = {y: ext};x.y.z = 1", + "const x = {y:ext};const y = x.y; y.z = 1", + "const x = {y: ext};delete x.y.z", // // MethodDefinition // "class x {static [ext()](){}}", // NewExpression diff --git a/crates/oxc_linter/src/snapshots/no_side_effects_in_initialization.snap b/crates/oxc_linter/src/snapshots/no_side_effects_in_initialization.snap index 0af1cf5234f17..dd8aec65114b8 100644 --- a/crates/oxc_linter/src/snapshots/no_side_effects_in_initialization.snap +++ b/crates/oxc_linter/src/snapshots/no_side_effects_in_initialization.snap @@ -800,10 +800,10 @@ expression: no_side_effects_in_initialization · ─ ╰──── - ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling - ╭─[no_side_effects_in_initialization.tsx:1:30] + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling imported function + ╭─[no_side_effects_in_initialization.tsx:1:13] 1 │ import * as y from "import"; y.x() - · ─── + · ─ ╰──── ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of mutating imported variable @@ -878,6 +878,120 @@ expression: no_side_effects_in_initialization · ─── ╰──── + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:19] + 1 │ loop: for(;true;){ext()} + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:11] + 1 │ if (true) ext() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:1] + 1 │ ext() && true + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:9] + 1 │ true && ext() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:10] + 1 │ false || ext() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:19] + 1 │ if (true && true) ext() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:20] + 1 │ if (false || true) ext() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:20] + 1 │ if (true || false) ext() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:19] + 1 │ if (true || true) ext() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:26] + 1 │ const x = {};const y = x[ext()] + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:1] + 1 │ ext.x() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling + ╭─[no_side_effects_in_initialization.tsx:1:11] + 1 │ const x = {}; x.y() + · ── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling member function + ╭─[no_side_effects_in_initialization.tsx:1:19] + 1 │ const x = ()=>{}; x().y() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling + ╭─[no_side_effects_in_initialization.tsx:1:16] + 1 │ const Object = {}; const x = Object.keys({}) + · ── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` + ╭─[no_side_effects_in_initialization.tsx:1:17] + 1 │ const x = {}; x[ext()]() + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling + ╭─[no_side_effects_in_initialization.tsx:1:11] + 1 │ const x = {}; x[ext()]() + · ── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of mutating + ╭─[no_side_effects_in_initialization.tsx:1:20] + 1 │ const x = {y: ext};x.y.z = 1 + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of mutating + ╭─[no_side_effects_in_initialization.tsx:1:29] + 1 │ const x = {y:ext};const y = x.y; y.z = 1 + · ─── + ╰──── + + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of mutating + ╭─[no_side_effects_in_initialization.tsx:1:27] + 1 │ const x = {y: ext};delete x.y.z + · ─── + ╰──── + ⚠ eslint-plugin-tree-shaking(no-side-effects-in-initialization): Cannot determine side-effects of calling global function `ext` ╭─[no_side_effects_in_initialization.tsx:1:15] 1 │ const x = new ext() diff --git a/crates/oxc_linter/src/utils/tree_shaking.rs b/crates/oxc_linter/src/utils/tree_shaking.rs index 8ebd4b3473caa..16dd0c972e4ba 100644 --- a/crates/oxc_linter/src/utils/tree_shaking.rs +++ b/crates/oxc_linter/src/utils/tree_shaking.rs @@ -1,7 +1,7 @@ use oxc_ast::{ast::Expression, AstKind, CommentKind}; use oxc_semantic::AstNodeId; use oxc_span::Span; -use oxc_syntax::operator::BinaryOperator; +use oxc_syntax::operator::{BinaryOperator, LogicalOperator}; use crate::LintContext; @@ -192,7 +192,6 @@ pub fn calculate_binary_operation(op: BinaryOperator, left: Value, right: Value) _ => Value::Unknown, }, // - #[allow(clippy::single_match)] BinaryOperator::LessThan => match (left, right) { // (Value::Unknown, Value::Number(_)) | (Value::Number(_), Value::Unknown) => { @@ -205,6 +204,20 @@ pub fn calculate_binary_operation(op: BinaryOperator, left: Value, right: Value) } } +pub fn calculate_logical_operation(op: LogicalOperator, left: Value, right: Value) -> Value { + match op { + LogicalOperator::And => match (left, right) { + (Value::Boolean(a), Value::Boolean(b)) => Value::Boolean(a && b), + _ => Value::Unknown, + }, + LogicalOperator::Or => match (left, right) { + (Value::Boolean(a), Value::Boolean(b)) => Value::Boolean(a || b), + _ => Value::Unknown, + }, + LogicalOperator::Coalesce => Value::Unknown, + } +} + #[test] fn test_calculate_binary_operation() { use oxc_syntax::operator::BinaryOperator;