feat: new diagnostic SuspiciousChangeAndValidate - #4398
Conversation
Detects methods annotated with &ChangeAndValidate that use both #Delete and #Insert directives for full method body replacement, which hides actual changes from reviewers. For full replacement, &Instead should be used instead. - Diagnostic type: CODE_SMELL, severity: MAJOR, minutesToFix: 5 - Checks methods with CHANGEANDVALIDATE annotation for full replacement pattern: #Delete near method start + #EndInsert near end - Does not trigger on targeted modifications (delete not at start) - RU/EN messages, docs, and test fixture included
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds the ChangesSuspicious Change and Validate diagnostic
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnosticTest.java (1)
38-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the test by its behavior and add edge cases.
Rename
test()to describe full-replacement detection. Add cases for incomplete directive pairs and complete pairs that preserve original code between the blocks. Assert that only the true full replacement reports a diagnostic.As per coding guidelines, diagnostic tests must use meaningful method names and cover edge cases.
🤖 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 `@src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnosticTest.java` around lines 38 - 49, Rename the test method to describe detection of a full replacement in SuspiciousChangeAndValidateDiagnosticTest. Extend the fixture with cases for incomplete directive pairs and complete directive pairs that retain original code between blocks, then assert that only the true full-replacement case produces a diagnostic.Source: Coding guidelines
🤖 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
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnostic.java`:
- Around line 82-96: Update isFullReplacement to verify that the paired deletion
and insertion intervals collectively cover the entire method body, rather than
relying only on the first `#Удаление` and final `#КонецВставки` markers. Match each
block’s start/end markers and ensure no original tokens or lines remain outside
the covered intervals before returning true.
- Around line 97-101: Update the directive validation in
SuspiciousChangeAndValidateDiagnostic so replacement detection requires both
markers of each pair: PREPROC_DELETE with PREPROC_ENDDELETE and PREPROC_INSERT
with PREPROC_ENDINSERT. Replace the current containsAny-based checks with
pair-specific validation before calculating boundary lines, while preserving the
existing early return when either complete pair is absent.
---
Nitpick comments:
In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnosticTest.java`:
- Around line 38-49: Rename the test method to describe detection of a full
replacement in SuspiciousChangeAndValidateDiagnosticTest. Extend the fixture
with cases for incomplete directive pairs and complete directive pairs that
retain original code between blocks, then assert that only the true
full-replacement case produces a diagnostic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08e0eea6-4b6f-4559-923f-dd1c6bc372f7
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/SuspiciousChangeAndValidateDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (6)
docs/diagnostics/SuspiciousChangeAndValidate.mddocs/en/diagnostics/SuspiciousChangeAndValidate.mdsrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnostic.javasrc/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnostic_en.propertiessrc/main/resources/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnostic_ru.propertiessrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnosticTest.java
| /** | ||
| * Полная замена: метод с {@code &ИзменениеИКонтроль}, у которого | ||
| * {@code #Удаление} в начале тела и {@code #КонецВставки} в конце — | ||
| * оригинального кода снаружи блоков удаления/вставки не осталось. | ||
| */ | ||
| private static boolean isFullReplacement( | ||
| com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol method, | ||
| List<Token> tokens | ||
| ) { | ||
| var range = method.getRange(); | ||
| int methodStart = range.getStart().getLine(); | ||
| int methodEnd = range.getEnd().getLine(); | ||
|
|
||
| var methodTokens = tokensInRange(tokens, range); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prove that the blocks cover the method body.
The current condition checks only the first deletion line and the last insertion-end line. It still reports a method that keeps original code between those blocks. That is a targeted change, not a full replacement. Match the paired intervals and verify that no original code remains outside them.
Also applies to: 102-109
🤖 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
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SuspiciousChangeAndValidateDiagnostic.java`
around lines 82 - 96, Update isFullReplacement to verify that the paired
deletion and insertion intervals collectively cover the entire method body,
rather than relying only on the first `#Удаление` and final `#КонецВставки` markers.
Match each block’s start/end markers and ensure no original tokens or lines
remain outside the covered intervals before returning true.
- Require both start/end markers for each directive pair (not just any) - Add hasCodeBetween check to verify no code between #EndDelete and #Insert - Rename test to detectsFullReplacementWithDeleteAndInsert - Update JavaDoc with detailed contract description
…blocks Extend fixture with: - НеполнаяПара: missing #КонецУдаления → not a full replacement - СКодомМеждуБлоками: real code between #КонецУдаления and #Вставка → not a full replacement (comment would be ignored — default channel only)
|
Выглядит корректно, но хочу посмотреть бенч |
2 participants
Описание
Новая диагностика SuspiciousChangeAndValidate — находит методы с аннотацией
&ИзменениеИКонтроль, в которых#Удаление/#КонецУдаленияи#Вставка/#КонецВставкииспользуются для полной замены тела метода.Аннотация
&ИзменениеИКонтрольпредназначена для точечных правок с сохранением читаемого diff'а. Полная замена скрывает реальные изменения от ревьюера — для этого следует использовать&Вместо.Closes #2739
Метаданные
Как работает
&ИзменениеИКонтроль#Удаление/#КонецУдаленияAND#Вставка/#КонецВставки#Удалениедолжно быть в начале тела метода (≤ 3 строки),#КонецВставки— в конце (≤ 3 строки от конца)Зеркальная к
InsertionWithoutChangeAndValidate(#3815 → PR #4397).Состав PR
_ru.properties/_en.propertiesЧеклист
Summary by CodeRabbit
New Features
Documentation
Tests