Skip to content

V0.0.4#147

Merged
leogdion merged 10 commits intomainfrom
v0.0.4
Apr 6, 2026
Merged

V0.0.4#147
leogdion merged 10 commits intomainfrom
v0.0.4

Conversation

@leogdion
Copy link
Copy Markdown
Member

@leogdion leogdion commented Apr 6, 2026

No description provided.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 6, 2026

Important

Review skipped

Too many files!

This PR contains 211 files, which is 61 over the limit of 150.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f39ec43d-5604-436a-a615-390d041770c1

📥 Commits

Reviewing files that changed from the base of the PR and between 1bd7f84 and 5802af0.

📒 Files selected for processing (211)
  • .devcontainer/devcontainer.json
  • .devcontainer/swift-6.0/devcontainer.json
  • .devcontainer/swift-6.1/devcontainer.json
  • .devcontainer/swift-6.2/devcontainer.json
  • .devcontainer/swift-6.3/devcontainer.json
  • .github/workflows/SyntaxKit.yml
  • .github/workflows/cleanup-caches.yml
  • .github/workflows/cleanup-pr-caches.yml
  • .github/workflows/codeql.yml
  • .gitignore
  • .mise.toml
  • .periphery.yml
  • .vscode/launch.json
  • CLAUDE.md
  • Docs/SwiftSyntax-LLM.md
  • Mintfile
  • Package.resolved
  • Package.swift
  • README.md
  • Scripts/lint.sh
  • Sources/DocumentationHarness/CodeBlock.swift
  • Sources/DocumentationHarness/CodeBlockExtraction.swift
  • Sources/DocumentationHarness/CodeBlockExtractor.swift
  • Sources/DocumentationHarness/CodeBlockExtractorError.swift
  • Sources/DocumentationHarness/CodeBlockType.swift
  • Sources/DocumentationHarness/CodeBlockValidationParameters.swift
  • Sources/DocumentationHarness/CodeSyntaxValidator.swift
  • Sources/DocumentationHarness/CompilationResult.swift
  • Sources/DocumentationHarness/DocumentationValidator.swift
  • Sources/DocumentationHarness/FileManager+Documentation.swift
  • Sources/DocumentationHarness/FileSearchError.swift
  • Sources/DocumentationHarness/FileSearcher.swift
  • Sources/DocumentationHarness/PackageValidator.swift
  • Sources/DocumentationHarness/SyntaxValidator.swift
  • Sources/DocumentationHarness/TestType.swift
  • Sources/DocumentationHarness/ValidationError.swift
  • Sources/DocumentationHarness/ValidationParameters.swift
  • Sources/DocumentationHarness/ValidationResult.swift
  • Sources/DocumentationHarness/Validator.swift
  • Sources/SyntaxKit/Attributes/Attribute.swift
  • Sources/SyntaxKit/Attributes/AttributeInfo.swift
  • Sources/SyntaxKit/Attributes/Trivia+Comments.swift
  • Sources/SyntaxKit/CodeBlocks/CodeBlock+ExprSyntax.swift
  • Sources/SyntaxKit/CodeBlocks/CodeBlock+Generate.swift
  • Sources/SyntaxKit/CodeBlocks/CodeBlockBuilder.swift
  • Sources/SyntaxKit/CodeBlocks/CodeBlockBuilderResult.swift
  • Sources/SyntaxKit/CodeBlocks/CodeBlockItemSyntax.Item.swift
  • Sources/SyntaxKit/CodeBlocks/CommentedCodeBlock.swift
  • Sources/SyntaxKit/CodeBlocks/EmptyCodeBlock.swift
  • Sources/SyntaxKit/CodeBlocks/ExprCodeBlock.swift
  • Sources/SyntaxKit/Collections/Array+LiteralValue.swift
  • Sources/SyntaxKit/Collections/ArrayLiteral.swift
  • Sources/SyntaxKit/Collections/CodeBlock+DictionaryValue.swift
  • Sources/SyntaxKit/Collections/CodeBlockableLiteral.swift
  • Sources/SyntaxKit/Collections/Dictionary+LiteralValue.swift
  • Sources/SyntaxKit/Collections/DictionaryExpr.swift
  • Sources/SyntaxKit/Collections/DictionaryLiteral.swift
  • Sources/SyntaxKit/Collections/DictionaryValue.swift
  • Sources/SyntaxKit/Collections/Literal+DictionaryValue.swift
  • Sources/SyntaxKit/Collections/PatternCodeBlock.swift
  • Sources/SyntaxKit/Collections/PatternConvertableCollection.swift
  • Sources/SyntaxKit/Collections/Tuple.swift
  • Sources/SyntaxKit/Collections/TupleAssignment+AsyncSet.swift
  • Sources/SyntaxKit/Collections/TupleAssignment.swift
  • Sources/SyntaxKit/Collections/TupleLiteralArray.swift
  • Sources/SyntaxKit/Collections/TuplePattern.swift
  • Sources/SyntaxKit/ControlFlow/Do.swift
  • Sources/SyntaxKit/ControlFlow/For.swift
  • Sources/SyntaxKit/ControlFlow/Guard.swift
  • Sources/SyntaxKit/ControlFlow/If+Body.swift
  • Sources/SyntaxKit/ControlFlow/If+CodeBlockItem.swift
  • Sources/SyntaxKit/ControlFlow/If+Conditions.swift
  • Sources/SyntaxKit/ControlFlow/If+ElseBody.swift
  • Sources/SyntaxKit/ControlFlow/If.swift
  • Sources/SyntaxKit/ControlFlow/Switch.swift
  • Sources/SyntaxKit/ControlFlow/SwitchCase.swift
  • Sources/SyntaxKit/ControlFlow/SwitchLet.swift
  • Sources/SyntaxKit/ControlFlow/While.swift
  • Sources/SyntaxKit/Core/AccessModifier.swift
  • Sources/SyntaxKit/Core/CaptureReferenceType.swift
  • Sources/SyntaxKit/Core/CodeBlock.swift
  • Sources/SyntaxKit/Core/ExprCodeBlockBuilder.swift
  • Sources/SyntaxKit/Core/Keyword+AccessModifier.swift
  • Sources/SyntaxKit/Core/Keyword+CaptureReferenceType.swift
  • Sources/SyntaxKit/Core/Line+Trivia.swift
  • Sources/SyntaxKit/Core/Line.swift
  • Sources/SyntaxKit/Core/PatternConvertible.swift
  • Sources/SyntaxKit/Core/PatternConvertibleBuilder.swift
  • Sources/SyntaxKit/Core/String+TypeRepresentable.swift
  • Sources/SyntaxKit/Core/TypeRepresentable.swift
  • Sources/SyntaxKit/Declarations/Class.swift
  • Sources/SyntaxKit/Declarations/Enum.swift
  • Sources/SyntaxKit/Declarations/Extension.swift
  • Sources/SyntaxKit/Declarations/Import.swift
  • Sources/SyntaxKit/Declarations/Init.swift
  • Sources/SyntaxKit/Declarations/Protocol.swift
  • Sources/SyntaxKit/Declarations/Struct+Public.swift
  • Sources/SyntaxKit/Declarations/Struct.swift
  • Sources/SyntaxKit/Declarations/TypeAlias.swift
  • Sources/SyntaxKit/Documentation.docc/Tutorials/Resources/QuickStartDemo.swift
  • Sources/SyntaxKit/ErrorHandling/Catch.swift
  • Sources/SyntaxKit/ErrorHandling/CatchBuilder.swift
  • Sources/SyntaxKit/ErrorHandling/Throw.swift
  • Sources/SyntaxKit/Expressions/Assignment.swift
  • Sources/SyntaxKit/Expressions/Call.swift
  • Sources/SyntaxKit/Expressions/CaptureInfo.swift
  • Sources/SyntaxKit/Expressions/Closure+Body.swift
  • Sources/SyntaxKit/Expressions/Closure+Capture.swift
  • Sources/SyntaxKit/Expressions/Closure+Signature.swift
  • Sources/SyntaxKit/Expressions/Closure.swift
  • Sources/SyntaxKit/Expressions/ClosureParameter.swift
  • Sources/SyntaxKit/Expressions/ClosureParameterBuilderResult.swift
  • Sources/SyntaxKit/Expressions/ClosureType.swift
  • Sources/SyntaxKit/Expressions/ConditionalOp.swift
  • Sources/SyntaxKit/Expressions/FunctionCallExp.swift
  • Sources/SyntaxKit/Expressions/Infix+Comparison.swift
  • Sources/SyntaxKit/Expressions/Infix.swift
  • Sources/SyntaxKit/Expressions/Literal+Convenience.swift
  • Sources/SyntaxKit/Expressions/Literal+ExprCodeBlock.swift
  • Sources/SyntaxKit/Expressions/Literal+PatternConvertible.swift
  • Sources/SyntaxKit/Expressions/Literal.swift
  • Sources/SyntaxKit/Expressions/LiteralValue.swift
  • Sources/SyntaxKit/Expressions/NegatedPropertyAccessExp.swift
  • Sources/SyntaxKit/Expressions/OptionalChainingExp.swift
  • Sources/SyntaxKit/Expressions/PlusAssign.swift
  • Sources/SyntaxKit/Expressions/PropertyAccessExp.swift
  • Sources/SyntaxKit/Expressions/PropertyAccessible.swift
  • Sources/SyntaxKit/Expressions/ReferenceExp.swift
  • Sources/SyntaxKit/Expressions/Return.swift
  • Sources/SyntaxKit/Functions/Function+EffectSpecifiers.swift
  • Sources/SyntaxKit/Functions/Function+Effects.swift
  • Sources/SyntaxKit/Functions/Function+Modifiers.swift
  • Sources/SyntaxKit/Functions/Function+Syntax.swift
  • Sources/SyntaxKit/Functions/Function.swift
  • Sources/SyntaxKit/Functions/FunctionParameterSyntax+Init.swift
  • Sources/SyntaxKit/Functions/FunctionRequirement.swift
  • Sources/SyntaxKit/Parameters/Parameter.swift
  • Sources/SyntaxKit/Parameters/ParameterBuilderResult.swift
  • Sources/SyntaxKit/Parameters/ParameterExp.swift
  • Sources/SyntaxKit/Parameters/ParameterExpBuilderResult.swift
  • Sources/SyntaxKit/Parser/String.swift
  • Sources/SyntaxKit/Parser/StructureProperty.swift
  • Sources/SyntaxKit/Parser/Token.swift
  • Sources/SyntaxKit/Parser/TokenVisitor+Helpers.swift
  • Sources/SyntaxKit/Parser/TokenVisitor.swift
  • Sources/SyntaxKit/Parser/TreeNode.swift
  • Sources/SyntaxKit/Patterns/Int+PatternConvertible.swift
  • Sources/SyntaxKit/Patterns/LetBindingPattern.swift
  • Sources/SyntaxKit/Patterns/Range+PatternConvertible.swift
  • Sources/SyntaxKit/Patterns/String+PatternConvertible.swift
  • Sources/SyntaxKit/Utilities/Break.swift
  • Sources/SyntaxKit/Utilities/Case.swift
  • Sources/SyntaxKit/Utilities/CodeBlock+Comment.swift
  • Sources/SyntaxKit/Utilities/CodeBlockable.swift
  • Sources/SyntaxKit/Utilities/CommentBuilderResult.swift
  • Sources/SyntaxKit/Utilities/Continue.swift
  • Sources/SyntaxKit/Utilities/Default.swift
  • Sources/SyntaxKit/Utilities/EnumCase+Syntax.swift
  • Sources/SyntaxKit/Utilities/EnumCase.swift
  • Sources/SyntaxKit/Utilities/Fallthrough.swift
  • Sources/SyntaxKit/Utilities/Group.swift
  • Sources/SyntaxKit/Utilities/Let.swift
  • Sources/SyntaxKit/Utilities/Parenthesized.swift
  • Sources/SyntaxKit/Utilities/PropertyRequirement.swift
  • Sources/SyntaxKit/Utilities/Then.swift
  • Sources/SyntaxKit/Variables/AttributeArguments.swift
  • Sources/SyntaxKit/Variables/ComputedProperty.swift
  • Sources/SyntaxKit/Variables/Variable+Attributes.swift
  • Sources/SyntaxKit/Variables/Variable+LiteralInitializers.swift
  • Sources/SyntaxKit/Variables/Variable+Modifiers.swift
  • Sources/SyntaxKit/Variables/Variable+TypedInitializers.swift
  • Sources/SyntaxKit/Variables/Variable.swift
  • Sources/SyntaxKit/Variables/VariableExp.swift
  • Sources/SyntaxKit/Variables/VariableKind.swift
  • Sources/SyntaxParser/README.md
  • Sources/SyntaxParser/SourceRange.swift
  • Sources/SyntaxParser/SyntaxParser.swift
  • Sources/SyntaxParser/SyntaxResponse.swift
  • Sources/SyntaxParser/TreeNode.swift
  • Sources/TokenVisitor/String.swift
  • Sources/TokenVisitor/StructureProperty.swift
  • Sources/TokenVisitor/StructureValue.swift
  • Sources/TokenVisitor/Syntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable.swift
  • Sources/TokenVisitor/SyntaxClassifiable/AttributeListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/CatchClauseListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/CodeBlockItemListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/ConditionElementListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/DeclModifierListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/DeclSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/ExprSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/FunctionParameterListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/IdentifierPatternSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/IdentifierTypeSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/LabeledExprListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/PatternBindingListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/PatternSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/SwitchCaseItemListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/SwitchCaseListSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/TypeSyntax.swift
  • Sources/TokenVisitor/SyntaxClassifiable/VariableDeclSyntax.swift
  • Sources/TokenVisitor/SyntaxType.swift
  • Sources/TokenVisitor/Token.swift
  • Sources/TokenVisitor/TokenVisitor.swift
  • Sources/TokenVisitor/TreeNodeProtocol+Extensions.swift
  • Sources/TokenVisitor/TreeNodeProtocol.swift
  • Sources/TokenVisitor/TriviaPiece.swift
  • Sources/skit/Skit.swift
  • Sources/skit/main.swift
  • Tests/SyntaxDocTests/DocumentationExampleTests.swift
  • Tests/SyntaxDocTests/Settings.swift

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch v0.0.4

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

