refactor(linter): LintContext can now only be constructed with a cf…

…g enabled semantic. (#3761) It has the same spirit as #3747 but with a much simpler approach. I've used the fact that @Boshen mentioned about linter always using CFG so now we assert `semantic.cfg().is_some()` in the `LintContext::new` because of this assertion we can have a `LintContext::cfg` that unwraps unchecked. Eliminates unnecessary checks in our hot paths. It has the best of both worlds, No complicated typing yet we still get the CFG as a non-optional value without extra ASM or branching.
oxc-project · Jun 19, 2024 · 4d2b7f1 · 4d2b7f1
1 parent 4fb90eb
commit 4d2b7f1
Show file tree

Hide file tree

Showing 11 changed files with 45 additions and 38 deletions.
diff --git a/crates/oxc_language_server/src/linter.rs b/crates/oxc_language_server/src/linter.rs
@@ -287,6 +287,7 @@ impl IsolatedLintHandler {
 
             let program = allocator.alloc(ret.program);
             let semantic_ret = SemanticBuilder::new(javascript_source_text, source_type)
+                .with_cfg(true)
                 .with_trivias(ret.trivias)
                 .with_check_syntax_error(true)
                 .build(program);

diff --git a/crates/oxc_linter/src/context.rs b/crates/oxc_linter/src/context.rs
@@ -1,5 +1,6 @@
 use std::{cell::RefCell, path::Path, rc::Rc, sync::Arc};
 
+use oxc_cfg::ControlFlowGraph;
 use oxc_diagnostics::{OxcDiagnostic, Severity};
 use oxc_semantic::{AstNodes, JSDocFinder, ScopeTree, Semantic, SymbolTable};
 use oxc_span::{SourceType, Span};
@@ -34,7 +35,15 @@ pub struct LintContext<'a> {
 }
 
 impl<'a> LintContext<'a> {
+    /// # Panics
+    /// If `semantic.cfg()` is `None`.
     pub fn new(file_path: Box<Path>, semantic: Rc<Semantic<'a>>) -> Self {
+        // We should always check for `semantic.cfg()` being `Some` since we depend on it and it is
+        // unwrapped without any runtime checks after construction.
+        assert!(
+            semantic.cfg().is_some(),
+            "`LintContext` depends on `Semantic::cfg`, Build your semantic with cfg enabled(`SemanticBuilder::with_cfg`)."
+        );
         let disable_directives =
             DisableDirectivesBuilder::new(semantic.source_text(), semantic.trivias().clone())
                 .build();
@@ -78,6 +87,15 @@ impl<'a> LintContext<'a> {
         &self.semantic
     }
 
+    pub fn cfg(&self) -> &ControlFlowGraph {
+        #[allow(unsafe_code)]
+        // SAFETY: `LintContext::new` is the only way to construct a `LintContext` and we always
+        // assert the existence of control flow so it should always be `Some`.
+        unsafe {
+            self.semantic().cfg().unwrap_unchecked()
+        }
+    }
+
     pub fn disable_directives(&self) -> &DisableDirectives<'a> {
         &self.disable_directives
     }

diff --git a/crates/oxc_linter/src/rules/eslint/getter_return.rs b/crates/oxc_linter/src/rules/eslint/getter_return.rs
@@ -6,7 +6,7 @@ use oxc_ast::{
     AstKind,
 };
 use oxc_cfg::{
-    graph::visit::neighbors_filtered_by_edge_weight, ControlFlowGraph, EdgeType, InstructionKind,
+    graph::visit::neighbors_filtered_by_edge_weight, EdgeType, InstructionKind,
     ReturnInstructionKind,
 };
 use oxc_diagnostics::OxcDiagnostic;
@@ -54,19 +54,16 @@ declare_oxc_lint!(
 
 impl Rule for GetterReturn {
     fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
-        // control flow dependant
-        let Some(cfg) = ctx.semantic().cfg() else { return };
-
         // https://eslint.org/docs/latest/rules/getter-return#handled_by_typescript
         if ctx.source_type().is_typescript() {
             return;
         }
         match node.kind() {
             AstKind::Function(func) if !func.is_typescript_syntax() => {
-                self.run_diagnostic(node, ctx, cfg, func.span);
+                self.run_diagnostic(node, ctx, func.span);
             }
             AstKind::ArrowFunctionExpression(expr) => {
-                self.run_diagnostic(node, ctx, cfg, expr.span);
+                self.run_diagnostic(node, ctx, expr.span);
             }
             _ => {}
         }
@@ -180,17 +177,13 @@ impl GetterReturn {
         false
     }
 
-    fn run_diagnostic<'a>(
-        &self,
-        node: &AstNode<'a>,
-        ctx: &LintContext<'a>,
-        cfg: &ControlFlowGraph,
-        span: Span,
-    ) {
+    fn run_diagnostic<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>, span: Span) {
         if !Self::is_wanted_node(node, ctx) {
             return;
         }
 
+        let cfg = ctx.cfg();
+
         let output = neighbors_filtered_by_edge_weight(
             cfg.graph(),
             node.cfg_id(),

diff --git a/crates/oxc_linter/src/rules/eslint/no_fallthrough.rs b/crates/oxc_linter/src/rules/eslint/no_fallthrough.rs
@@ -10,7 +10,7 @@ use oxc_cfg::{
         visit::{neighbors_filtered_by_edge_weight, EdgeRef},
         Direction,
     },
-    BasicBlockId, ControlFlowGraph, EdgeType, ErrorEdgeKind, InstructionKind,
+    BasicBlockId, EdgeType, ErrorEdgeKind, InstructionKind,
 };
 use oxc_diagnostics::OxcDiagnostic;
 use oxc_macros::declare_oxc_lint;
@@ -89,15 +89,13 @@ impl Rule for NoFallthrough {
     }
 
     fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
-        // control flow dependant
-        let Some(cfg) = ctx.semantic().cfg() else { return };
-
         let AstKind::SwitchStatement(switch) = node.kind() else { return };
 
+        let cfg = ctx.cfg();
         let switch_id = node.cfg_id();
         let graph = cfg.graph();
 
-        let (cfg_ids, tests, default, exit) = get_switch_semantic_cases(ctx, cfg, node, switch);
+        let (cfg_ids, tests, default, exit) = get_switch_semantic_cases(ctx, node, switch);
 
         let Some(default_or_exit) = default.or(exit) else {
             // TODO: our `get_switch_semantic_cases` can't evaluate cfg_ids for switch statements
@@ -262,7 +260,6 @@ impl NoFallthrough {
 // Issue: <https://github.com/oxc-project/oxc/issues/3662>
 fn get_switch_semantic_cases(
     ctx: &LintContext,
-    cfg: &ControlFlowGraph,
     node: &AstNode,
     switch: &SwitchStatement,
 ) -> (
@@ -271,6 +268,7 @@ fn get_switch_semantic_cases(
     /* default */ Option<BasicBlockId>,
     /* exit */ Option<BasicBlockId>,
 ) {
+    let cfg = ctx.cfg();
     let graph = cfg.graph();
     let has_default = switch.cases.iter().any(SwitchCase::is_default_case);
     let (tests, exit) = graph

diff --git a/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs b/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs
@@ -56,9 +56,8 @@ enum DefinitelyCallsThisBeforeSuper {
 
 impl Rule for NoThisBeforeSuper {
     fn run_once(&self, ctx: &LintContext) {
+        let cfg = ctx.cfg();
         let semantic = ctx.semantic();
-        // control flow dependant
-        let Some(cfg) = ctx.semantic().cfg() else { return };
 
         // first pass -> find super calls and local violations
         let mut wanted_nodes = Vec::new();

diff --git a/crates/oxc_linter/src/rules/eslint/no_unreachable.rs b/crates/oxc_linter/src/rules/eslint/no_unreachable.rs
@@ -31,11 +31,9 @@ declare_oxc_lint!(
 
 impl Rule for NoUnreachable {
     fn run_once(&self, ctx: &LintContext) {
-        // control flow dependant
-        let Some(cfg) = ctx.semantic().cfg() else { return };
-
         let nodes = ctx.nodes();
         let Some(root) = nodes.root_node() else { return };
+        let cfg = ctx.cfg();
         let graph = cfg.graph();
 
         // A pre-allocated vector containing the reachability status of all the basic blocks.

diff --git a/crates/oxc_linter/src/rules/react/require_render_return.rs b/crates/oxc_linter/src/rules/react/require_render_return.rs
@@ -1,7 +1,7 @@
 use oxc_ast::{ast::Expression, AstKind};
 use oxc_cfg::{
-    graph::visit::neighbors_filtered_by_edge_weight, ControlFlowGraph, EdgeType, Instruction,
-    InstructionKind, ReturnInstructionKind,
+    graph::visit::neighbors_filtered_by_edge_weight, EdgeType, Instruction, InstructionKind,
+    ReturnInstructionKind,
 };
 use oxc_diagnostics::OxcDiagnostic;
 use oxc_macros::declare_oxc_lint;
@@ -50,9 +50,6 @@ declare_oxc_lint!(
 
 impl Rule for RequireRenderReturn {
     fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
-        // control flow dependant
-        let Some(cfg) = ctx.semantic().cfg() else { return };
-
         if !matches!(node.kind(), AstKind::ArrowFunctionExpression(_) | AstKind::Function(_)) {
             return;
         }
@@ -66,7 +63,7 @@ impl Rule for RequireRenderReturn {
             return;
         }
 
-        if !contains_return_statement(node, cfg) {
+        if !contains_return_statement(node, ctx) {
             match parent.kind() {
                 AstKind::MethodDefinition(method) => {
                     ctx.diagnostic(require_render_return_diagnostic(method.key.span()));
@@ -93,7 +90,8 @@ enum FoundReturn {
 const KEEP_WALKING_ON_THIS_PATH: bool = true;
 const STOP_WALKING_ON_THIS_PATH: bool = false;
 
-fn contains_return_statement(node: &AstNode, cfg: &ControlFlowGraph) -> bool {
+fn contains_return_statement(node: &AstNode, ctx: &LintContext) -> bool {
+    let cfg = ctx.cfg();
     let state = neighbors_filtered_by_edge_weight(
         cfg.graph(),
         node.cfg_id(),

diff --git a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs
@@ -107,14 +107,14 @@ declare_oxc_lint!(
 
 impl Rule for RulesOfHooks {
     fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
-        // control flow dependant
-        let Some(cfg) = ctx.semantic().cfg() else { return };
-
         let AstKind::CallExpression(call) = node.kind() else { return };
 
         if !is_react_hook(&call.callee) {
             return;
         }
+
+        let cfg = ctx.cfg();
+
         let span = call.span;
         let hook_name =
             call.callee_name().expect("We identify hooks using their names so it should be named.");

diff --git a/crates/oxc_linter/src/utils/jest.rs b/crates/oxc_linter/src/utils/jest.rs
@@ -315,7 +315,8 @@ mod test {
         let source_type = SourceType::default();
         let parser_ret = Parser::new(&allocator, "", source_type).parse();
         let program = allocator.alloc(parser_ret.program);
-        let semantic_ret = SemanticBuilder::new("", source_type).build(program).semantic;
+        let semantic_ret =
+            SemanticBuilder::new("", source_type).with_cfg(true).build(program).semantic;
         let semantic_ret = Rc::new(semantic_ret);
 
         let path = Path::new("foo.js");

diff --git a/crates/oxc_macros/src/declare_all_lint_rules.rs b/crates/oxc_macros/src/declare_all_lint_rules.rs
@@ -101,19 +101,19 @@ pub fn declare_all_lint_rules(metadata: AllLintRulesMeta) -> TokenStream {
                 }
             }
 
-            pub fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
+            pub(super) fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
                 match self {
                     #(Self::#struct_names(rule) => rule.run(node, ctx)),*
                 }
             }
 
-            pub fn run_on_symbol<'a>(&self, symbol_id: SymbolId, ctx: &LintContext<'a>) {
+            pub(super) fn run_on_symbol<'a>(&self, symbol_id: SymbolId, ctx: &LintContext<'a>) {
                 match self {
                     #(Self::#struct_names(rule) => rule.run_on_symbol(symbol_id, ctx)),*
                 }
             }
 
-            pub fn run_once<'a>(&self, ctx: &LintContext<'a>) {
+            pub(super) fn run_once<'a>(&self, ctx: &LintContext<'a>) {
                 match self {
                     #(Self::#struct_names(rule) => rule.run_once(ctx)),*
                 }

diff --git a/crates/oxc_wasm/src/lib.rs b/crates/oxc_wasm/src/lib.rs
@@ -180,6 +180,7 @@ impl Oxc {
         let program = allocator.alloc(ret.program);
 
         let semantic_ret = SemanticBuilder::new(source_text, source_type)
+            .with_cfg(true)
             .with_trivias(ret.trivias.clone())
             .with_check_syntax_error(true)
             .build(program);