-
Notifications
You must be signed in to change notification settings - Fork 262
feat(swc): dead code eliminate unreferenced private class members in workflow mode #1671
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@workflow/swc-plugin": patch | ||
| --- | ||
|
|
||
| Eliminate unreferenced private class members in workflow mode after `"use step"` stripping |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ use swc_core::{ | |
| common::{errors::HANDLER, SyntaxContext, DUMMY_SP}, | ||
| ecma::{ | ||
| ast::*, | ||
| visit::{noop_visit_mut_type, VisitMut, VisitMutWith}, | ||
| visit::{noop_visit_mut_type, noop_visit_type, Visit, VisitMut, VisitMutWith, VisitWith}, | ||
| }, | ||
| }; | ||
|
|
||
|
|
@@ -491,6 +491,198 @@ impl TryFrom<&Expr> for Name { | |
| } | ||
| } | ||
|
|
||
| /// Collects all member names referenced within an AST subtree via | ||
| /// `this.foo`, `this.#foo`, or `obj.foo` (when `foo` is a known | ||
| /// TS-private name) patterns. Used after stripping `"use step"` methods | ||
| /// in workflow mode to determine which private class members are still | ||
| /// referenced by the remaining body, so unreferenced ones can be | ||
| /// dead-code-eliminated. | ||
| /// | ||
| /// Handles both: | ||
| /// - JS native private members (`#field`, `#method()`) — stored with `#` | ||
| /// prefix to avoid collisions with TS private members of the same name | ||
| /// - TypeScript `private` members — stored without prefix; detected via | ||
| /// `this.foo` and also `obj.foo` when `foo` is a known TS-private name | ||
| /// (to handle same-class access patterns like `static compare(a, b) { | ||
| /// return a.x - b.x }`) | ||
| struct ClassMemberRefCollector { | ||
| /// All member names referenced. JS native private names are prefixed | ||
| /// with `#` (e.g. `"#foo"`), TS private names are unprefixed (`"foo"`). | ||
| referenced: HashSet<String>, | ||
| /// Known TS-private member names in the current class, so that `a.foo` | ||
| /// accesses (not just `this.foo`) are recognized as references. | ||
| ts_private_names: HashSet<String>, | ||
| } | ||
|
|
||
| impl ClassMemberRefCollector { | ||
| fn new(ts_private_names: HashSet<String>) -> Self { | ||
| Self { | ||
| referenced: HashSet::new(), | ||
| ts_private_names, | ||
| } | ||
| } | ||
|
|
||
| /// Collects all member names transitively referenced by non-private | ||
| /// (public) members of the class. Private members that are only | ||
| /// referenced by other private members (which are themselves | ||
| /// unreferenced) are NOT included, enabling cascading elimination. | ||
| /// | ||
| /// Algorithm: seed the referenced set from public members, then | ||
| /// iteratively expand by adding references from surviving private | ||
| /// members until the set stabilizes. | ||
| fn collect_from_class_body(body: &[ClassMember]) -> HashSet<String> { | ||
| // Build the set of known TS-private names for the collector | ||
| let ts_private_names: HashSet<String> = body | ||
| .iter() | ||
| .filter_map(|m| match m { | ||
| ClassMember::Method(m) if m.accessibility == Some(Accessibility::Private) => { | ||
| match &m.key { | ||
| PropName::Ident(i) => Some(i.sym.to_string()), | ||
| PropName::Str(s) => Some(s.value.to_string_lossy().to_string()), | ||
| _ => None, | ||
| } | ||
| } | ||
| ClassMember::ClassProp(p) if p.accessibility == Some(Accessibility::Private) => { | ||
| match &p.key { | ||
| PropName::Ident(i) => Some(i.sym.to_string()), | ||
| PropName::Str(s) => Some(s.value.to_string_lossy().to_string()), | ||
| _ => None, | ||
| } | ||
| } | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Phase 1: collect references from all non-private members | ||
| let mut collector = Self::new(ts_private_names); | ||
| for member in body { | ||
| if !Self::is_private_member(member) { | ||
| member.visit_with(&mut collector); | ||
| } | ||
| } | ||
|
|
||
| // Phase 2: iteratively expand — if a private member is referenced, | ||
| // its body may reference other private members | ||
| loop { | ||
| let prev_len = collector.referenced.len(); | ||
| for member in body { | ||
| if let Some(name) = Self::private_member_name(member) { | ||
| if collector.referenced.contains(&name) { | ||
| // This private member survived; scan its body for | ||
| // references to other private members | ||
| Self::visit_member_body(member, &mut collector); | ||
| } | ||
| } | ||
| } | ||
| if collector.referenced.len() == prev_len { | ||
| break; // fixed point reached | ||
| } | ||
| } | ||
|
|
||
| collector.referenced | ||
| } | ||
|
|
||
| /// Visit the body/initializer of a class member for reference collection. | ||
| fn visit_member_body(member: &ClassMember, collector: &mut Self) { | ||
| match member { | ||
| ClassMember::PrivateMethod(m) => { | ||
| if let Some(body) = &m.function.body { | ||
| body.visit_with(collector); | ||
| } | ||
| } | ||
| ClassMember::PrivateProp(p) => { | ||
| if let Some(value) = &p.value { | ||
| value.visit_with(collector); | ||
| } | ||
| } | ||
| ClassMember::Method(m) => { | ||
| if let Some(body) = &m.function.body { | ||
| body.visit_with(collector); | ||
| } | ||
| } | ||
| ClassMember::ClassProp(p) => { | ||
| if let Some(value) = &p.value { | ||
| value.visit_with(collector); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| /// Returns true if the member is a private member (JS native or TS). | ||
| fn is_private_member(member: &ClassMember) -> bool { | ||
| matches!( | ||
| member, | ||
| ClassMember::PrivateMethod(_) | ClassMember::PrivateProp(_) | ||
| ) || matches!(member, ClassMember::Method(m) if m.accessibility == Some(Accessibility::Private)) | ||
| || matches!(member, ClassMember::ClassProp(p) if p.accessibility == Some(Accessibility::Private)) | ||
| } | ||
|
|
||
| /// Returns the canonical name of a private member. JS native private | ||
| /// names are prefixed with `#` to avoid collisions with TS private | ||
| /// members of the same name. | ||
| fn private_member_name(member: &ClassMember) -> Option<String> { | ||
| match member { | ||
| ClassMember::PrivateMethod(m) => Some(format!("#{}", m.key.name)), | ||
| ClassMember::PrivateProp(p) => Some(format!("#{}", p.key.name)), | ||
| ClassMember::Method(m) if m.accessibility == Some(Accessibility::Private) => { | ||
| match &m.key { | ||
| PropName::Ident(i) => Some(i.sym.to_string()), | ||
| PropName::Str(s) => Some(s.value.to_string_lossy().to_string()), | ||
| _ => None, | ||
| } | ||
| } | ||
| ClassMember::ClassProp(p) if p.accessibility == Some(Accessibility::Private) => { | ||
| match &p.key { | ||
| PropName::Ident(i) => Some(i.sym.to_string()), | ||
| PropName::Str(s) => Some(s.value.to_string_lossy().to_string()), | ||
| _ => None, | ||
| } | ||
| } | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| /// Removes unreferenced private class members from a class body. | ||
| /// Call after stripping `"use step"` methods in workflow mode. | ||
| fn retain_referenced_private_members(body: &mut Vec<ClassMember>) { | ||
| let referenced = Self::collect_from_class_body(body); | ||
| body.retain(|member| { | ||
| if let Some(name) = Self::private_member_name(member) { | ||
| referenced.contains(&name) | ||
| } else { | ||
| true | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| impl Visit for ClassMemberRefCollector { | ||
| noop_visit_type!(); | ||
|
|
||
| fn visit_member_expr(&mut self, expr: &MemberExpr) { | ||
| match &expr.prop { | ||
| // Native JS private: `this.#foo` — stored as `#foo` | ||
| MemberProp::PrivateName(name) => { | ||
| self.referenced.insert(format!("#{}", name.name)); | ||
| } | ||
| // TS private or any ident member access. Track `this.foo` as | ||
| // before, and also track `obj.foo` when `foo` is a known | ||
| // TS-private member of the current class so same-class | ||
| // accesses like `a.x` / `b.x` are not missed. | ||
| MemberProp::Ident(ident) => { | ||
| let name = ident.sym.to_string(); | ||
| if matches!(&*expr.obj, Expr::This(_)) || self.ts_private_names.contains(&name) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI Review: Note For
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct - the collector is conservative: it records all |
||
| self.referenced.insert(name); | ||
| } | ||
|
Comment on lines
+664
to
+677
|
||
| } | ||
| _ => {} | ||
| } | ||
| // Continue visiting children, including computed property expressions | ||
| expr.visit_children_with(self); | ||
| } | ||
| } | ||
|
|
||
| // Visitor to collect closure variables from a nested step function | ||
| struct ClosureVariableCollector { | ||
| closure_vars: HashSet<String>, | ||
|
|
@@ -8194,6 +8386,14 @@ impl VisitMut for StepTransform { | |
| } | ||
| true | ||
| }); | ||
|
|
||
| // After stripping "use step" methods, eliminate private class | ||
| // members (both JS native `#field`/`#method()` and TypeScript | ||
| // `private field`/`private method()`) that are no longer | ||
| // referenced by any remaining member. | ||
| ClassMemberRefCollector::retain_referenced_private_members( | ||
| &mut class_decl.class.body, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -8312,6 +8512,11 @@ impl VisitMut for StepTransform { | |
| } | ||
| true | ||
| }); | ||
|
|
||
| // Dead-code-eliminate unreferenced private members | ||
| ClassMemberRefCollector::retain_referenced_private_members( | ||
| &mut class_expr.class.body, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { WORKFLOW_SERIALIZE, WORKFLOW_DESERIALIZE } from '@workflow/serde'; | ||
| import { getWorld } from './world.js'; | ||
| import { importKey } from './encryption.js'; | ||
|
|
||
| export class Run { | ||
| static [WORKFLOW_SERIALIZE](instance) { | ||
| return { id: instance.id }; | ||
| } | ||
|
|
||
| static [WORKFLOW_DESERIALIZE](data) { | ||
| return new Run(data.id); | ||
| } | ||
|
|
||
| // Public field — should be kept | ||
| id; | ||
|
|
||
| // Native private field — only referenced by #getEncryptionKey | ||
| #encryptionKeyPromise = null; | ||
|
|
||
| // Native private field — referenced by toString (public), should survive | ||
| #label = 'run'; | ||
|
|
||
| // Native private method — only called by stripped step methods | ||
| async #getEncryptionKey() { | ||
| if (!this.#encryptionKeyPromise) { | ||
| this.#encryptionKeyPromise = importKey(this.id); | ||
| } | ||
| return this.#encryptionKeyPromise; | ||
| } | ||
|
|
||
| constructor(id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| get value() { | ||
| 'use step'; | ||
| return this.#getEncryptionKey().then(() => getWorld().get(this.id)); | ||
| } | ||
|
|
||
| async cancel() { | ||
| 'use step'; | ||
| const key = await this.#getEncryptionKey(); | ||
| await getWorld().cancel(this.id, key); | ||
| } | ||
|
|
||
| toString() { | ||
| return `Run(${this.id}, ${this.#label})`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI Review: Nit
This section explains the algorithm well. If you want to reduce future issue reports, a short explicit note on unsupported patterns (for example computed
this[...]access to private fields) could help set expectations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. Computed access like
this[name]wherenamehappens to be a private member name won't be tracked, which could lead to incorrect elimination. In practice this pattern is very rare for private members (especially in SDK code likeRun), but worth noting if we want to document limitations.