PR Review: V0.0.4

This is a substantial refactor that extracts the parser infrastructure into dedicated targets (TokenVisitor, SyntaxParser) and upgrades tooling/CI. Overall the structure is an improvement, but there are a few issues worth addressing before merging.


🔴 High Priority

1. actions/checkout@v6 does not exist

Every job in the updated SyntaxKit.yml uses actions/checkout@v6, but the latest release is v4. This will cause all CI jobs to fail at the checkout step.

# All instances should be:
- uses: actions/checkout@v4

2. unsafeFlags applied to all library targets

The previously-commented .unsafeFlags([...]) block has been uncommented in swiftSettings, which is applied to every target including the public-facing SyntaxKit, TokenVisitor, and SyntaxParser libraries.

Per Swift Package Manager documentation, packages that use .unsafeFlags cannot be used as dependencies by other packages — SPM will refuse to resolve them. This breaks the library's core use case.

// Package.swift:63 — .unsafeFlags should be restricted to internal/test targets only
.unsafeFlags([
  "-warn-concurrency",
  "-enable-actor-data-race-checks",
  ...
])

Options:

  • Move to test targets only
  • Use supported equivalents: .enableExperimentalFeature("StrictConcurrency") / .swiftLanguageVersion(.v6) replace most of these in Swift 6
  • Remove entirely and rely on the Swift 6 language mode already enforcing strict concurrency

