feat(flags): Support indirect destructuring and property access patterns

[kdy1]

Summary

Extends the swc_feature_flags build-time transformer to support multiple ways of accessing feature flags beyond direct destructuring.

New patterns now supported:

• ✅ Indirect destructuring: const flags = useFlags(); const { featureA } = flags;
• ✅ Property access: const flags = useFlags(); if (flags.featureA) {}
• ✅ Mixed patterns: All patterns can be used together in the same file

All patterns transform identically:

// Before (any of these patterns)
featureA              // from destructuring
flags.featureA        // from property access

// After
__SWC_FLAGS__.featureA

Motivation

Previously, the build-time transform only supported direct destructuring:

// ✅ Worked before
const { featureA } = useExperimentalFlags();

This was limiting for developers who prefer intermediate variables or property access:

// ❌ Didn't work before
const flags = useExperimentalFlags();
const { featureA } = flags;  // or: if (flags.featureA) { }

This PR makes the transform work consistently across all usage patterns.

Implementation Details

Core Changes

1. Data Structure: Added FlagObjectInfo struct and flag_object_map to track flag object variables
2. Analysis: Enhanced analyze_declarator() to detect three patterns:
   • Direct destructuring: const { featureA } = useFlags()
   • Flag object binding: const flags = useFlags()
   • Indirect destructuring: const { featureA } = flags
3. Transformation: Extended visit_mut_expr() to transform member expressions: flags.featureA → __SWC_FLAGS__.featureA
4. Smart Removal: Declarations are only removed when flags are actually transformed (respects exclude_flags)

Key Features

• Scope Safety: Uses SWC's Id system to correctly handle variable shadowing
• Exclude Flags: Works correctly with all patterns
• Backward Compatible: All existing tests pass, no breaking changes
• Clean Output: Flag object declarations removed only when necessary

Testing

Added 5 new test fixtures (10 files total):

1. indirect-destructuring: const flags = useFlags(); const { featureA } = flags;
2. property-access: const flags = useFlags(); if (flags.featureA) {}
3. mixed-patterns: All patterns used together
4. scope-safety-object: Ensures shadowing works correctly
5. exclude-flags: Tests exclusion with new patterns

Test results: ✅ All 10 tests passing (7 build-time + 3 runtime)

Example Transformations

Indirect Destructuring:

// Input
const flags = useExperimentalFlags();
const { featureA } = flags;
if (featureA) { console.log('enabled'); }

// Output
if (__SWC_FLAGS__.featureA) { console.log('enabled'); }

Property Access:

// Input
const flags = useExperimentalFlags();
if (flags.featureA) { console.log('enabled'); }

// Output
if (__SWC_FLAGS__.featureA) { console.log('enabled'); }

Scope Safety:

// Input
const flags = useExperimentalFlags();
if (flags.featureA) {  // Transformed
  const flags = { featureA: false };
  if (flags.featureA) {  // NOT transformed - local scope
  }
}

// Output
if (__SWC_FLAGS__.featureA) {
  const flags = { featureA: false };
  if (flags.featureA) {  // Kept as-is
  }
}

Test Infrastructure Improvements

• Added support for per-test options.json configuration files
• Added serde_json as dev dependency for test configuration parsing
• Tests can now specify custom exclude_flags per fixture

Documentation

Updated README.md with:

• "Supported Usage Patterns" section with examples
• Clarification that all three patterns can be mixed freely

Files Changed

• crates/swc_feature_flags/src/build_time.rs - Core transformation logic
• crates/swc_feature_flags/tests/fixture.rs - Test infrastructure
• crates/swc_feature_flags/Cargo.toml - Added serde_json dependency
• crates/swc_feature_flags/README.md - Documentation
• 5 new test fixture directories (11 new files)

Breaking Changes

None. This is a pure feature addition that's backward compatible.

Test Plan

• All existing tests pass
• New tests for indirect destructuring
• New tests for property access
• New tests for mixed patterns
• New tests for scope safety
• New tests for exclude flags with new patterns
• Documentation updated

🤖 Generated with Claude Code

[changeset-bot]

🦋 Changeset detected

Latest commit: 0579961

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[claude]

