T-006: FLUX Language Server Protocol implementation#5
T-006: FLUX Language Server Protocol implementation#5SuperInstance wants to merge 3 commits intomainfrom
Conversation
![beta-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/2443040?s=60&v=4){kind=small}
| tokens.push( | ||
| line.lineNumber - prevLine, | ||
| col - prevChar, | ||
| line.mnemonic.length, | ||
| SemanticTokenType.mnemonic, | ||
| 0, | ||
| ); | ||
| prevLine = line.lineNumber; | ||
| prevChar = col; |
There was a problem hiding this comment.
🔴 Semantic tokens deltaStartChar is relative instead of absolute when crossing lines
In onSemanticTokens, the deltaStartChar is always computed as col - prevChar. According to the LSP semantic tokens spec, when deltaLine > 0 (i.e., the token is on a different line than the previous one), deltaStartChar must be the absolute start character from the beginning of the line, not relative to the previous token. The code never resets prevChar when crossing lines, producing negative or incorrect delta values.
Trace showing the bug with a two-line program
For ADD R0, R1, R2\nHALT:
- Last token on line 0: R2 at col 12 → prevChar=12
- First token on line 1: HALT at col 0 → deltaLine=1, deltaStartChar = 0 - 12 = -12 (should be 0)
| tokens.push( | |
| line.lineNumber - prevLine, | |
| col - prevChar, | |
| line.mnemonic.length, | |
| SemanticTokenType.mnemonic, | |
| 0, | |
| ); | |
| prevLine = line.lineNumber; | |
| prevChar = col; | |
| const col = line.mnemonicRange?.start.character ?? 0; | |
| const deltaLine = line.lineNumber - prevLine; | |
| tokens.push( | |
| deltaLine, | |
| deltaLine > 0 ? col : col - prevChar, | |
| line.mnemonic.length, | |
| SemanticTokenType.mnemonic, | |
| 0, | |
| ); | |
| prevLine = line.lineNumber; | |
| prevChar = col; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| insertText: op.operands.length === 0 | ||
| ? op.mnemonic | ||
| : `${op.mnemonic} ${op.operands.filter(o => o.role !== '-').map(o => o.role === 'imm8' ? '${1:0}' : o.role === 'imm16' ? '${1:0}' : `$\{${op.role}\``).join(', ')}$0`, | ||
| : `${op.mnemonic} ${op.operands.filter(o => o.role !== '-').map(o => o.role === 'imm8' ? '${1:0}' : o.role === 'imm16' ? '${1:0}' : `$\{${o.role}\``).join(', ')}$0`, |
There was a problem hiding this comment.
🔴 Snippet generation produces malformed VS Code snippet text (backtick instead of closing brace)
The inline snippet generation in getOpcodeCompletionItems uses the template literal `$\{${o.role}\ for register operands, which produces `${rd\ (dollar-brace-role-backtick) instead of the correct VS Code snippet syntax ${1:rd}. For example, ADD produces ADD ${rd\, ${rs1`, ${rs2`$0instead ofADD ${1:rd}, ${2:rs1}, ${3:rs2}$0. The function buildOpcodeSnippetatsrc/opcode-database.ts:406-425` generates correct snippets and was used before this change but is now unused dead code. This breaks snippet insertion for all ~200+ opcodes with register operands.
Comparison of old vs new output
Correct (old buildOpcodeSnippet): ADD ${1:rd}, ${2:rs1}, ${3:rs2}$0
Broken (new inline): ADD ${rd\, ${rs1`, ${rs2`$0`
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
- Fix TypeScript compilation errors (Connection.log, CompletionList import, foldingRanges API, type narrowing issues) - Add Find References feature for label definitions and usages - Add comprehensive test suite (104 tests covering parser, diagnostics, opcode database, and server integration) - Add GitHub CI workflow with multi-node matrix - Add .gitignore and jest.config.js - Update capabilities: referencesProvider enabled, fix foldingRange API
- Add .fluxasm file extension support - Add semantic tokens provider for enhanced syntax highlighting (mnemonics, labels, registers, immediates, label references) - Add signature help provider for opcode operand hints - Add workspace symbol search across opcodes, labels, and sections - Add rename support for labels (prepareRename + onRename) - Add duplicate label detection in diagnostics - Add isFluxAsmUri/isFluxMdUri utility methods - Update MockConnection in tests for new handlers - 32 new tests (248 total passing)
SuperInstance
force-pushed
the
superz/T-006
branch
from
60689ae to
185ba00
Compare
|
Closing: superseded by merged work on main. The changes from this PR have been incorporated through other merged PRs. Thank you for the contribution! 🙏 |
| // Pass 3: duplicate label definitions | ||
| const labelDefs: Map<string, number> = new Map(); | ||
| for (const line of lines) { | ||
| if (line.label && (line.type === 'label' || line.type === 'opcode')) { | ||
| if (labelDefs.has(line.label)) { | ||
| diagnostics.push(makeDiagnostic( | ||
| rangeForLine(defLines[0]), | ||
| `Label '@${name}' is defined but never referenced`, | ||
| DiagnosticSeverity.Hint, | ||
| 'flux-unused-label', | ||
| rangeForLine(line.lineNumber), | ||
| `Duplicate label '@${line.label}' (previously defined at line ${labelDefs.get(line.label)! + 1})`, | ||
| DiagnosticSeverity.Warning, | ||
| 'flux-duplicate-label', | ||
| )); | ||
| } else { | ||
| labelDefs.set(line.label, line.lineNumber); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Duplicate label detection runs twice (Pass 0 and Pass 3), producing redundant diagnostics with conflicting severity
Pass 0 (lines 36-56) reports DiagnosticSeverity.Error for ALL occurrences of a duplicate label, and the newly added Pass 3 (lines 191-206) reports DiagnosticSeverity.Warning for the second occurrence. For input @loop:\n NOP\n@loop:\n NOP, this produces 3 diagnostics with code flux-duplicate-label (2 Errors from Pass 0 + 1 Warning from Pass 3) instead of the intended 1 Warning. The test in src/__tests__/new-features.test.ts:307 expects exactly 1 diagnostic, confirming the author intended only Pass 3 to handle duplicates. Pass 0 should be removed or Pass 3 should be removed to avoid the conflict.
Prompt for agents
In src/diagnostics.ts, the provideDiagnostics function has two duplicate-label detection passes that conflict:
Pass 0 (lines 36-56) detects duplicates and emits DiagnosticSeverity.Error for ALL occurrences of each duplicate label.
Pass 3 (lines 191-206) also detects duplicates and emits DiagnosticSeverity.Warning for the second occurrence, with a message that includes the first definition's line number.
These produce redundant diagnostics (3 instead of 1 for a pair of duplicate labels). The new test in new-features.test.ts:307 expects only 1 diagnostic with code flux-duplicate-label, confirming the intended behavior is Pass 3 only.
Fix: Remove the Pass 0 duplicate detection block (lines 36-56, the labelDefinitionCounts logic) and keep only Pass 3, which provides better messages with the prior definition's line number. Alternatively, remove Pass 3 and keep Pass 0 if the Error severity is preferred, but adjust the test accordingly.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
1 participant
Summary
Comprehensive LSP implementation for FLUX .flux/.fluxasm/.flux.md assembly files with full IDE support.
Changes
New Features
Files Modified (7)
Test Results
Verification