3. No tests for new TokenVisitor and SyntaxParser targets

The two new parsing infrastructure targets have no test coverage. SyntaxKitTests depends only on SyntaxKit; there's no test target for the new modules. Given that the skit executable has been re-wired to use these targets, at minimum a basic smoke test covering SyntaxParser.parse(code:) with a simple Swift snippet would be valuable.


🟡 Medium Priority

4. TreeNode is a mutable class — consider struct

TreeNode is declared as final class with all-var properties and implements Codable for JSON serialization. For a data-transfer type that is:

  • Built up during a traversal then read
  • Encoded to JSON immediately
  • Compared by value (the Equatable conformance compares fields, not identity)

…a struct would be more idiomatic Swift and avoids potential reference-aliasing bugs during tree construction. The one genuine need for a reference type is the mutable current pointer inside TokenVisitor, but that's already a separate internal type.


5. Deprecated API shipped alongside new API in the same module

SyntaxParser ships parse(code:options:) and SyntaxResponse marked @available(*, deprecated) in the same PR that introduces the replacement parse(code:). Since this is a package-level API (package access), there are no external callers to migrate — it's safe to just remove the deprecated path entirely rather than adding deprecation noise.


🟢 Looks Good

  • CI matrix refactor: The new configure job with dynamic matrix is a clean improvement. The concurrency group with cancel-in-progress: true is a welcome addition.
  • Conditional coverage upload (if: steps.build.outputs.contains-code-coverage == 'true') prevents false CI failures on platforms that don't support code coverage.
  • build-macos split into build-macos / build-macos-full: Good separation of fast-path vs. full platform matrix.
  • Android build job: Nice addition for broader platform support.
  • Mintfile → .mise.toml: Clean migration; mise is easier to install in CI than Mint.
  • Tool version bumps (SwiftLint 0.58→0.63, Periphery 3.0→3.7, SwiftFormat 600→602): All appropriate.
  • retain_codable_properties: true in .periphery.yml: Correct — prevents Periphery from flagging Codable backing properties as unused.
  • TreeNodeProtocol: Good abstraction that lets TokenVisitor remain generic and testable independently of the concrete TreeNode type.
  • Documentation path fix in tests ("Documentation.docc/..." vs "Sources/SyntaxKit/Documentation.docc/..."): Correct fix.