Code Review: PR #583 - Support indirect destructuring and property access patterns

Summary

This PR extends the swc_feature_flags build-time transformer to support multiple patterns for accessing feature flags beyond direct destructuring. The implementation is well-designed, thoroughly tested, and maintains backward compatibility.

✅ Strengths

1. Excellent Code Quality

• Clean abstraction: The new extract_flags_from_object_pattern() method eliminates code duplication effectively
• Clear separation of concerns: Three distinct patterns are handled systematically in analyze_declarator()
• Proper use of Rust idioms: Good use of pattern matching, early returns, and error handling
• Well-documented: Comments clearly explain each pattern and the reasoning behind design decisions

2. Robust Implementation

• Scope safety: Uses SWC's Id system correctly to handle variable shadowing (tested in scope-safety-object fixture)
• Smart removal logic: Declarations are only removed when flags are actually transformed, respecting exclude_flags
• Proper span tracking: Uses span.lo.0 consistently for tracking declarators to remove

3. Comprehensive Testing

• 5 new test fixtures covering all patterns and edge cases
• Test infrastructure improvements: Per-test options.json configuration is a great addition
• All tests passing: Build-time and runtime tests both pass
• Good test coverage: Tests cover indirect destructuring, property access, mixed patterns, scope safety, and exclude flags

4. Documentation

• Clear README updates with examples for all three patterns
• Detailed PR description with motivation, implementation details, and examples
• Code comments explain non-obvious implementation choices

🔍 Observations & Suggestions

1. Potential Edge Case: Nested Destructuring (Low Priority)

The current implementation may not handle deeply nested destructuring:

const flags = useExperimentalFlags();
const { nested: { featureA } } = flags; // May not be transformed

However, this is likely an acceptable limitation given typical usage patterns. Consider documenting this limitation if it's intentional.

2. Member Expression Edge Case (Low Priority)

In visit_mut_expr() at line 255-274, the code handles flags.featureA but may not handle:

• Computed property access: flags['featureA'] or flags[key]
• Optional chaining: flags?.featureA

These are edge cases that may not need support, but worth considering for completeness.

3. Code Organization (Minor)

The implementation at lines 152-174 has a comment about avoiding borrow checker issues by extracting span_lo early. This is correct, but the logic flow could be slightly cleaner with a match guard or by restructuring. Not critical, but consider:

// Alternative approach (optional)
if let Pat::Object(obj_pat) = &declarator.name {
    if let Some(init) = &declarator.init {
        if let Expr::Ident(ident) = &**init {
            let source_id = ident.to_id();
            if let Some(info) = self.flag_object_map.get(&source_id) {
                let source_span = info.span_lo;
                let extracted = self.extract_flags_from_object_pattern(obj_pat);
                if extracted {
                    self.declarators_to_remove.insert(declarator.span.lo.0);
                    self.declarators_to_remove.insert(source_span);
                }
            }
        }
    }
}

4. Test Infrastructure (Positive)

The addition of serde_json as a dev dependency and the TestOptions struct is well-designed. Good forward-thinking for future test configuration needs.

🔒 Security Considerations

✅ No security concerns identified:

• No unsafe code introduced
• Proper handling of user input (flag names)
• No potential for injection or code execution vulnerabilities
• Span-based tracking prevents identifier collisions

⚡ Performance Considerations

✅ Performance impact is minimal and well-designed:

• Three-pass approach (analyze, transform, cleanup) is efficient for this use case
• HashMap lookups for flag_map and flag_object_map are O(1)
• Span tracking avoids expensive re-traversals
• The additional overhead of tracking flag_object_map is negligible

🧪 Test Coverage

✅ Excellent test coverage:

• Direct destructuring: ✅ (existing basic test)
• Indirect destructuring: ✅ (new indirect-destructuring test)
• Property access: ✅ (new property-access test)
• Mixed patterns: ✅ (new mixed-patterns test)
• Scope safety: ✅ (both nested-scope and scope-safety-object tests)
• Exclude flags: ✅ (new exclude-flags test with options.json)

All 10 tests pass successfully.

📋 Best Practices Compliance

✅ Follows Rust/SWC best practices:

