diff --git a/CHANGELOG.md b/CHANGELOG.md index 789bed7d10..af232c0474 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,12 +20,16 @@ #### :bug: Bug fix +- Reanalyze: make optional args analysis liveness-aware, preventing false positives when functions are only called from dead code. https://github.com/rescript-lang/rescript/pull/8082 + #### :memo: Documentation #### :nail_care: Polish #### :house: Internal +- Reanalyze: refactor DCE to pure pipeline architecture for order-independence and incremental update support. https://github.com/rescript-lang/rescript/pull/8043 + # 12.0.1 #### :bug: Bug fix diff --git a/analysis/reanalyze/ARCHITECTURE.md b/analysis/reanalyze/ARCHITECTURE.md index 23e6e5c662..1d341ae52e 100644 --- a/analysis/reanalyze/ARCHITECTURE.md +++ b/analysis/reanalyze/ARCHITECTURE.md @@ -70,14 +70,17 @@ This design enables: ╠══════════════════════════════════════════════════╪══════════════════════╣ ║ ▼ ║ ║ ┌─────────────────────────────────────────────────────────────────┐ ║ - ║ │ DeadCommon.solveDead │ ║ - ║ │ ~annotations ~decls ~refs ~file_deps │ ║ - ║ │ ~optional_args_state ~config ~checkOptionalArg │ ║ + ║ │ Pass 1: DeadCommon.solveDead (core deadness) │ ║ + ║ │ ~annotations ~decls ~refs ~file_deps ~config │ ║ + ║ │ → AnalysisResult.t (dead/live status resolved) │ ║ ║ │ │ ║ + ║ │ Pass 2: Optional args analysis (liveness-aware) │ ║ + ║ │ CrossFileItems.compute_optional_args_state ~is_live │ ║ + ║ │ DeadOptionalArgs.check (only for live decls) │ ║ ║ │ → AnalysisResult.t { issues: Issue.t list } │ ║ ║ └─────────────────────────────────────────────────────────────────┘ ║ ║ │ ║ - ║ Pure function: immutable in → immutable out │ issues ║ + ║ Pure functions: immutable in → immutable out │ issues ║ ╚══════════════════════════════════════════════════╪══════════════════════╝ │ ╔══════════════════════════════════════════════════╪══════════════════════╗ @@ -149,20 +152,30 @@ let file_deps = FileDeps.merge_all (file_data_list |> List.map (fun fd -> fd.fil ### Phase 3: SOLVE (Deadness Computation) -**Entry point**: `DeadCommon.solveDead` +**Entry point**: `DeadCommon.solveDead` + optional args second pass in `Reanalyze.runAnalysis` **Input**: All merged data + config **Output**: `AnalysisResult.t` containing `Issue.t list` -**Algorithm**: +**Algorithm** (two-pass for liveness-aware optional args): + +**Pass 1: Core deadness resolution** 1. Build file dependency order (roots to leaves) 2. Sort declarations by dependency order 3. For each declaration, resolve references recursively 4. Determine dead/live status based on reference count 5. Collect issues for dead declarations -**Key property**: Pure function - immutable in, immutable out. No side effects. +**Pass 2: Liveness-aware optional args analysis** +1. Use `Decl.isLive` to build an `is_live` predicate from Pass 1 results +2. Compute optional args state via `CrossFileItems.compute_optional_args_state`, filtering out calls from dead code +3. Collect optional args issues only for live declarations +4. Merge optional args issues into the final result + +This two-pass approach ensures that optional argument warnings (e.g., "argument X is never used") only consider calls from live code, preventing false positives when a function is only called from dead code. + +**Key property**: Pure functions - immutable in, immutable out. No side effects. ### Phase 4: REPORT (Output) diff --git a/analysis/reanalyze/src/CrossFileItems.ml b/analysis/reanalyze/src/CrossFileItems.ml index c7c5f5504a..cf038fdb8f 100644 --- a/analysis/reanalyze/src/CrossFileItems.ml +++ b/analysis/reanalyze/src/CrossFileItems.ml @@ -8,6 +8,7 @@ type exception_ref = {exception_path: DcePath.t; loc_from: Location.t} type optional_arg_call = { + pos_from: Lexing.position; pos_to: Lexing.position; arg_names: string list; arg_names_maybe: string list; @@ -37,9 +38,10 @@ let create_builder () : builder = let add_exception_ref (b : builder) ~exception_path ~loc_from = b.exception_refs <- {exception_path; loc_from} :: b.exception_refs -let add_optional_arg_call (b : builder) ~pos_to ~arg_names ~arg_names_maybe = +let add_optional_arg_call (b : builder) ~pos_from ~pos_to ~arg_names + ~arg_names_maybe = b.optional_arg_calls <- - {pos_to; arg_names; arg_names_maybe} :: b.optional_arg_calls + {pos_from; pos_to; arg_names; arg_names_maybe} :: b.optional_arg_calls let add_function_reference (b : builder) ~pos_from ~pos_to = b.function_refs <- {pos_from; pos_to} :: b.function_refs @@ -71,7 +73,7 @@ let process_exception_refs (t : t) ~refs ~file_deps ~find_exception ~config = (** Compute optional args state from calls and function references. Returns a map from position to final OptionalArgs.t state. Pure function - does not mutate declarations. *) -let compute_optional_args_state (t : t) ~decls : OptionalArgsState.t = +let compute_optional_args_state (t : t) ~decls ~is_live : OptionalArgsState.t = let state = OptionalArgsState.create () in (* Initialize state from declarations *) let get_state pos = @@ -85,22 +87,24 @@ let compute_optional_args_state (t : t) ~decls : OptionalArgsState.t = let set_state pos s = OptionalArgsState.set state pos s in (* Process optional arg calls *) t.optional_arg_calls - |> List.iter (fun {pos_to; arg_names; arg_names_maybe} -> - let current = get_state pos_to in - let updated = - OptionalArgs.apply_call ~argNames:arg_names - ~argNamesMaybe:arg_names_maybe current - in - set_state pos_to updated); + |> List.iter (fun {pos_from; pos_to; arg_names; arg_names_maybe} -> + if is_live pos_from then + let current = get_state pos_to in + let updated = + OptionalArgs.apply_call ~argNames:arg_names + ~argNamesMaybe:arg_names_maybe current + in + set_state pos_to updated); (* Process function references *) t.function_refs |> List.iter (fun {pos_from; pos_to} -> - let state_from = get_state pos_from in - let state_to = get_state pos_to in - if not (OptionalArgs.isEmpty state_to) then ( - let updated_from, updated_to = - OptionalArgs.combine_pair state_from state_to - in - set_state pos_from updated_from; - set_state pos_to updated_to)); + if is_live pos_from then + let state_from = get_state pos_from in + let state_to = get_state pos_to in + if not (OptionalArgs.isEmpty state_to) then ( + let updated_from, updated_to = + OptionalArgs.combine_pair state_from state_to + in + set_state pos_from updated_from; + set_state pos_to updated_to)); state diff --git a/analysis/reanalyze/src/CrossFileItems.mli b/analysis/reanalyze/src/CrossFileItems.mli index 34620b6917..199089baaf 100644 --- a/analysis/reanalyze/src/CrossFileItems.mli +++ b/analysis/reanalyze/src/CrossFileItems.mli @@ -23,11 +23,12 @@ val add_exception_ref : val add_optional_arg_call : builder -> + pos_from:Lexing.position -> pos_to:Lexing.position -> arg_names:string list -> arg_names_maybe:string list -> unit -(** Add a cross-file optional argument call. *) +(** Add a cross-file optional argument call, recording caller and callee. *) val add_function_reference : builder -> pos_from:Lexing.position -> pos_to:Lexing.position -> unit @@ -52,6 +53,10 @@ val process_exception_refs : (** {2 Optional Args State} *) val compute_optional_args_state : - t -> decls:Declarations.t -> OptionalArgsState.t -(** Compute final optional args state from calls and function references. + t -> + decls:Declarations.t -> + is_live:(Lexing.position -> bool) -> + OptionalArgsState.t +(** Compute final optional args state from calls and function references, + taking into account caller liveness via the [is_live] predicate. Pure function - does not mutate declarations. *) diff --git a/analysis/reanalyze/src/DeadOptionalArgs.ml b/analysis/reanalyze/src/DeadOptionalArgs.ml index d5842e5eaa..c7fcc93b8e 100644 --- a/analysis/reanalyze/src/DeadOptionalArgs.ml +++ b/analysis/reanalyze/src/DeadOptionalArgs.ml @@ -40,20 +40,22 @@ let rec fromTypeExpr (texpr : Types.type_expr) = | _ -> [] let addReferences ~config ~cross_file ~(locFrom : Location.t) - ~(locTo : Location.t) ~path (argNames, argNamesMaybe) = + ~(locTo : Location.t) ~(binding : Location.t) ~path (argNames, argNamesMaybe) + = if active () then ( let posTo = locTo.loc_start in - let posFrom = locFrom.loc_start in - CrossFileItems.add_optional_arg_call cross_file ~pos_to:posTo - ~arg_names:argNames ~arg_names_maybe:argNamesMaybe; + let posFrom = binding.loc_start in + CrossFileItems.add_optional_arg_call cross_file ~pos_from:posFrom + ~pos_to:posTo ~arg_names:argNames ~arg_names_maybe:argNamesMaybe; if config.DceConfig.cli.debug then + let callPos = locFrom.loc_start in Log_.item "DeadOptionalArgs.addReferences %s called with optional argNames:%s \ argNamesMaybe:%s %s@." (path |> DcePath.fromPathT |> DcePath.toString) (argNames |> String.concat ", ") (argNamesMaybe |> String.concat ", ") - (posFrom |> Pos.toString)) + (callPos |> Pos.toString)) (** Check for optional args issues. Returns issues instead of logging. Uses optional_args_state map for final computed state. *) diff --git a/analysis/reanalyze/src/DeadValue.ml b/analysis/reanalyze/src/DeadValue.ml index 6197ce7417..c75a6d0ac8 100644 --- a/analysis/reanalyze/src/DeadValue.ml +++ b/analysis/reanalyze/src/DeadValue.ml @@ -78,7 +78,7 @@ let collectValueBinding ~config ~decls ~file ~(current_binding : Location.t) loc let processOptionalArgs ~config ~cross_file ~expType ~(locFrom : Location.t) - ~locTo ~path args = + ~(binding : Location.t) ~locTo ~path args = if expType |> DeadOptionalArgs.hasOptionalArgs then ( let supplied = ref [] in let suppliedMaybe = ref [] in @@ -107,7 +107,8 @@ let processOptionalArgs ~config ~cross_file ~expType ~(locFrom : Location.t) if argIsSupplied = None then suppliedMaybe := s :: !suppliedMaybe | _ -> ()); (!supplied, !suppliedMaybe) - |> DeadOptionalArgs.addReferences ~config ~cross_file ~locFrom ~locTo ~path) + |> DeadOptionalArgs.addReferences ~config ~cross_file ~locFrom ~locTo + ~binding ~path) let rec collectExpr ~config ~refs ~file_deps ~cross_file ~(last_binding : Location.t) super self (e : Typedtree.expression) = @@ -142,7 +143,7 @@ let rec collectExpr ~config ~refs ~file_deps ~cross_file args |> processOptionalArgs ~config ~cross_file ~expType:exp_type ~locFrom:(locFrom : Location.t) - ~locTo ~path + ~binding:last_binding ~locTo ~path | Texp_let ( (* generated for functions with optional args *) Nonrecursive, @@ -183,7 +184,7 @@ let rec collectExpr ~config ~refs ~file_deps ~cross_file args |> processOptionalArgs ~config ~cross_file ~expType:exp_type ~locFrom:(locFrom : Location.t) - ~locTo ~path + ~binding:last_binding ~locTo ~path | Texp_field (_, _, {lbl_loc = {Location.loc_start = posTo; loc_ghost = false}; _}) -> if !Config.analyzeTypes then diff --git a/analysis/reanalyze/src/Decl.ml b/analysis/reanalyze/src/Decl.ml index 0ce1d08af2..c596e9bbbc 100644 --- a/analysis/reanalyze/src/Decl.ml +++ b/analysis/reanalyze/src/Decl.ml @@ -43,6 +43,12 @@ let isValue decl = | Value _ (* | Exception *) -> true | _ -> false +(** Check if a declaration is live (or unknown). Returns false only if resolved as dead. *) +let isLive decl = + match decl.resolvedDead with + | Some true -> false + | Some false | None -> true + let compareUsingDependencies ~orderedFiles { declKind = kind1; diff --git a/analysis/reanalyze/src/Reanalyze.ml b/analysis/reanalyze/src/Reanalyze.ml index 4b8619e751..cb5d70b121 100644 --- a/analysis/reanalyze/src/Reanalyze.ml +++ b/analysis/reanalyze/src/Reanalyze.ml @@ -168,18 +168,43 @@ let runAnalysis ~dce_config ~cmtRoot = CrossFileItems.process_exception_refs cross_file ~refs:refs_builder ~file_deps:file_deps_builder ~find_exception:DeadException.find_exception ~config:dce_config; - (* Compute optional args state (pure - no mutation) *) - let optional_args_state = - CrossFileItems.compute_optional_args_state cross_file ~decls - in (* Now freeze refs and file_deps for solver *) let refs = References.freeze_builder refs_builder in let file_deps = FileDeps.freeze_builder file_deps_builder in - (* Run the solver - returns immutable AnalysisResult.t *) - let analysis_result = + (* Run the solver - returns immutable AnalysisResult.t. + Optional-args checks are done in a separate pass after liveness is known. *) + let empty_optional_args_state = OptionalArgsState.create () in + let analysis_result_core = DeadCommon.solveDead ~annotations ~decls ~refs ~file_deps - ~optional_args_state ~config:dce_config - ~checkOptionalArg:DeadOptionalArgs.check + ~optional_args_state:empty_optional_args_state ~config:dce_config + ~checkOptionalArg:(fun + ~optional_args_state:_ ~annotations:_ ~config:_ _ -> []) + in + (* Compute liveness-aware optional args state *) + let is_live pos = + match Declarations.find_opt decls pos with + | Some decl -> Decl.isLive decl + | None -> true + in + let optional_args_state = + CrossFileItems.compute_optional_args_state cross_file ~decls ~is_live + in + (* Collect optional args issues only for live declarations *) + let optional_args_issues = + Declarations.fold + (fun _pos decl acc -> + if Decl.isLive decl then + let issues = + DeadOptionalArgs.check ~optional_args_state ~annotations + ~config:dce_config decl + in + List.rev_append issues acc + else acc) + decls [] + |> List.rev + in + let analysis_result = + AnalysisResult.add_issues analysis_result_core optional_args_issues in (* Report all issues *) AnalysisResult.get_issues analysis_result diff --git a/tests/analysis_tests/tests-reanalyze/deadcode/expected/deadcode.txt b/tests/analysis_tests/tests-reanalyze/deadcode/expected/deadcode.txt index 51493cee84..83ad64595e 100644 --- a/tests/analysis_tests/tests-reanalyze/deadcode/expected/deadcode.txt +++ b/tests/analysis_tests/tests-reanalyze/deadcode/expected/deadcode.txt @@ -1212,6 +1212,16 @@ Scanning OptArg.cmti Source:OptArg.resi addValueDeclaration +foo OptArg.resi:1:0 path:OptArg addValueDeclaration +bar OptArg.resi:2:0 path:OptArg + Scanning OptionalArgsLiveDead.cmt Source:OptionalArgsLiveDead.res + addValueDeclaration +formatDate OptionalArgsLiveDead.res:1:4 path:+OptionalArgsLiveDead + addValueDeclaration +deadCaller OptionalArgsLiveDead.res:3:4 path:+OptionalArgsLiveDead + addValueDeclaration +liveCaller OptionalArgsLiveDead.res:5:4 path:+OptionalArgsLiveDead + addValueReference OptionalArgsLiveDead.res:1:4 --> OptionalArgsLiveDead.res:1:26 + DeadOptionalArgs.addReferences formatDate called with optional argNames:fmt argNamesMaybe: OptionalArgsLiveDead.res:3:23 + addValueReference OptionalArgsLiveDead.res:3:4 --> OptionalArgsLiveDead.res:1:4 + DeadOptionalArgs.addReferences formatDate called with optional argNames: argNamesMaybe: OptionalArgsLiveDead.res:5:23 + addValueReference OptionalArgsLiveDead.res:5:4 --> OptionalArgsLiveDead.res:1:4 + addValueReference OptionalArgsLiveDead.res:7:8 --> OptionalArgsLiveDead.res:5:4 Scanning Records.cmt Source:Records.res addValueDeclaration +origin Records.res:11:4 path:+Records addValueDeclaration +computeArea Records.res:14:4 path:+Records @@ -1807,6 +1817,7 @@ File References Opaque.res -->> OptArg.res -->> OptArg.resi -->> OptArg.res + OptionalArgsLiveDead.res -->> Records.res -->> References.res -->> RepeatedLabel.res -->> @@ -2058,6 +2069,9 @@ File References Live Value +Opaque.+testConvertNestedRecordFromOtherFile: 0 references () [0] Live Value +Opaque.+noConversion: 0 references () [0] Dead VariantCase +Opaque.opaqueFromRecords.A: 0 references () [0] + Live Value +OptionalArgsLiveDead.+liveCaller: 1 references (OptionalArgsLiveDead.res:7:8) [0] + Dead Value +OptionalArgsLiveDead.+deadCaller: 0 references () [0] + Live Value +OptionalArgsLiveDead.+formatDate: 1 references (OptionalArgsLiveDead.res:5:4) [0] Live Value +Records.+testMyRecBsAs2: 0 references () [0] Live Value +Records.+testMyRecBsAs: 0 references () [0] Live RecordLabel +Records.myRecBsAs.type_: 1 references (Records.res:145:38) [0] @@ -2479,70 +2493,6 @@ File References DeadTest.res:153:1-28 deadIncorrect is annotated @dead but is live - Warning Unused Argument - TestOptArg.res:9:1-65 - optional argument x of function notSuppressesOptArgs is never used - - Warning Unused Argument - TestOptArg.res:9:1-65 - optional argument y of function notSuppressesOptArgs is never used - - Warning Unused Argument - TestOptArg.res:9:1-65 - optional argument z of function notSuppressesOptArgs is never used - - Warning Redundant Optional Argument - TestOptArg.res:3:1-28 - optional argument x of function foo is always supplied (1 calls) - - Warning Redundant Optional Argument - Unison.res:17:1-60 - optional argument break of function group is always supplied (2 calls) - - Warning Unused Argument - OptArg.resi:2:1-50 - optional argument x of function bar is never used - - Warning Redundant Optional Argument - OptArg.res:26:1-70 - optional argument c of function wrapfourArgs is always supplied (2 calls) - - Warning Unused Argument - OptArg.res:24:1-63 - optional argument d of function fourArgs is never used - - Warning Redundant Optional Argument - OptArg.res:20:1-51 - optional argument a of function wrapOneArg is always supplied (1 calls) - - Warning Unused Argument - OptArg.res:14:1-42 - optional argument a of function twoArgs is never used - - Warning Unused Argument - OptArg.res:14:1-42 - optional argument b of function twoArgs is never used - - Warning Unused Argument - OptArg.res:9:1-54 - optional argument b of function threeArgs is never used - - Warning Redundant Optional Argument - OptArg.res:9:1-54 - optional argument a of function threeArgs is always supplied (2 calls) - - Warning Unused Argument - OptArg.res:3:1-38 - optional argument x of function bar is never used - - Warning Unused Argument - OptArg.res:1:1-48 - optional argument y of function foo is never used - - Warning Unused Argument - OptArg.res:1:1-48 - optional argument z of function foo is never used - Warning Dead Module AutoAnnotate.res:0:1 AutoAnnotate is a dead module as all its items are dead. @@ -3459,6 +3409,10 @@ File References OptArg.resi:1:1-54 foo is never used + Warning Dead Value + OptionalArgsLiveDead.res:3:1-59 + deadCaller is never used + Warning Dead Type Records.res:24:3-14 person.name is a record label never used to read a value @@ -3682,5 +3636,77 @@ File References Warning Dead Type VariantsWithPayload.res:96:23-32 variant1Object.R is a variant case which is never constructed + + Warning Unused Argument + OptArg.res:24:1-63 + optional argument d of function fourArgs is never used + + Warning Unused Argument + TestOptArg.res:9:1-65 + optional argument x of function notSuppressesOptArgs is never used + + Warning Unused Argument + TestOptArg.res:9:1-65 + optional argument y of function notSuppressesOptArgs is never used + + Warning Unused Argument + TestOptArg.res:9:1-65 + optional argument z of function notSuppressesOptArgs is never used + + Warning Unused Argument + OptArg.res:1:1-48 + optional argument y of function foo is never used + + Warning Unused Argument + OptArg.res:1:1-48 + optional argument z of function foo is never used + + Warning Redundant Optional Argument + OptArg.res:1:1-48 + optional argument x of function foo is always supplied (1 calls) + + Warning Unused Argument + OptArg.resi:2:1-50 + optional argument x of function bar is never used + + Warning Unused Argument + OptArg.res:3:1-38 + optional argument x of function bar is never used + + Warning Unused Argument + OptArg.res:9:1-54 + optional argument b of function threeArgs is never used + + Warning Redundant Optional Argument + OptArg.res:9:1-54 + optional argument a of function threeArgs is always supplied (2 calls) + + Warning Unused Argument + OptionalArgsLiveDead.res:1:1-33 + optional argument fmt of function formatDate is never used + + Warning Redundant Optional Argument + Unison.res:17:1-60 + optional argument break of function group is always supplied (2 calls) + + Warning Unused Argument + OptArg.res:14:1-42 + optional argument a of function twoArgs is never used + + Warning Unused Argument + OptArg.res:14:1-42 + optional argument b of function twoArgs is never used + + Warning Redundant Optional Argument + TestOptArg.res:3:1-28 + optional argument x of function foo is always supplied (1 calls) + + Warning Redundant Optional Argument + OptArg.res:20:1-51 + optional argument a of function wrapOneArg is always supplied (1 calls) + + Warning Redundant Optional Argument + OptArg.res:26:1-70 + optional argument c of function wrapfourArgs is always supplied (2 calls) - Analysis reported 302 issues (Incorrect Dead Annotation:1, Warning Dead Exception:2, Warning Dead Module:21, Warning Dead Type:87, Warning Dead Value:173, Warning Dead Value With Side Effects:2, Warning Redundant Optional Argument:5, Warning Unused Argument:11) + Analysis reported 305 issues (Incorrect Dead Annotation:1, Warning Dead Exception:2, Warning Dead Module:21, Warning Dead Type:87, Warning Dead Value:174, Warning Dead Value With Side Effects:2, Warning Redundant Optional Argument:6, Warning Unused Argument:12) diff --git a/tests/analysis_tests/tests-reanalyze/deadcode/src/OptionalArgsLiveDead.res b/tests/analysis_tests/tests-reanalyze/deadcode/src/OptionalArgsLiveDead.res new file mode 100644 index 0000000000..367187be53 --- /dev/null +++ b/tests/analysis_tests/tests-reanalyze/deadcode/src/OptionalArgsLiveDead.res @@ -0,0 +1,8 @@ +let formatDate = (~fmt=?, s) => s + +let deadCaller = () => formatDate(~fmt="ISO", "2024-01-01") + +let liveCaller = () => formatDate("2024-01-01") + +let _ = liveCaller() +