fix(lsp): expression binding diagnostics and completion filtering#413
Open
bennypowers wants to merge 4 commits into
Open
fix(lsp): expression binding diagnostics and completion filtering#413bennypowers wants to merge 4 commits into
bennypowers wants to merge 4 commits into
Conversation
parseUnionType now only suggests quoted string literals and boolean literals. Bare identifiers (unresolved type aliases like NavigationPrimaryPalette), null, undefined, and primitives are silently dropped. Silence is better DX than suggesting type names as attribute values. Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add ExpressionKind and ExpressionDetail fields to AttributeMatch.
When a TS template attribute value contains ${...}, the TS tree is
walked to find the matching template_substitution node and classify
the expression:
- literal: string/number/boolean literals (detail = value)
- this-member: this.propertyName (detail = member name)
- identifier: bare const/variable reference (detail = identifier)
- complex: anything else (binary, ternary, call, etc.)
Classification happens in the document layer where tree-sitter
access is available. Diagnostics layer (next step) will consume
ExpressionKind/Detail for type-aware validation.
Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r type dispatch
Attribute value diagnostics:
- Skip validation for . (property), @ (event), ? (boolean) bindings
- Skip validation when ExpressionKind is set (${...} expressions)
- Dispatch on tree-sitter grammar name (predefined_type, union_type,
literal_type, array_type, generic_type) instead of string matching
Attribute name diagnostics:
- Strip Lit binding prefix (./?/@) before manifest lookup
- Skip unknown-attribute diagnostic for @ event bindings
Regression test verifies plain HTML attributes still validated.
Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace fragile grammar-name allowlist in findSubstitutionByContent
with byte-range pruning. The old approach skipped nodes by checking
grammar name against a hardcoded list (program, template_string,
call_expression), missing intermediate nodes like method_definition
or class_body that sit between the root and the template literal.
Also remove string-based ${...} boundary parsing -- the TS tree
node's expression child has the correct content directly, avoiding
issues with nested braces in expression values.
Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes add template-expression metadata to attribute matches, make attribute diagnostics binding- and grammar-aware, and restrict union completions to quoted string and boolean literals. Tests cover completion filtering and continued validation of plain HTML attribute values. ChangesAttribute analysis and completion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant findCustomElements
participant parseHTMLInTemplate
participant collectTemplateAttributes
participant TypeScriptSyntaxTree
findCustomElements->>parseHTMLInTemplate: pass tsRoot
parseHTMLInTemplate->>collectTemplateAttributes: collect template attributes
collectTemplateAttributes->>TypeScriptSyntaxTree: locate template_substitution
TypeScriptSyntaxTree-->>collectTemplateAttributes: return expression classification
collectTemplateAttributes-->>findCustomElements: return AttributeMatch metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Build Artifacts
Built from 8787335 @ fix/lsp/expression-binding-diagnostics |
LSP Benchmark Results
View this benchmark run in GitHub Actions 💡 Tip: Raw JSON results are available in workflow artifacts if needed. Generate Benchmarks
Perf/kb delta ratio: 0.97x 🐢 View this benchmark run in GitHub Actions 💡 Tip: Raw JSON outputs are available in workflow artifacts if needed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lsp/methods/textDocument/completion/completion_type_parsing_test.go (1)
26-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test cases for boolean literals and double-quoted literals.
The new
parseUnionTypeexplicitly handlestrue/false(line 467 ofcompletion.go) andisQuotedLiteralsupports both single and double quotes, but neither path is exercised by the current test cases. All literal cases use single quotes only, and no union containing boolean literals is tested.🧪 Suggested additional test cases
{ "only identifiers", "SomeType | AnotherType", nil, 0, "Pure unresolved identifiers should produce no completions", }, + { + "boolean literals in union", + "true | false", + []string{"true", "false"}, + 2, + "Boolean literals should be suggested as completion values", + }, + { + "double-quoted literals", + `"red" | "green"`, + []string{"red", "green"}, + 2, + "Double-quoted string literals should be suggested", + }, { "primitive string",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lsp/methods/textDocument/completion/completion_type_parsing_test.go` around lines 26 - 103, Extend TestTypeBasedCompletions_FiltersUnresolvedIdentifiers with table cases covering true/false boolean literals and double-quoted literals, including a union combining them where appropriate. Assert the expected labels and completion counts so parseUnionType and isQuotedLiteral exercise both supported paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lsp/document/typescript/document.go`:
- Around line 507-524: classifyExpression currently searches from
template.startByte with brittle +100 padding, causing later attributes to
resolve the wrong substitution. Anchor findFirstSubstitution to the specific
attribute value’s offset/range from match, derive the search end from that value
boundary, and preserve correct ExpressionKind and ExpressionDetail
classification for multi-expression templates.
---
Nitpick comments:
In `@lsp/methods/textDocument/completion/completion_type_parsing_test.go`:
- Around line 26-103: Extend
TestTypeBasedCompletions_FiltersUnresolvedIdentifiers with table cases covering
true/false boolean literals and double-quoted literals, including a union
combining them where appropriate. Assert the expected labels and completion
counts so parseUnionType and isQuotedLiteral exercise both supported paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dff64c24-ed38-4ed0-ac5a-7166312ab8e4
📒 Files selected for processing (7)
lsp/document/typescript/document.golsp/methods/textDocument/completion/completion.golsp/methods/textDocument/completion/completion_type_parsing_test.golsp/methods/textDocument/publishDiagnostics/attributeDiagnostics.golsp/methods/textDocument/publishDiagnostics/attributeValueDiagnostics.golsp/methods/textDocument/publishDiagnostics/attributeValueDiagnostics_test.golsp/types/document.go
Comment on lines
+507
to
+524
| func classifyExpression(match *types.AttributeMatch, template TemplateContext, tsRoot *ts.Node, docContent []byte) { | ||
| templateEnd := template.startByte + uint(len(match.Value)+100) | ||
| sub := findFirstSubstitution(tsRoot, template.startByte, templateEnd) | ||
| if sub == nil { | ||
| match.ExpressionKind = "complex" | ||
| return | ||
| } | ||
|
|
||
| cursor := sub.Walk() | ||
| defer cursor.Close() | ||
| children := sub.NamedChildren(cursor) | ||
| if len(children) == 0 { | ||
| match.ExpressionKind = "complex" | ||
| return | ||
| } | ||
|
|
||
| classifyExpressionNode(&children[0], match, docContent) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'findFirstSubstitution|templateEnd|classifyExpression' lsp/document/typescript/document.go
fd -e go typescript | xargs rg -nl 'classifyExpression|ExpressionKind'Repository: bennypowers/cem
Length of output: 2265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,590p' lsp/document/typescript/document.go
printf '\n---\n'
sed -n '1,220p' lsp/document/typescript/types.go
printf '\n---\n'
rg -n 'type AttributeMatch|ExpressionKind|ExpressionDetail|TemplateContext|closestValue|template.startByte|startByte' lsp/document/typescript -g '*.go'Repository: bennypowers/cem
Length of output: 9423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,380p' lsp/document/typescript/document.go
printf '\n---\n'
sed -n '380,450p' lsp/document/typescript/document.go
printf '\n---\n'
rg -n 'template_substitution|ExpressionKind|classifyExpression\(' lsp -g '*.go'Repository: bennypowers/cem
Length of output: 7368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' lsp/types/document.go
printf '\n---\n'
rg -n 'type AttributeMatch|ValueRange|ExpressionKind|ExpressionDetail' lsp/types -g '*.go'Repository: bennypowers/cem
Length of output: 7888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' lsp/methods/textDocument/publishDiagnostics/attributeValueDiagnostics.go
printf '\n---\n'
sed -n '1,180p' lsp/methods/textDocument/publishDiagnostics/attributeDiagnostics.goRepository: bennypowers/cem
Length of output: 16554
Anchor classification to the attribute value, not the template start classifyExpression always starts at template.startByte, so every ${...} in the same template is resolved against the first matching substitution. That can misclassify later attributes in multi-expression templates and leave ExpressionKind/ExpressionDetail wrong. Use the attribute’s own value offset/range instead of the template origin; the +100 padding is also brittle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lsp/document/typescript/document.go` around lines 507 - 524,
classifyExpression currently searches from template.startByte with brittle +100
padding, causing later attributes to resolve the wrong substitution. Anchor
findFirstSubstitution to the specific attribute value’s offset/range from match,
derive the search end from that value boundary, and preserve correct
ExpressionKind and ExpressionDetail classification for multi-expression
templates.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
${expr}) in the TS document layer.prop,@event,?bool)validateAttributeValueinstead of string matchingDetails
Completion filtering:
parseUnionTypenow only suggests quoted string literalsand boolean literals. Bare identifiers like
NavigationPrimaryPalette(unresolvedtype aliases),
null,undefined, and primitives are silently dropped.Expression classification: when a TS template attribute value contains
${...},the TS tree is walked to find the matching
template_substitutionnode and classifythe expression as literal, this-member, identifier, or complex. Classification
happens in the document layer; diagnostics layer consumes it.
Diagnostics: property bindings (
.prop) pass JS values not HTML strings, eventbindings (
@event) are function handlers, boolean bindings (?attr) controlpresence via expression truthiness. All skip value validation. Expression-valued
attributes (
ExpressionKind != "") also skip validation since we cannot type-checkthe expression without a full type checker.
Attribute name diagnostics now strip
./?/@prefixes before manifest lookupand skip unknown-attribute diagnostics for
@event bindings.Test plan
make lintcleanSummary by CodeRabbit
New Features
Bug Fixes
null, andundefined.