Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions docs/lessons-learned/swift-parser-investigation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# Lessons Learned: Swift Parser Investigation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent lessons learned document! For a document of this length, would it be helpful to add a table of contents at the top? It would make navigation easier for future readers looking for specific sections.


## Issue Context

**Date:** January 22, 2025
**PR:** #7309 - "fix: prevent Swift parser loading to avoid VS Code GUI crashes"
**Issue:** #7308 - Reports of VS Code GUI crashes when working with Swift/iOS projects

## Initial Incorrect Approach

### What Happened

1. When faced with reports of VS Code crashes with Swift files, an assumption was made that the Swift tree-sitter parser was unstable
2. The proposed solution was to add Swift to the fallback extensions list, bypassing the tree-sitter parser entirely
3. This approach was implemented without concrete evidence of parser instability

### Why This Was Wrong

- **No Evidence**: There was no actual proof that the Swift parser was unstable
- **Ignored User Experience**: Developers who work with Swift daily (like @adamhill) had been using VS Code with Swift projects without issues
- **Hasty Solution**: Jumped to a solution without proper investigation of the root cause

## Correct Approach Demonstrated

### What Should Have Been Done

1. **Investigate First**: Before assuming parser instability, should have:

- Checked if the Swift parser loads correctly
- Reviewed existing test suites for Swift parsing
- Looked for error logs or crash reports specifically related to the parser
- Tested Swift file parsing in isolation

2. **Gather Evidence**: Should have collected:

- Specific error messages or stack traces
- Reproducible test cases
- User reports with detailed context

3. **Respect Domain Expertise**: Should have considered that:
- Developers using Swift daily would know if there were systematic issues
- Their experience is valuable real-world evidence
- Community feedback is crucial for understanding actual vs. perceived issues

## Technical Findings

### Swift Parser Status

After proper investigation, it was found that:

- The `tree-sitter-swift.wasm` file exists and loads properly
- Swift has proper query patterns defined in `src/services/tree-sitter/queries/swift.ts`
- Test suites exist for Swift parsing (though marked as skip due to performance, not instability)
- The parser is functional and not inherently unstable

### Current Implementation

- Swift parser is properly integrated in `languageParser.ts`
- Query patterns support various Swift constructs (classes, structs, protocols, etc.)
- The parser handles complex Swift syntax including generics and protocol conformance

## Key Lessons

### 1. Evidence-Based Decision Making

- **Never assume** technical problems without concrete evidence
- **Always investigate** thoroughly before proposing solutions
- **Document findings** to support technical decisions

### 2. Respect User Experience

- **Listen to users** who work with the technology daily
- **Value real-world experience** over theoretical assumptions
- **Engage with the community** before making breaking changes

### 3. Proper Debugging Process

- **Start with reproduction**: Can the issue be reproduced?
- **Isolate the problem**: Is it really the parser, or something else?
- **Test hypotheses**: Verify assumptions with actual tests
- **Review existing code**: Understand current implementation before changing it

### 4. Communication and Humility

- **Admit mistakes quickly** when corrected
- **Learn from feedback** and incorporate it into future work
- **Thank reviewers** for their patience and expertise
- **Document lessons learned** for future reference

## Action Items for Future Issues

When encountering similar issues:

1. **Before Making Changes:**

- [ ] Reproduce the issue locally
- [ ] Check existing tests and documentation
- [ ] Review related code thoroughly
- [ ] Search for similar past issues

2. **During Investigation:**

- [ ] Collect concrete evidence (logs, stack traces, etc.)
- [ ] Test individual components in isolation
- [ ] Consider multiple hypotheses
- [ ] Consult with domain experts if available

3. **When Proposing Solutions:**
- [ ] Provide evidence supporting the solution
- [ ] Consider impact on existing users
- [ ] Test the solution thoroughly
- [ ] Be open to feedback and alternative approaches

## Related Files for Reference

- Parser implementation: `src/services/tree-sitter/languageParser.ts`
- Swift queries: `src/services/tree-sitter/queries/swift.ts`
- Swift tests: `src/services/tree-sitter/__tests__/parseSourceCodeDefinitions.swift.spec.ts`
- Fallback extensions: `src/services/code-index/shared/supported-extensions.ts`

## Conclusion

This incident serves as a valuable reminder that thorough investigation and evidence-based decision making are crucial in software development. Assumptions, especially about stability issues, should never be made without concrete proof. The Swift parser case demonstrates that what might seem like an obvious problem (parser instability causing crashes) can be completely incorrect, and the actual issue may lie elsewhere entirely.

The grace and patience shown by @adamhill in correcting this mistake is appreciated, and this documentation ensures that the lessons learned are preserved for future reference.
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,9 @@ describe("Fallback Extensions Configuration", () => {
// Extensions that should use fallback
expect(shouldUseFallbackChunking(".vb")).toBe(true)
expect(shouldUseFallbackChunking(".scala")).toBe(true)
expect(shouldUseFallbackChunking(".swift")).toBe(true)

// Extensions that should not use fallback (have working parsers)
expect(shouldUseFallbackChunking(".swift")).toBe(false) // Swift has a working parser
expect(shouldUseFallbackChunking(".js")).toBe(false)
expect(shouldUseFallbackChunking(".ts")).toBe(false)
expect(shouldUseFallbackChunking(".py")).toBe(false)
Expand Down
1 change: 0 additions & 1 deletion src/services/code-index/shared/supported-extensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export const scannerExtensions = allExtensions
export const fallbackExtensions = [
".vb", // Visual Basic .NET - no dedicated WASM parser
".scala", // Scala - uses fallback chunking instead of Lua query workaround
".swift", // Swift - uses fallback chunking due to parser instability
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a brief comment here explaining why Swift was removed? Something like:

This would help future maintainers understand the history without needing to dig through PRs.


/**
Expand Down
Loading