Minor Nits

  • .devcontainer/swift-6.1/devcontainer.json has a leading space before the extension ID (" swiftlang.swift-vscode") while others don't — minor inconsistency.
  • .periphery.yml and .vscode/launch.json are both missing trailing newlines.
  • TreeNodeProtocol+Extensions.swift has package import SwiftSyntaxpackage import is only available in Swift 6.0+ and applies to re-exporting. If the intent is just to use SwiftSyntax internally, a plain import SwiftSyntax is more appropriate.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.44%. Comparing base (1bd7f84) to head (5802af0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   73.67%   78.44%   +4.77%     
==========================================
  Files         136      126      -10     
  Lines        6020     4555    -1465     
==========================================
- Hits         4435     3573     -862     
+ Misses       1585      982     -603     
Flag Coverage Δ
spm 78.00% <ø> (+5.21%) ⬆️
swift- ?
swift-6.0 78.11% <ø> (?)
swift-6.1 78.04% <ø> (?)
swift-6.2 78.04% <ø> (?)
swift-6.3 78.04% <ø> (?)
ubuntu 78.28% <ø> (+4.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Comment out .unsafeFlags in Package.swift to restore SPM dependency compatibility
- Update README SPM version reference from 0.0.3 to 0.0.4
- Fix devcontainer.json extension indentation (3-space → 4-space)
- Add trailing newlines to .periphery.yml and .vscode/launch.json
- Remove .vscode from .gitignore
- Fix multiline_arguments_brackets violations in SyntaxDocTests/Settings.swift

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

PR Review: V0.0.4

This is a significant release touching CI/CD infrastructure, toolchain migration (Mint → mise), two new Swift targets (TokenVisitor, SyntaxParser), and a large documentation file. Here's my feedback:


Bugs

TokenVisitor: Missing visitPost breaks parent tracking

visitPre sets current = treeNode when entering a node, but there's no visitPost override to restore current to the parent when leaving a node. After visiting a node's first child subtree, current still points to the deepest node visited, so the second and subsequent children at the same level will get incorrect parent IDs. This will produce a malformed tree for any node with more than one child.

A visitPost that restores current to current?.parent-resolved node (or using a stack) is needed.

parseTree silently ignores showingMissingTokens parameter

In TreeNodeProtocol+Extensions.swift:

package static func parseTree(
  from sourceFile: SourceFileSyntax,
  withFileName fileName: String = .defaultFileName,
  showingMissingTokens: Bool = false   // ← received
) -> [Self] {
  let visitor = TokenVisitor<Self>(
    locationConverter: ...,
    showMissingTokens: false           // ← hardcoded, parameter ignored
  )

The showingMissingTokens argument is never forwarded. The deprecated parse(code:options:) method in SyntaxParser passes this correctly, but the new path silently drops it.


Code Quality

SyntaxRewriter used for read-only traversal

TokenVisitor extends SyntaxRewriter but never actually rewrites anything — visit(_ token:) returns the unmodified token and visitPre doesn't modify nodes. SyntaxVisitor is the correct base class for read-only traversal and avoids the overhead of rebuilding the tree on every node visit. This also removes the need for _ = visitor.rewrite(syntax).

Missing @retroactive on SwiftSyntax conformances

All the SyntaxClassifiable/ conformances add protocol conformances to types from SwiftSyntax (e.g., extension AttributeListSyntax: SyntaxClassifiable). Swift 5.7+ requires @retroactive for conformances of external types to external protocols to suppress warnings, and the project targets Swift 6.0+ with strict settings. These should be:

extension AttributeListSyntax: @retroactive SyntaxClassifiable { ... }

Deprecated constants are unreachable dead code

In SyntaxParser.swift, fold and showMissing are private static let properties marked @available(*, deprecated). Since they're private, no external caller can reference them, and the parse(code:options:) method they're used in is itself deprecated. They could be removed entirely or at least inlined.

TokenVisitor.tree visibility mismatch

TokenVisitor is internal final class but tree is declared package var. Since the class is internal, making the property package provides no benefit — it can't be accessed from outside the module any more than internal would allow.


CI/CD

actions/checkout@v6 — verify this exists

All jobs now use actions/checkout@v6. Please verify this tag is published; the widely-used version is v4. If v6 doesn't exist, all jobs will fail immediately on checkout.

Unpinned major versions for third-party actions

uses: brightdigit/swift-build@v1          # was @v1.3.0
uses: sersoft-gmbh/swift-coverage-action@v5  # was @v4
uses: codecov/codecov-action@v6              # was @v4/@v5
uses: jdx/mise-action@v4

Pinning to a major version tag (@v1) means any breaking change pushed to that tag will silently break CI. For supply chain security, prefer SHA-pinned or minor-version-pinned references (e.g., @v1.3.0). This is especially important for third-party actions with write permissions.

codeql.yml trigger inconsistency

The CodeQL workflow still uses branches-ignore: '*WIP' while the main workflow now uses an allowlist (branches: [main] + tags:). These should be consistent to avoid analyzing branches that aren't built.

Android build matrix is very wide

Building for swift 6.2 + 6.3 × API levels 33/34/35/36 = 8 jobs per full-matrix run, each requiring disk-space cleanup. This will significantly increase CI time and cost. Consider whether all four API levels need testing on every PR, or if a subset suffices.


Performance / Size

Docs/SwiftSyntax-LLM.md is 27,825 lines (~590KB)

Committing a 590KB generated reference document directly into the repo will bloat git clone sizes permanently (it stays in history even if later deleted). Consider:

  • Git LFS for large generated docs
  • Keeping it in .gitignore and generating it as a CI artifact
  • Linking to the upstream SwiftSyntax documentation instead

Minor

  • .gitignore now tracks .vscode/ — this is intentional given the updated launch.json, but worth noting for contributors who have local VS Code settings they don't want committed.
  • Tool version bumps in .mise.toml (SwiftLint 0.63.2, Periphery 3.7.2, SwiftFormat 602.0.0) look reasonable; CLAUDE.md is updated to match.
  • The retain_codable_properties: true addition to .periphery.yml is a good fix to avoid false positives on Codable types.
  • Copyright year update to 2026 across all files is correct.

Summary: The CI restructuring and mise migration are solid improvements, but there are two bugs in the new TokenVisitor/SyntaxParser targets that need fixing before merge: the missing visitPost causing incorrect parent IDs, and the silently ignored showingMissingTokens parameter. The retroactive conformance warnings will also likely fail the lint job under Swift 6 strict mode.

@leogdion
Copy link
Copy Markdown
Member Author

leogdion commented Apr 6, 2026

PR Review: V0.0.4

This is a significant release touching CI/CD infrastructure, toolchain migration (Mint → mise), two new Swift targets (TokenVisitor, SyntaxParser), and a large documentation file. Here's my feedback:

Bugs

TokenVisitor: Missing visitPost breaks parent tracking

visitPre sets current = treeNode when entering a node, but there's no visitPost override to restore current to the parent when leaving a node. After visiting a node's first child subtree, current still points to the deepest node visited, so the second and subsequent children at the same level will get incorrect parent IDs. This will produce a malformed tree for any node with more than one child.

A visitPost that restores current to current?.parent-resolved node (or using a stack) is needed.

parseTree silently ignores showingMissingTokens parameter

In TreeNodeProtocol+Extensions.swift:

package static func parseTree(
  from sourceFile: SourceFileSyntax,
  withFileName fileName: String = .defaultFileName,
  showingMissingTokens: Bool = false   // ← received
) -> [Self] {
  let visitor = TokenVisitor<Self>(
    locationConverter: ...,
    showMissingTokens: false           // ← hardcoded, parameter ignored
  )

The showingMissingTokens argument is never forwarded. The deprecated parse(code:options:) method in SyntaxParser passes this correctly, but the new path silently drops it.

Code Quality

SyntaxRewriter used for read-only traversal

TokenVisitor extends SyntaxRewriter but never actually rewrites anything — visit(_ token:) returns the unmodified token and visitPre doesn't modify nodes. SyntaxVisitor is the correct base class for read-only traversal and avoids the overhead of rebuilding the tree on every node visit. This also removes the need for _ = visitor.rewrite(syntax).

Missing @retroactive on SwiftSyntax conformances

All the SyntaxClassifiable/ conformances add protocol conformances to types from SwiftSyntax (e.g., extension AttributeListSyntax: SyntaxClassifiable). Swift 5.7+ requires @retroactive for conformances of external types to external protocols to suppress warnings, and the project targets Swift 6.0+ with strict settings. These should be:

extension AttributeListSyntax: @retroactive SyntaxClassifiable { ... }

Deprecated constants are unreachable dead code

In SyntaxParser.swift, fold and showMissing are private static let properties marked @available(*, deprecated). Since they're private, no external caller can reference them, and the parse(code:options:) method they're used in is itself deprecated. They could be removed entirely or at least inlined.

TokenVisitor.tree visibility mismatch

TokenVisitor is internal final class but tree is declared package var. Since the class is internal, making the property package provides no benefit — it can't be accessed from outside the module any more than internal would allow.

CI/CD

actions/checkout@v6 — verify this exists

All jobs now use actions/checkout@v6. Please verify this tag is published; the widely-used version is v4. If v6 doesn't exist, all jobs will fail immediately on checkout.

Unpinned major versions for third-party actions

uses: brightdigit/swift-build@v1          # was @v1.3.0
uses: sersoft-gmbh/swift-coverage-action@v5  # was @v4
uses: codecov/codecov-action@v6              # was @v4/@v5
uses: jdx/mise-action@v4

Pinning to a major version tag (@v1) means any breaking change pushed to that tag will silently break CI. For supply chain security, prefer SHA-pinned or minor-version-pinned references (e.g., @v1.3.0). This is especially important for third-party actions with write permissions.

codeql.yml trigger inconsistency

The CodeQL workflow still uses branches-ignore: '*WIP' while the main workflow now uses an allowlist (branches: [main] + tags:). These should be consistent to avoid analyzing branches that aren't built.

Android build matrix is very wide

Building for swift 6.2 + 6.3 × API levels 33/34/35/36 = 8 jobs per full-matrix run, each requiring disk-space cleanup. This will significantly increase CI time and cost. Consider whether all four API levels need testing on every PR, or if a subset suffices.

Performance / Size

Docs/SwiftSyntax-LLM.md is 27,825 lines (~590KB)

Committing a 590KB generated reference document directly into the repo will bloat git clone sizes permanently (it stays in history even if later deleted). Consider:

  • Git LFS for large generated docs
  • Keeping it in .gitignore and generating it as a CI artifact
  • Linking to the upstream SwiftSyntax documentation instead

Minor

  • .gitignore now tracks .vscode/ — this is intentional given the updated launch.json, but worth noting for contributors who have local VS Code settings they don't want committed.
  • Tool version bumps in .mise.toml (SwiftLint 0.63.2, Periphery 3.7.2, SwiftFormat 602.0.0) look reasonable; CLAUDE.md is updated to match.
  • The retain_codable_properties: true addition to .periphery.yml is a good fix to avoid false positives on Codable types.
  • Copyright year update to 2026 across all files is correct.

Summary: The CI restructuring and mise migration are solid improvements, but there are two bugs in the new TokenVisitor/SyntaxParser targets that need fixing before merge: the missing visitPost causing incorrect parent IDs, and the silently ignored showingMissingTokens parameter. The retroactive conformance warnings will also likely fail the lint job under Swift 6 strict mode.

PR Review: V0.0.4

This is a significant release touching CI/CD infrastructure, toolchain migration (Mint → mise), two new Swift targets (TokenVisitor, SyntaxParser), and a large documentation file. Here's my feedback:

Bugs

TokenVisitor: Missing visitPost breaks parent tracking

visitPre sets current = treeNode when entering a node, but there's no visitPost override to restore current to the parent when leaving a node. After visiting a node's first child subtree, current still points to the deepest node visited, so the second and subsequent children at the same level will get incorrect parent IDs. This will produce a malformed tree for any node with more than one child.

A visitPost that restores current to current?.parent-resolved node (or using a stack) is needed.

parseTree silently ignores showingMissingTokens parameter

In TreeNodeProtocol+Extensions.swift:

package static func parseTree(
  from sourceFile: SourceFileSyntax,
  withFileName fileName: String = .defaultFileName,
  showingMissingTokens: Bool = false   // ← received
) -> [Self] {
  let visitor = TokenVisitor<Self>(
    locationConverter: ...,
    showMissingTokens: false           // ← hardcoded, parameter ignored
  )

The showingMissingTokens argument is never forwarded. The deprecated parse(code:options:) method in SyntaxParser passes this correctly, but the new path silently drops it.

Code Quality

SyntaxRewriter used for read-only traversal

TokenVisitor extends SyntaxRewriter but never actually rewrites anything — visit(_ token:) returns the unmodified token and visitPre doesn't modify nodes. SyntaxVisitor is the correct base class for read-only traversal and avoids the overhead of rebuilding the tree on every node visit. This also removes the need for _ = visitor.rewrite(syntax).

Missing @retroactive on SwiftSyntax conformances

All the SyntaxClassifiable/ conformances add protocol conformances to types from SwiftSyntax (e.g., extension AttributeListSyntax: SyntaxClassifiable). Swift 5.7+ requires @retroactive for conformances of external types to external protocols to suppress warnings, and the project targets Swift 6.0+ with strict settings. These should be:

extension AttributeListSyntax: @retroactive SyntaxClassifiable { ... }

Deprecated constants are unreachable dead code

In SyntaxParser.swift, fold and showMissing are private static let properties marked @available(*, deprecated). Since they're private, no external caller can reference them, and the parse(code:options:) method they're used in is itself deprecated. They could be removed entirely or at least inlined.

TokenVisitor.tree visibility mismatch

TokenVisitor is internal final class but tree is declared package var. Since the class is internal, making the property package provides no benefit — it can't be accessed from outside the module any more than internal would allow.

CI/CD

actions/checkout@v6 — verify this exists

All jobs now use actions/checkout@v6. Please verify this tag is published; the widely-used version is v4. If v6 doesn't exist, all jobs will fail immediately on checkout.

Unpinned major versions for third-party actions

uses: brightdigit/swift-build@v1          # was @v1.3.0
uses: sersoft-gmbh/swift-coverage-action@v5  # was @v4
uses: codecov/codecov-action@v6              # was @v4/@v5
uses: jdx/mise-action@v4

Pinning to a major version tag (@v1) means any breaking change pushed to that tag will silently break CI. For supply chain security, prefer SHA-pinned or minor-version-pinned references (e.g., @v1.3.0). This is especially important for third-party actions with write permissions.

codeql.yml trigger inconsistency

The CodeQL workflow still uses branches-ignore: '*WIP' while the main workflow now uses an allowlist (branches: [main] + tags:). These should be consistent to avoid analyzing branches that aren't built.

Android build matrix is very wide

Building for swift 6.2 + 6.3 × API levels 33/34/35/36 = 8 jobs per full-matrix run, each requiring disk-space cleanup. This will significantly increase CI time and cost. Consider whether all four API levels need testing on every PR, or if a subset suffices.

Performance / Size

Docs/SwiftSyntax-LLM.md is 27,825 lines (~590KB)

Committing a 590KB generated reference document directly into the repo will bloat git clone sizes permanently (it stays in history even if later deleted). Consider:

  • Git LFS for large generated docs
  • Keeping it in .gitignore and generating it as a CI artifact
  • Linking to the upstream SwiftSyntax documentation instead

Minor

  • .gitignore now tracks .vscode/ — this is intentional given the updated launch.json, but worth noting for contributors who have local VS Code settings they don't want committed.
  • Tool version bumps in .mise.toml (SwiftLint 0.63.2, Periphery 3.7.2, SwiftFormat 602.0.0) look reasonable; CLAUDE.md is updated to match.
  • The retain_codable_properties: true addition to .periphery.yml is a good fix to avoid false positives on Codable types.
  • Copyright year update to 2026 across all files is correct.

Summary: The CI restructuring and mise migration are solid improvements, but there are two bugs in the new TokenVisitor/SyntaxParser targets that need fixing before merge: the missing visitPost causing incorrect parent IDs, and the silently ignored showingMissingTokens parameter. The retroactive conformance warnings will also likely fail the lint job under Swift 6 strict mode.

@claude create a GitHub issue for the bug

@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

Claude finished @leogdion's task in 1m 1s —— View job


Creating GitHub Issue for Bugs

  • Analyze bugs from PR review
  • Attempted to create GitHub issue — blocked by tool permissions

I wasn't able to create the GitHub issue automatically because the gh issue create command requires explicit tool approval in this environment.

Here is the issue content ready to be created manually (or you can re-run with the gh CLI tool allowed):


Title: Bug: TokenVisitor missing visitPost breaks parent tracking + showingMissingTokens ignored

Body:

Two bugs were identified in the new TokenVisitor/SyntaxParser targets introduced in PR #147 (v0.0.4).

Bug 1: TokenVisitor missing visitPost breaks parent tracking

visitPre sets current = treeNode when entering a node, but there is no visitPost override to restore current to the parent node when leaving. After visiting a node's first child subtree, current still points to the deepest node visited, so subsequent children at the same level will get incorrect parent IDs. This produces a malformed tree for any syntax node with more than one child.

Fix: Add a visitPost override that restores current to its parent (using a parent reference or a stack):

override func visitPost(_ node: Syntax) {
    current = current?.parentNode  // or pop from a stack
}

Bug 2: parseTree silently ignores showingMissingTokens parameter

In TreeNodeProtocol+Extensions.swift, parseTree(from:withFileName:showingMissingTokens:) hardcodes showMissingTokens: false when constructing TokenVisitor, silently discarding the caller's value.

Fix: Forward the parameter:

let visitor = TokenVisitor<Self>(
    locationConverter: ...,
    showMissingTokens: showingMissingTokens  // was hardcoded false
)

To create the issue, run:

gh issue create --repo brightdigit/SyntaxKit \
  --title "Bug: TokenVisitor missing visitPost breaks parent tracking + showingMissingTokens ignored" \
  --label bug

@leogdion leogdion marked this pull request as ready for review April 6, 2026 18:18
@leogdion leogdion merged commit 475ae7c into main Apr 6, 2026
45 checks passed
@leogdion leogdion deleted the v0.0.4 branch April 6, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant