-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: remove Swift from fallback chunking extensions #7325
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| # Lessons Learned: Swift Parser Investigation | ||
|
|
||
| ## 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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| ] | ||
|
Contributor
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. 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. |
||
|
|
||
| /** | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.