-
Notifications
You must be signed in to change notification settings - Fork 121
Fix inline text insertion handling in semantic token delta calculation #3707
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
Fix inline text insertion handling in semantic token delta calculation #3707
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
When text is inserted on the same line without line breaks (lineOffset=0), the boundary token may have a different deltaStart value while keeping the same deltaLine. The algorithm now accounts for this case by allowing deltaStart to differ for the boundary token when lineOffset=0. Addresses comment from @nixel2007 about text insertion without line breaks. Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
...test/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProviderTest.java
Outdated
Show resolved
Hide resolved
|
@coderabbitai в оригинальном pr ты говорил, что проблемы нет. А здесь проблема нашлась. Перепроверь, кто прав? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1859 🏁 Script executed: Length of output: 4314 🏁 Script executed: Length of output: 27649 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 6026 🏁 Script executed: Length of output: 9638 @nixel2007, вы правы — я ошибался в PR #3706. Проблема, которую я не увидел: В оригинальном PR #3706 я неправильно утверждал, что алгоритм корректно обрабатывает вставку текста без перевода строки. На самом деле, при вставке текста на той же строке (например, "Перем Новая, А;" вместо "Перем А;"):
Старый алгоритм в Исправление в PR #3707: Теперь при
Ваш вопрос в оригинальном PR был абсолютно оправдан, и обнаруженная проблема реальна. Приношу извинения за первоначальный неверный анализ. __( o)> ✏️ Learnings added
|
Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
- Added DELTA_LINE_INDEX and DELTA_START_INDEX constants for clarity - Simplified redundant condition check - Improved test assertion to compare against new token size Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
…test Added detailed assertions on decoded token fields (line, start, length, type, modifiers) for both original and modified token arrays to verify the exact token structure. Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
...test/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProviderTest.java
Show resolved
Hide resolved
Added specific assertions on delta edit fields: - start position (5 - after prefix match) - deleteCount (0 - no deletion needed due to suffix match) - data size (10 integers = 2 tokens to insert) The test now verifies that with lineOffset=0, the suffix matching correctly identifies "А" and ";" tokens despite different deltaStart positions. Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
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.
Pull request overview
This PR addresses inline text insertion handling in semantic token delta calculations, specifically for cases where text is inserted on the same line without line breaks (lineOffset = 0). The changes modify the findSuffixMatchWithOffset algorithm to detect when tokens have shifted deltaStart values while maintaining the same deltaLine.
Key Changes:
- Modified the suffix matching algorithm to handle inline text insertions where
lineOffset == 0anddeltaStartvalues differ - Added comprehensive test case with detailed token and delta edit assertions
- Introduced named constants for improved code readability
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java |
Updated findSuffixMatchWithOffset method to handle inline edits by conditionally skipping deltaStart checks and tracking boundary tokens |
src/test/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProviderTest.java |
Added test case deltaWithTextInsertedOnSameLine_shouldReturnOptimalDelta with detailed assertions for decoded tokens and delta edits |
| // Для граничного токена при inline-редактировании (lineOffset == 0) | ||
| // разрешаем различие в deltaStart | ||
| int firstFieldToCheck = (!foundBoundary && lineOffset == 0) ? DELTA_START_INDEX + 1 : DELTA_START_INDEX; | ||
|
|
||
| // Проверяем поля кроме deltaLine (и возможно deltaStart для граничного токена) | ||
| boolean otherFieldsMatch = true; | ||
| for (int j = 1; j < tokenSize; j++) { | ||
| for (int j = firstFieldToCheck; j < tokenSize; j++) { |
Copilot
AI
Dec 28, 2025
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.
There appears to be a logical inconsistency between the algorithm implementation and the test expectations.
The test expects that when inserting text inline (lineOffset=0), BOTH the "А" token and the ";" token should be part of the suffix match, even though "А" has a different deltaStart value (line 1401-1402 comment states "suffix match includes А and ;").
However, the algorithm only allows ONE boundary token. After finding the first token with a different deltaStart (the ";" token), it sets foundBoundary = true. Then for the next token ("А"), it sets firstFieldToCheck = DELTA_START_INDEX (line 320), which means it WILL check deltaStart. Since "А" also has a different deltaStart, the check at line 325 will fail, breaking the loop.
This means the algorithm would find suffixMatch = 1 (only ";"), but the test expects suffixMatch = 2 (both "А" and ";"), resulting in deleteCount = 0.
Either:
- The algorithm needs to be fixed to allow ALL tokens with matching deltaLine when lineOffset=0 (not just one boundary token), OR
- The test expectations are incorrect and should expect deleteCount > 0
Please verify which behavior is intended and correct either the implementation or the test.
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.
@copilot check the comment
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.
Проверил алгоритм - он работает корректно. Анализ бота неверен.
Фактическое поведение:
- i=0: ";" токены: deltaStart одинаковы (1==1), НЕ граничный токен, suffixMatch=1
- i=1: "А" токены: deltaStart различны (6≠2), ЭТО граничный токен, suffixMatch=2, foundBoundary=true
- i=2: цикл прерывается из-за foundBoundary=true
Алгоритм находит suffixMatch=2 ("А" и ";"), что соответствует ожиданиям теста (deleteCount=0, insert 10 integers). Тест проходит успешно.
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java
Show resolved
Hide resolved
9ee333e
into
feature/optimize-delta-semantic-tokens
findSuffixMatchWithOffsetSummary
Successfully addressed @nixel2007's feedback about text insertion without line breaks. The
findSuffixMatchWithOffsetalgorithm now correctly handles inline text insertions (lineOffset=0) by allowing the boundary token to have a differentdeltaStartvalue while maintaining the samedeltaLine,length,type, andmodifiers.Changes:
SemanticTokensProvider.findSuffixMatchWithOffset()to handle inline insertionsdeltaWithTextInsertedOnSameLine_shouldReturnOptimalDelta()with comprehensive assertions:💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.