• Proper use of VisitMut trait
• Consistent naming conventions
• No clippy warnings (besides unrelated unreachable patterns in stats.rs)
• Proper error handling
• Good use of pattern matching

🎯 Recommendation

APPROVE with minor suggestions ✅

This is a high-quality PR that:

• Solves a real developer pain point
• Maintains backward compatibility
• Is thoroughly tested
• Has clear documentation
• Follows best practices

The implementation is production-ready. The suggestions above are minor enhancements that could be addressed in future PRs if needed.

💬 Questions for Author

1. Is the limitation around computed property access (flags[key]) intentional?
2. Should optional chaining (flags?.featureA) be supported, or is it expected that flag objects are always defined?

Great work on this feature! The code quality and testing are exemplary. 🎉

---

Review generated by Claude Code

[kdy1]

We don't need a feature to exclude a feature flag. @claude Discard changes related to 'exclude' feature

[claude]

Claude finished @kdy1's task in 4m 57s —— View job

---

Completed: Exclude feature removed

Successfully removed all exclude flags functionality from the PR while preserving the core indirect destructuring and property access patterns feature.

Changes Made

Removed:

1. exclude-flags test fixture directory (3 files)
2. Test infrastructure: TestOptions struct and options.json parsing in fixture.rs
3. serde_json dev dependency from Cargo.toml
4. exclude_flags field from FeatureFlagsConfig and BuildTimeConfig structs
5. Exclude flag checks in build_time.rs (3 locations)
6. Documentation references to exclude feature in README.md and lib.rs

Preserved:

• All core functionality for indirect destructuring
• All property access pattern support
• All other test fixtures (9 tests passing)

Verification

• ✅ Code compiles successfully
• ✅ All 9 tests pass (down from 10 after removing exclude-flags test)
• ✅ Changes committed and pushed

--- | Branch: kdy1/more-dce

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ kdy1
❌ github-actions[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[kdy1]

@claude Fix CI

[claude]

Claude finished @kdy1's task in 5m 53s —— View job

---

CI Fixed ✅

What Was Wrong

The CI was failing because when the exclude_flags feature was removed in commit f59632b, the plugin wrapper package swc_plugin_experimental_feature_flags (packages/feature-flags/src/lib.rs) wasn't updated. It was still trying to pass config.exclude_flags to BuildTimeConfig, which no longer has that field.

Changes Made

• Removed the exclude_flags: config.exclude_flags, line from packages/feature-flags/src/lib.rs:36
• Verified all tests pass locally
• Committed and pushed the fix

Verification

• ✅ Build succeeds: cargo build --all
• ✅ All tests pass: cargo test --all
• ✅ Fix committed: 0933167
• ✅ Changes pushed to branch

The CI should now pass once the new workflow run completes.

--- | Branch: kdy1/more-dce

[claude]

Pull Request Review: Feature Flags - Support for Indirect Destructuring and Property Access

Summary

This PR extends the swc_feature_flags build-time transformer to support additional patterns for accessing feature flags beyond direct destructuring. The implementation is well-designed and the tests demonstrate the new functionality works correctly.

✅ Strengths

1. Strong Architecture

• Clean separation of concerns with the new FlagObjectInfo struct
• Good use of SWC's Id system for scope-safe transformations
• Three-pass analysis approach (analyze → transform → cleanup) is sound

2. Excellent Test Coverage

• 4 new test fixtures covering all new patterns
• Tests pass successfully (9/9 passing)
• Good edge case coverage including scope safety and mixed patterns

3. Backward Compatibility

• All existing tests pass
• No breaking changes
• Pure additive feature

4. Documentation

• Comprehensive PR description with examples
• README updated with clear usage patterns
• Code comments explain the logic

🔍 Code Quality Observations

build_time.rs (lines 65-106)

Good:

• The extract_flags_from_object_pattern method properly handles both KeyValue and Assign patterns
• Proper handling of UTF-8 conversion for string property names (lines 77-84)

Minor Concern:
The method continues processing all properties even after encountering errors in string conversion. This is likely fine, but consider whether you want to log/track failed conversions for debugging.

build_time.rs (lines 108-165)

Issue - Potential Bug in Indirect Destructuring:

In Pattern 3 (lines 142-164), when handling indirect destructuring like const { flagA } = flags, the code marks both declarations for removal:

self.declarators_to_remove.insert(declarator.span.lo.0);
self.declarators_to_remove.insert(source_span);

Problem: If flags is used multiple times (destructured OR accessed), this could lead to the source being removed on the first usage, but then subsequent usages in visit_mut_expr (line 256) will try to access flag_object_map and insert the span again. This appears to work correctly based on tests, but the logic is somewhat implicit.

Potential Edge Case:

const flags = useExperimentalFlags();
const { featureA } = flags;  // First usage - marks flags for removal
const { featureB } = flags;  // Second usage - tries to mark again (already in set)
console.log(flags.featureC); // Third usage - also marks for removal

This should work fine since HashSet handles duplicates, but it's worth noting that the removal decision is made incrementally across multiple sites.

build_time.rs (lines 245-261)

Potential Issue - Member Expression Handling:

When transforming flags.featureA → __SWC_FLAGS__.featureA (line 257), the code doesn't validate that the property is actually a valid feature flag. This means ANY property access on the flags object gets transformed:

const flags = useExperimentalFlags();
const x = flags.unknownProperty; // This WILL be transformed
const y = flags.toString();      // This too!

Recommendation: Consider whether you want to validate against a known set of flags, or document this behavior clearly. The current approach is permissive (transforms all property accesses), which could be intentional for flexibility.

config.rs - Breaking Change Consideration

The removal of exclude_flags from BuildTimeConfig is marked as non-breaking, but it IS a config schema change:

Before:

pub struct BuildTimeConfig {
    pub libraries: HashMap<String, LibraryConfig>,
    pub exclude_flags: Vec<String>,  // REMOVED
    pub marker_object: String,
}

Impact:

• Existing config files with excludeFlags will have that field ignored (via #[serde(default)] behavior)
• This is technically non-breaking for functionality but could be confusing for users

Recommendation: Add a deprecation note to the documentation explaining that exclude_flags was removed and why (if this was intentional).

🐛 Potential Bugs

1. Multiple Flag Object Declarations with Same Name

Scenario:

function outer() {
  const flags = useExperimentalFlags();
  if (flags.featureA) {
    function inner() {
      const flags = useExperimentalFlags(); // Different flags, same name
      if (flags.featureB) {
        // ...
      }
    }
  }
}

Analysis: The code uses SWC's Id system (which includes scope), so this should work correctly. The test scope-safety-object validates similar behavior with non-flag shadowing. ✅ This appears handled correctly.

2. Destructuring from Non-Flag Variables

Scenario:

const flags = useExperimentalFlags();
const notFlags = { featureA: false };
const { featureA } = notFlags; // Should NOT be transformed

Analysis: Looking at lines 145-151, the code checks if let Some(source_span) = source_span_lo which verifies the source is in flag_object_map. ✅ This appears safe.

🎯 Best Practices

Good Practices Used:

1. ✅ Proper use of visit_mut_children_with for AST traversal
2. ✅ Span-based tracking for declarations to remove
3. ✅ Immutable borrowing followed by mutable operations to avoid borrow checker issues (line 147-151)
4. ✅ Early returns to avoid deep nesting (line 137)

🚀 Performance Considerations

Current Approach:

• Three-pass transformation (analyze → transform → remove) is reasonable for correctness
• HashSet lookups are O(1) on average
• The recursive analysis functions (lines 266-314) could potentially visit some nodes multiple times

Potential Optimization (Low Priority):

Consider using a single-pass visitor if performance becomes a concern. However, the current approach prioritizes clarity and correctness, which is appropriate for a transformer.

🔒 Security Concerns

No security issues identified. The transformation is purely syntactic and doesn't introduce injection risks or unsafe operations.

📝 Suggestions

1. Add Test Case: Computed Property Access

const flags = useExperimentalFlags();
const key = 'featureA';
console.log(flags[key]); // What happens here?

Currently, this would NOT be transformed (only MemberProp::Ident is handled). Consider documenting this limitation.

2. Add Test Case: Spread Operator

const flags = useExperimentalFlags();
const allFlags = { ...flags }; // What happens here?

This is an interesting edge case worth testing.

3. Add Test Case: Renamed Destructuring

const flags = useExperimentalFlags();
const { featureA: renamedFeature } = flags;
console.log(renamedFeature); // Should this be transformed?

Looking at line 90, the code extracts binding_ident which should handle this, but a test would confirm.

4. Consider Warning on Unused Flag Objects

If a flag object is declared but never used (no properties accessed or destructured), consider emitting a warning since the original call is removed.

5. Documentation: Add Migration Guide

Since exclude_flags was removed, provide guidance for users who were using it on how to achieve similar functionality (if applicable).

📊 Test Coverage Assessment

Current Coverage: Good ✅

• Direct destructuring ✅
• Indirect destructuring ✅
• Property access ✅
• Mixed patterns ✅
• Scope safety ✅

Additional Tests to Consider:

• Renamed destructuring (const { featureA: renamed } = flags)
• Spread operator usage
• Computed property access (to document limitation)
• Flag object passed as argument to function
• Multiple flag objects from different libraries in same scope

🎓 Overall Assessment

Verdict: APPROVE with minor suggestions ✅

This is a well-implemented feature that maintains backward compatibility while adding useful flexibility. The code quality is good, tests are comprehensive, and documentation is thorough.

Before Merging:

1. ✅ Tests pass (verified)
2. ✅ Documentation updated
3. ⚠️ Consider addressing the member expression validation concern
4. ⚠️ Add note about exclude_flags removal in changeset/migration guide
5. 💡 Consider adding suggested edge case tests (optional)

Minor Issues (Non-blocking):

• Member expression transforms ALL properties (may be intentional)
• Removal of exclude_flags could use migration note
• Some edge cases could use additional test coverage

The implementation correctly handles scope safety, multiple patterns, and avoids common pitfalls. Great work! 🎉

---

Review conducted by: Claude Code
Files reviewed: 5 core files + 8 test fixtures
Tests verified: All 9 tests passing

[Copilot]

Pull request overview

This PR extends the swc_feature_flags build-time transformer to support indirect destructuring and property access patterns for feature flags, beyond the previously supported direct destructuring pattern. However, it also removes the exclude_flags configuration option as a breaking change, which is not properly documented.

Changes:

• Added support for indirect destructuring (const flags = useFlags(); const { featureA } = flags;)
• Added support for property access patterns (flags.featureA)
• Removed exclude_flags configuration option from all config structs
• Added 4 new test fixtures for the new patterns

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
crates/swc_feature_flags/src/build_time.rs	Core transformation logic extended with flag_object_map tracking and member expression transformation
crates/swc_feature_flags/src/config.rs	Removed exclude_flags field from BuildTimeConfig and FeatureFlagsConfig structs (breaking change)
crates/swc_feature_flags/src/lib.rs	Updated documentation examples to remove exclude_flags references
crates/swc_feature_flags/tests/fixture.rs	Removed exclude_flags initialization from test configuration
crates/swc_feature_flags/README.md	Updated documentation to remove exclude_flags and add new usage patterns
crates/swc_feature_flags/tests/fixture/build-time/*/input.js	New test fixtures for indirect destructuring, property access, mixed patterns, and scope safety
crates/swc_feature_flags/tests/fixture/build-time/*/output.js	Expected outputs for new test fixtures
.changeset/little-cherries-compete.md	Changeset file (incorrectly marked as minor)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kdy1]

@claude fix CI

[claude]

Claude finished @kdy1's task in 2m 47s —— View job

---

CI Fixed ✅

Summary

The CI was failing because a test case for the removed exclude_flags feature was still present in the test suite. This test expected certain flags to NOT be transformed when excluded, but since the exclude feature was removed, all flags are now transformed, causing a snapshot mismatch.

Changes Made

Removed test case "Should handle exclude flags in mark mode":

• packages/feature-flags/__tests__/wasm.test.ts (removed test)
• packages/feature-flags/__tests__/__snapshots__/wasm.test.ts.snap (removed snapshot)

Verification

✅ All 9 Rust tests pass locally
✅ Changes committed: 0579961
✅ Changes pushed to branch kdy1/more-dce

The CI should now pass once the new workflow run completes.

---

View job | Branch: kdy1/more-dce