fix: Исправлены ложные срабатывания диагностик MagicNumber и MagicDate при работе со структурами #3542
Conversation
WalkthroughReplaces per-diagnostic visitor logic with a shared AbstractMagicValueDiagnostic that centralizes expression extraction and context checks (structures/correspondences/defaults); MagicNumberDiagnostic and MagicDateDiagnostic now extend it, add configuration, and adjust eligibility/validation; tests and docs updated to reflect changed diagnostic behavior and documented exceptions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant D as Diagnostic (MagicNumber/MagicDate)
participant A as AbstractMagicValueDiagnostic
participant P as ParseTree / AST
D->>D: visit literal node
D->>A: request getExpression() & shouldAddDiagnostic()
A->>P: traverse ancestors (assign, call params, struct ctor, insert, prop assign, return)
P-->>A: context info (structure/correspondence/default/return)
alt insideStructureOrCorrespondence or default-value
A-->>D: early bailout (skip diagnostic) note right of A `#d0f0c0`: skip
else
A-->>D: validate literal (pattern/config)
D-->>D: emit diagnostic if violation note right of D `#f9f0d9`: report
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-02-10T17:13:41.452ZApplied to files:
📚 Learning: 2025-04-18T22:46:43.245ZApplied to files:
📚 Learning: 2025-11-19T09:02:03.154ZApplied to files:
📚 Learning: 2024-07-04T19:35:26.747ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
201-217: Consider simplifying the null-safety checks.The implementation correctly identifies structure property assignments by checking for
acceptor.accessProperty(). However, thelValue.isEmpty()check on line 212 may be redundant since an empty lValue would result in a null acceptor, which is already handled by theObjects::nonNullfilter on line 214.Consider this minor simplification:
private static boolean insideStructurePropertyAssignment(Optional<BSLParser.ExpressionContext> expression) { return expression .map(BSLParserRuleContext::getParent) // callParam .filter(context -> context.getChildCount() == 1) .map(BSLParserRuleContext::getParent) // callParamList .filter(context -> context.getChildCount() == 1) .map(BSLParserRuleContext::getParent) // doCall .map(BSLParserRuleContext::getParent) // assignment .filter(BSLParser.AssignmentContext.class::isInstance) .map(BSLParser.AssignmentContext.class::cast) .map(BSLParser.AssignmentContext::lValue) - .filter(lValue -> lValue != null && !lValue.isEmpty()) + .filter(Objects::nonNull) .map(BSLParser.LValueContext::acceptor) .filter(Objects::nonNull) .map(acceptor -> acceptor.accessProperty() != null) .orElse(false); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/diagnostics/MagicDate.md(1 hunks)docs/diagnostics/MagicNumber.md(2 hunks)docs/en/diagnostics/MagicDate.md(1 hunks)docs/en/diagnostics/MagicNumber.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(1 hunks)src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build
- GitHub Check: Analyse
- GitHub Check: Benchmark
🔇 Additional comments (8)
src/main/resources/com/github/_1c_syntax/bsl/languageserver/configuration/schema.json (1)
924-929: LGTM! Formatting improvement.The enum arrays have been reformatted from single-line to multi-line for better readability. No semantic changes.
Also applies to: 933-939, 943-947, 978-983
docs/diagnostics/MagicNumber.md (1)
34-56: LGTM! Clear documentation of exception cases.The new "Exceptions" section effectively documents when the diagnostic does not trigger, with clear code examples for structure property assignments and method calls.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
37-37: LGTM! Necessary import added.The
Objectsimport is used for the null-safety check in the newinsideStructurePropertyAssignmentmethod.
98-98: The implementation is incomplete and does not cover direct property assignments.My verification confirms the original review comment's concern. The
insideStructurePropertyAssignmentmethod traverses throughcallParam → callParamList → doCall → assignment, which only suppresses diagnostics when the magic date is inside a method call. However, it does not handle direct property assignments likeStructure.Property = '20240101'.Evidence:
- The implementation specifically requires
doCall(method call context) in the traversal path- Direct property assignments lack this intermediate node structure
- Test data file contains no test cases for direct property assignments
- The method only detects assignments where the RHS contains a method/function invocation, not bare value assignments
This gap means direct property assignments to structure properties will still trigger the diagnostic, contrary to what the review comment verified.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
129-160: Implementation is sound, but verify it's used correctly.The helper methods
isStructurePropertyAssignmentandfindAssignmentContextare well-implemented:
findAssignmentContextcorrectly traverses up the AST to find the nearestAssignmentContextisStructurePropertyAssignmentproperly checks for structure property access viaacceptor.accessProperty()However, the usage of this method in
isWrongExpression(line 121) may be incorrect. See the related comment on lines 118-123.docs/en/diagnostics/MagicDate.md (1)
39-60: LGTM! Clear exception documentation.The Exceptions section clearly documents the cases where the diagnostic does not trigger, with helpful code examples for both structure property assignments and method calls.
docs/diagnostics/MagicDate.md (1)
39-60: LGTM! Comprehensive exception documentation.The Exceptions section effectively documents when the MagicDate diagnostic does not trigger, with clear Russian code examples for structure property assignments and method calls.
docs/en/diagnostics/MagicNumber.md (1)
34-56: LGTM! Helpful exception documentation.The Exceptions section clearly describes the non-trigger scenarios with appropriate code examples for both structure property assignments and method calls.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
Outdated
Show resolved
Hide resolved
johnnyshut
force-pushed
the
deature/3496
branch
from
0643789 to
a73b562
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
202-230: Code duplication: identical helpers in MagicNumberDiagnostic.These methods are duplicated in
MagicNumberDiagnostic.java(lines 137-163). See the refactoring suggestion in that file to extract shared logic to a common utility class.
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
137-163: Consider extracting shared helper methods to a common utility class.Both
isStructurePropertyAssignmentandfindAssignmentContexthave identical implementations inMagicDateDiagnostic.java(lines 202-230). Extracting these to a shared utility class would improve maintainability and reduce duplication.For example, create a utility class in the diagnostics package:
package com.github._1c_syntax.bsl.languageserver.diagnostics; public final class StructureAssignmentHelper { private StructureAssignmentHelper() { // utility class } public static boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expression) { var assignment = findAssignmentContext(expression); if (assignment == null) { return false; } var lValue = assignment.lValue(); if (lValue == null || lValue.isEmpty()) { return false; } var acceptor = lValue.acceptor(); return acceptor != null && acceptor.accessProperty() != null; } public static BSLParser.AssignmentContext findAssignmentContext(BSLParserRuleContext ctx) { var current = ctx.getParent(); while (current != null) { if (current instanceof BSLParser.AssignmentContext assignmentContext) { return assignmentContext; } current = current.getParent(); } return null; } }src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
37-37: Remove unused import.The
java.util.Objectsimport added on line 37 is not used anywhere in the file.Apply this diff:
-import java.util.Objects;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (6)
docs/diagnostics/MagicDate.md(1 hunks)docs/diagnostics/MagicNumber.md(2 hunks)docs/en/diagnostics/MagicDate.md(1 hunks)docs/en/diagnostics/MagicNumber.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/en/diagnostics/MagicDate.md
- docs/diagnostics/MagicDate.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Benchmark
🔇 Additional comments (4)
docs/diagnostics/MagicNumber.md (2)
34-56: Well-documented exceptions align with the fix.The new "Exceptions" section clearly explains when the diagnostic does not trigger, using concrete, representative examples that match the reported issue. The examples correctly show structure property assignments and method calls as exception cases. The formatting and Russian description are clear and consistent with the documentation style.
34-56: Verify whether English documentation requires parallel updates.The PR checklist mentions descriptions should be filled for both Russian and English versions (though English translation can be omitted). Please confirm whether a corresponding English documentation file (likely
docs/diagnostics/MagicNumber.mdor similar without language qualifier) needs to be updated with equivalent exception documentation.docs/en/diagnostics/MagicNumber.md (1)
34-56: LGTM! Clear documentation of exceptions.The new Exceptions section clearly documents when the diagnostic should not trigger, with helpful examples that align with the PR objectives.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
118-131: Logic is correct - false positive issue properly addressed.Contrary to the previous review comment, the current implementation correctly excludes structure property assignments. The early return with
falseon line 123 ensures the filter rejects structure property assignments, making the Optional empty and causingisWrongExpressionto return false, which prevents the diagnostic from firing.Logic flow:
- If
isStructurePropertyAssignment→ filter returnsfalse→ Optional empty →isPresent()returnsfalse→ diagnostic does NOT fire ✓The code has been updated since the previous review and now implements the correct behavior.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
37-37: Remove unused import.The
java.util.Objectsimport is not used anywhere in this file.Apply this diff to remove the unused import:
-import java.util.Objects;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Benchmark
🔇 Additional comments (4)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
42-42: LGTM! Test expectation correctly updated.The reduction from 18 to 17 diagnostics reflects the new suppression logic for structure property assignments.
72-72: LGTM! Test expectation correctly updated.The reduction from 10 to 9 diagnostics confirms the suppression logic works correctly with custom configuration.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
97-100: LGTM! Past review concern successfully addressed.The diagnostic condition now correctly includes the
insideStructurePropertyAssignmentcheck, resolving the critical issue flagged in the previous review where this method was defined but not called.
203-226: LGTM! Logic correctly detects structure property assignments.The method properly traverses the AST to find assignments with property accessors, successfully identifying patterns like
structure.property = value. Null safety is appropriately handled at each step.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: Analyze the repo with CodeSee
🔇 Additional comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (2)
118-131: LGTM! Logic correctly excludes structure property assignments.The filter logic is correct: when
isStructurePropertyAssignmentreturnstrue, the filter returnsfalse, making the Optional empty, soisPresent()returnsfalseand the diagnostic does not fire. This aligns with the PR objective to suppress false positives for structure property assignments.
143-162: No code duplication found. The method correctly uses the inherited implementation from AbstractVisitorDiagnostic.The verification confirms that
MagicNumberDiagnosticextendsAbstractVisitorDiagnosticand the call tofindAssignmentContextat line 150 correctly resolves to the inheritedprotected staticmethod defined in the base class. There is no private duplicate implementation inMagicNumberDiagnostic, so no refactoring is needed.src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
42-60: LGTM! Test expectations updated correctly.The diagnostic count adjustments (18→17 and 10→9) correctly reflect the new behavior where structure property assignments no longer trigger the MagicDate diagnostic.
Also applies to: 72-82
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java (1)
52-67: Code verified—shared implementation is properly reused.The search confirms that both
MagicNumberDiagnosticandMagicDateDiagnosticare calling the sharedfindAssignmentContext()method fromAbstractVisitorDiagnostic(lines 150 and 209 respectively). No duplicate private implementations exist in the subclasses. The refactoring successfully eliminated code duplication.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
86-180: Refactor or remove redundant test methods.These six test methods repeat the same pattern from past reviews: all call
getDiagnostics()on the default fixture and verify different subsets of the same 17 diagnostics. They don't isolate specific behaviors with dedicated test inputs.For example:
testValidDateFormatsclaims to test various date formats but doesn't create test code with different format variationstestReturnSimpleDateclaims to verify return statement suppression but doesn't create a focused test case with only return statementstestStructurePropertyAssignmentclaims to test structure property suppression but doesn't create test code demonstrating structure property assignmentsThese tests don't add value beyond the main
test()method. Consider:
- Removing these tests entirely, or
- Refactoring each to test focused scenarios with custom test code via
getDocumentContext(testCode):@Test void testStructurePropertyAssignment() { var testCode = """ Структура = Новый Структура(); Структура.Свойство = "00020101"; // Should NOT trigger diagnostic Переменная = "00020101"; // SHOULD trigger diagnostic """; var diagnostics = getDiagnostics(getDocumentContext(testCode)); assertThat(diagnostics).hasSize(1); assertThat(diagnostics, true).hasRange(2, ...); // Only line 2 }Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(3 hunks)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
139-156: Time bounds are too permissive (can accept 24:60:60).Use canonical ranges to avoid false positives on invalid date strings.
Apply this diff:
- return hh <= 24 && mm <= 60 && ss <= 60; + return hh >= 0 && hh < 24 && mm >= 0 && mm < 60 && ss >= 0 && ss < 60;
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
298-307: Duplicate helper: extract or reuse base implementation.findAssignmentContext(...) appears here and (likely) in related diagnostics. Prefer a single protected static helper in AbstractVisitorDiagnostic; remove duplicates and call the shared one.
Apply this diff (assuming base now provides it):
- private static BSLParser.AssignmentContext findAssignmentContext(BSLParserRuleContext ctx) { - var current = ctx.getParent(); - while (current != null) { - if (current instanceof BSLParser.AssignmentContext assignmentContext) { - return assignmentContext; - } - current = current.getParent(); - } - return null; - }If not yet extracted, move this implementation to AbstractVisitorDiagnostic and adjust usages accordingly.
🧹 Nitpick comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (3)
83-89: Normalize configured authorizedDates to digits‑only and validate length.isExcluded() strips non‑digits from candidate values; mirror that normalization here to ensure matches and reduce surprises.
Apply this diff:
- Set<String> authD = Arrays.stream(authorizedDatesString.split(",")) - .map(String::trim) - .collect(Collectors.toSet()); + Set<String> authD = Arrays.stream(authorizedDatesString.split(",")) + .map(String::trim) + .map(s -> nonNumberPattern.matcher(s).replaceAll("")) + .filter(s -> !s.isEmpty() && (s.length() == 8 || s.length() == 12 || s.length() == 14)) + .collect(Collectors.toSet());
118-131: 12‑digit format gap vs defaults.DEFAULT_AUTHORIZED_DATES contains "000101010000" (12 digits), but string validation accepts only 8/14 digits. If 12‑digit "YYYYMMDDHHMM" strings are valid inputs in this context, extend the validator accordingly.
- Update validator to handle 12‑digit strings:
- if (strDate.length() == 8) { + if (strDate.length() == 8) { return true; } - var hh = parseInt(strDate.substring(8, 10)); - var mm = parseInt(strDate.substring(10, 12)); - var ss = parseInt(strDate.substring(12, 14)); - return hh <= 24 && mm <= 60 && ss <= 60; + if (strDate.length() == 12) { + var hh = parseInt(strDate.substring(8, 10)); + var mm = parseInt(strDate.substring(10, 12)); + return hh >= 0 && hh < 24 && mm >= 0 && mm < 60; + } + var hh = parseInt(strDate.substring(8, 10)); + var mm = parseInt(strDate.substring(10, 12)); + var ss = parseInt(strDate.substring(12, 14)); + return hh >= 0 && hh < 24 && mm >= 0 && mm < 60 && ss >= 0 && ss < 60;
- If 12‑digit strings are intended, also broaden paramPattern to include 12 digits:
""[0123]{1}\d{7}"|"[0123]{1}\d{11}"|"[0123]{1}\d{13}"
195-209: getExpression() brittleness (strict childCount==1 hops).This can miss valid trees with extra trivia nodes. Consider a more tolerant climb (e.g., walk parents until ExpressionContext) or reuse a central AST utility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Analyse
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
107-111: Structure‑property suppression is correctly wired.The added check prevents false positives for structure property assignments as intended. LGTM.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
johnnyshut
force-pushed
the
deature/3496
branch
from
198a942 to
f17fa5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
85-167: Remove or refactor superficial test methods.These five test methods (
testValidDateFormats,testExcludedDates,testSimpleDateAssignment,testReturnSimpleDate,testAssignmentWithDateMethod) all perform identical operations—callinggetDiagnostics()and asserting size/ranges—without isolating or verifying the specific private methods they claim to test. Past review feedback already identified this issue with similar tests.Consider either removing these redundant tests or refactoring them to create focused test cases that exercise specific validation logic (e.g., dedicated fixtures for date format validation, exclusion lists, or assignment contexts).
169-198: Misleading test—correct the comment and add focused structure property assignment test cases.The comment at lines 175-176 incorrectly identifies line 30 as a structure property assignment when it's actually an
Execute()call. Past review confirmed this discrepancy. Additionally, the test doesn't demonstrate actual structure property assignment suppression (e.g.,Структура.Свойство = "00020101"orСтруктура.Вставить("Ключ", "00020101")).Required fixes:
- Remove or correct the misleading comment.
- Add dedicated test cases with actual structure property assignments to verify the suppression feature works as intended.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
281-291: Consider narrowing suppression to Structure type only.The current implementation suppresses diagnostics for all property assignments (e.g.,
Obj.Property = "00020101"), regardless of whether the base object is aСтруктура/Structure. Past review identified this over-broad suppression concern.To align precisely with the PR objective, consider using
documentContext.getSymbolTree()(available to diagnostics) to verify the base expression type isСтруктура/Structurebefore suppressing. This would prevent unintended suppression for non-Structure property assignments and ensure test coverage includes such cases.
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
149-162: Consider narrowing suppression to Structure type only.The current implementation suppresses diagnostics for all property assignments (e.g.,
Obj.Property = 20), regardless of whether the base object is actually aСтруктура/Structure. This matches the over-broad suppression concern raised in theMagicDateDiagnosticpast review.To align with the PR objective (suppress for structure properties specifically), consider using
documentContext.getSymbolTree()to verify the base expression type isСтруктура/Structurebefore suppressing. This refinement would prevent unintended suppression for non-Structure property assignments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (8)
docs/diagnostics/MagicDate.md(1 hunks)docs/diagnostics/MagicNumber.md(2 hunks)docs/en/diagnostics/MagicDate.md(1 hunks)docs/en/diagnostics/MagicNumber.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(10 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/diagnostics/MagicDate.md
- docs/en/diagnostics/MagicDate.md
- docs/diagnostics/MagicNumber.md
- docs/en/diagnostics/MagicNumber.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java
🔇 Additional comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java (1)
52-67: LGTM! Well-placed helper to eliminate duplication.The
findAssignmentContextmethod correctly walks up the AST to locate the nearest enclosingAssignmentContext. Extracting this logic to the base class eliminates duplication betweenMagicDateDiagnosticandMagicNumberDiagnostic, improving maintainability.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
118-131: LGTM! Structure property assignment suppression logic is correct.The early-return pattern at lines 122-124 correctly suppresses the diagnostic when
isStructurePropertyAssignmentreturnstrue. The flow ensures structure property assignments (e.g.,Структура.Свойство = 20) do not trigger MagicNumber diagnostics, aligning with the PR objectives.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
108-111: LGTM! Structure property assignment check correctly integrated.The addition of
insideStructurePropertyAssignmentat line 111 correctly suppresses the diagnostic for structure property assignments, resolving the critical issue from past reviews. The condition chain now properly handles all intended exclusion contexts.
77-273: LGTM! Well-documented validation and context detection methods.The new and updated methods (
configure,isValidDate,parseInt,isAccepted,isExcluded,getExpression, and various context-checking helpers) are well-structured with clear Javadoc comments. The validation logic for date formats and context detection is straightforward and appropriate for the diagnostic's purpose.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
...ain/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
85-174: Remove or refactor superficial test methods that don't add meaningful coverage.These five test methods (
testValidDateFormats,testExcludedDates,testSimpleDateAssignment,testReturnSimpleDate,testAssignmentWithDateMethod) all callgetDiagnostics()on the same fixture and assert subsets of the same 17 diagnostics. They claim to test specific scenarios (various date formats, excluded dates, simple assignments, return statements, Date() method usage) but don't isolate or demonstrate those behaviors—they simply repeat assertions on the same integration test output.These tests were flagged as redundant in previous review rounds and haven't been meaningfully improved. Consider either:
- Removing them entirely (recommended), or
- Creating focused test fixtures that isolate each scenario:
- Create separate
.bslfiles with specific test cases for each scenario- Use dedicated configuration where relevant (e.g., for excluded dates)
- Assert the specific behavior being tested (e.g., for
testExcludedDates, configure authorized dates and verify those specific dates don't generate diagnostics)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(2 hunks)
🔇 Additional comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
42-42: Diagnostic count reduction is correct and properly implemented.The test counts were reduced (18→17 and 10→9) due to newly added logic in
MagicDateDiagnostic.insideStructurePropertyAssignment()that suppresses magic date diagnostics when they appear in structure property assignments.Verification confirms:
- The diagnostic implementation includes the
insideStructurePropertyAssignment()method (lines 280-293) that checks if a magic date is on the right-hand side of a structure property assignment- The method is correctly called in
visitConstValue()as part of the suppression chain- Fixture line 32 (
ОтборЭлемента.ПравоеЗначение = Новый СтандартнаяДатаНачала(Дата('19800101000000'));) contains exactly this pattern: a structure property assignment with a magic date inside a Date() constructor- This is the suppressed diagnostic causing the count reduction from 18→17 (first test) and 10→9 (second test)
The change is intentional and appropriate—it reduces false positives by not flagging magic dates in structure property assignments where the property name itself provides semantic context.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java
Outdated
Show resolved
Hide resolved
johnnyshut
force-pushed
the
deature/3496
branch
from
2762463 to
5db7b11
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
122-122: Remove redundant class name qualifier.As noted in the past review, the explicit
MagicNumberDiagnostic.qualifier is unnecessary when calling a static method from within the same class.Apply this diff:
- if (MagicNumberDiagnostic.isStructurePropertyAssignment(expression)) { + if (isStructurePropertyAssignment(expression)) {src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
421-451: Over-broad suppression remains unaddressed: matches any property assignment.The implementation suppresses diagnostics for all property assignments (e.g.,
Obj.Date = "20250101"), not just Structure properties. The past review flagged this concern and suggested usingdocumentContext.getSymbolTree()to verify the base type is actually "Структура"/"Structure", but this type validation was not implemented.This means cases like:
ОтборЭлемента.ПравоеЗначение = "19800101" // Not a Structure, but suppressedwill incorrectly suppress the diagnostic.
Consider either:
- Adding type checking via the symbol tree to verify the base object is a Structure type, or
- Documenting this limitation if type information is unavailable or the broader suppression is intentional
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
191-221: Clarify test expectations for return statement suppression.The Javadoc states "Проверяет, что возврат дат из функций НЕ генерирует диагностики" (verifies that returning dates from functions does NOT generate diagnostics), but the test expects 19 diagnostics—the same count as the main test.
If return statements are being suppressed, we'd expect either:
- Fewer diagnostics than the baseline test, or
- Assertions that specifically verify return statement lines are absent from the diagnostic list
The current test doesn't demonstrate that return statement suppression is working. Consider adding explicit checks using
assertNoDiagnosticOnLinefor specific return statement lines to verify suppression.
259-270: Make edge case test more specific.The test claims to verify "пустые строки, null значения, некорректные форматы" (empty strings, null values, incorrect formats) but only asserts that diagnostics are not empty, without verifying specific edge case behavior. Consider adding explicit test cases or assertions for each claimed edge case scenario.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (8)
docs/diagnostics/MagicDate.md(1 hunks)docs/diagnostics/MagicNumber.md(2 hunks)docs/en/diagnostics/MagicDate.md(1 hunks)docs/en/diagnostics/MagicNumber.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(7 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/diagnostics/MagicDate.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/en/diagnostics/MagicDate.md
- docs/en/diagnostics/MagicNumber.md
- docs/diagnostics/MagicNumber.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Analyse
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build
- GitHub Check: Benchmark
🔇 Additional comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java (1)
56-68: LGTM!The helper method is well-implemented, uses the appropriate
Trees.getRootParentutility as suggested in past reviews, and is correctly scoped for reuse by diagnostic subclasses.src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
239-257: LGTM! Core feature verification is correct.This test properly verifies the PR's main objective: suppressing MagicDate diagnostics for structure property assignments while still generating them for simple variable assignments. The use of line-specific assertions clearly demonstrates the expected behavior.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
434-487: Consider adding type verification to structure property assignments.There's an asymmetry in how structure detection works:
isStructureMethodCall(lines 466-487): Verifies the object is actually a Structure type viaisVariableOfStructureTypebefore suppressingisStructurePropertyAssignment(lines 434-457): Only checks foraccessProperty()without type verificationThis means
NonStructureObject.Property = "00020101"would be incorrectly suppressed even though the object isn't a Structure.For consistency and correctness, consider extracting the base object name from the
acceptorinisStructurePropertyAssignmentand callingisVariableOfStructureTypeto verify it's actually a Structure, similar to howisStructureMethodCallhandles it.Example approach:
private static boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expr) { if (expr == null) { return false; } var assignment = (BSLParser.AssignmentContext) Trees.getRootParent((BSLParserRuleContext) expr, BSLParser.RULE_assignment); if (assignment == null) { return false; } var lValue = assignment.lValue(); if (lValue == null || lValue.isEmpty()) { return false; } var acceptor = lValue.acceptor(); if (acceptor == null) { return false; } var accessProperty = acceptor.accessProperty(); - return accessProperty != null; + if (accessProperty == null) { + return false; + } + + // Extract base object name and verify it's a Structure type + String objectName = extractObjectName(acceptor); + return objectName != null && isVariableOfStructureType(objectName); }Note: You'll need to make
isVariableOfStructureTypenon-private (protected or package-private) and implementextractObjectNameto parse the object identifier from the acceptor context.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(7 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Benchmark
🔇 Additional comments (4)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
27-336: Excellent test coverage and structure!The test file demonstrates strong engineering practices:
- Helper methods (lines 67-96) eliminate duplication and provide clear, reusable assertions
- Line number constants (lines 40-50) make tests maintainable and self-documenting
- Comprehensive scenarios: covers configuration, date formats, exclusions, assignments, structure handling, edge cases, and bilingual support
- Clear documentation: Javadoc comments explain what each test verifies
The test suite thoroughly validates the PR objectives around suppressing diagnostics for structure property assignments while ensuring normal cases still trigger.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (3)
47-68: Outstanding documentation!The class-level Javadoc comprehensively documents:
- Purpose and behavior
- Supported date formats
- Suppression scenarios (structure properties, method calls, returns, etc.)
- Configuration options
This makes the diagnostic's behavior crystal clear to users and maintainers.
205-218: Well-designed centralized decision logic.The
shouldGenerateDiagnosticmethod consolidates all suppression checks into a single, testable decision point. This improves maintainability and makes the diagnostic's behavior easier to understand and modify.The optimization note about minimizing AST tree traversals shows attention to performance.
515-569: Robust structure type detection with graceful fallback.The implementation demonstrates good engineering:
- Primary path: Uses
SymbolTreefor accurate type resolution (lines 528-544)- Fallback: Heuristic name-based detection when symbol tree unavailable (lines 554-569)
- Bilingual support: Handles both Russian and English naming conventions
The heuristic approach is pragmatic—while not perfect, it provides reasonable coverage for common naming patterns when formal type information isn't available.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
514-568: Symbol resolution with sensible heuristic fallback.The
isVariableOfStructureTypemethod correctly prioritizes symbol tree lookup with description analysis, falling back to name-based heuristics when symbol information is unavailable. The heuristic keywords (структура, structure, стр, str, данные, data, параметры, params) are reasonable for typical 1C:Enterprise code patterns.Note: The heuristic at lines 560-567 could have false positives (e.g., a variable named
"строка"would match"стр"), but this trade-off is acceptable given the fallback nature and difficulty of perfect type inference without full semantic analysis.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
DiagnosticMetadata(54-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
🔇 Additional comments (7)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (7)
24-25: VariableSymbol import is correctly used.Despite a past review comment flagging this import as unused, it's actually required at line 532 where
symbolTree.getVariableSymbol()returnsOptional<VariableSymbol>. The import should remain.
46-67: Excellent documentation.The comprehensive JavaDoc clearly explains the diagnostic's purpose, suppression scenarios, and supported date formats. This aligns perfectly with the PR objectives and helps future maintainers understand the intended behavior.
108-147: Robust configuration handling.The enhanced configuration logic with null checks, empty-string filtering, and date validation (
isValidDateString) prevents configuration errors and ensures only valid dates are authorized. This defensive approach improves reliability.
149-193: Excellent refactoring with centralized gating.Both
visitStringandvisitConstValuenow route throughshouldGenerateDiagnostic, eliminating code duplication and ensuring consistent suppression logic across all entry points. The flow is clear: validate → get context → gate → report.
195-217: Core fix correctly implemented.The centralized
shouldGenerateDiagnosticmethod properly implements the PR objective by suppressing diagnostics for structure property assignments and method calls (line 211), while preserving other suppression conditions. The boolean logic is correct: diagnostics are generated only when none of the suppression conditions are met.
225-283: Enhanced date validation is solid.The improved validation logic correctly handles both date formats (YYYYMMDD and YYYYMMDDHHMMSS), validates component ranges, and aligns with 1C platform limits (year 1-9999). The
parseInthelper safely handles leading zeros and exceptions. Note: day validation doesn't enforce month-specific limits (e.g., February 30), but this is acceptable for magic-number detection heuristics.
427-486: Structure detection logic is well-implemented.The methods correctly identify structure property assignments and method calls:
isStructurePropertyAssignmentuses AST traversal to detectObj.Property = valuepatternsisStructureMethodCalluses the bilingualSTRUCTURE_METHOD_PATTERNto match both"Вставить"and"Insert"(case-insensitive)- Text parsing in
isStructureObjectCallis reasonable given AST constraintsThis addresses the PR objective and past review feedback about bilingual support.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
86-90: Fix paramPattern: excludes valid dates (years starting with 4–9).The pattern forces the first digit to [0–3], so strings like "99991231" and "40010101" are never checked. This causes false negatives.
Apply:
- private static final Pattern paramPattern = CaseInsensitivePattern.compile( - "\"[0123]{1}\\d{7}\"|\"[0123]{1}\\d{13}\"" - ); + private static final Pattern paramPattern = CaseInsensitivePattern.compile( + "\"\\d{8}\"|\"\\d{14}\"" + );
80-82: Remove unsupported 12-digit default date from authorized list.DEFAULT_AUTHORIZED_DATES includes "000101010000" (12 digits) while supported lengths are 8 or 14; configure() filters it out anyway, creating a misleading default.
- private static final String DEFAULT_AUTHORIZED_DATES = "00010101,00010101000000,000101010000"; + private static final String DEFAULT_AUTHORIZED_DATES = "00010101,00010101000000";Also update docs mentioning supported formats.
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
433-456: Structure property suppression matches any property; add type check.isStructurePropertyAssignment() returns true for all property assignments (Obj.Prop = "..."), not only Structure. This suppresses diagnostics in non-Structure contexts.
- private static boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expr) { + private boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expr) { if (expr == null) { return false; } @@ - var accessProperty = acceptor.accessProperty(); - return accessProperty != null; + var accessProperty = acceptor.accessProperty(); + if (accessProperty == null) { + return false; + } + // LHS like Var.Property — verify Var is a Structure + String lhs = acceptor.getText(); + int dot = lhs.indexOf('.'); + if (dot <= 0) { + return false; + } + String baseName = lhs.substring(0, dot).trim(); + return isVariableOfStructureType(baseName); }Add a test ensuring non-Structure property assignments (e.g., ОтборЭлемента.ПравоеЗначение = "00020101") still produce diagnostics.
🧹 Nitpick comments (8)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (3)
95-99: Redundant inline (?i) with CaseInsensitivePattern.CaseInsensitivePattern.compile already sets case-insensitive mode; the embedded "(?i)" is redundant.
- private static final Pattern STRUCTURE_METHOD_PATTERN = CaseInsensitivePattern.compile( - "(?i)Вставить|Insert" - ); + private static final Pattern STRUCTURE_METHOD_PATTERN = CaseInsensitivePattern.compile( + "Вставить|Insert" + );
313-321: Duplicate expression-mapping helpers; consider unifying.getExpression(Optional) and getExpressionFromString(StringContext) are identical apart from parameter type.
Extract a single private method accepting BSLParserRuleContext (or a generic mapper) to reduce duplication.
Also applies to: 329-337
404-424: Fragile multi-parent walk; prefer direct AST queries.The deep parent chain in insideAssignmentWithDateMethodForSimpleDate() is brittle. Use Trees.getRootParent(expr, RULE_assignment) and then inspect the RHS for GlobalMethodCallContext with methodPattern, or traverse with Trees.findFirst(...).
If you want, I can draft a helper that finds a GlobalMethodCallContext under an assignment’s rValue.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (5)
195-225: Test intent vs assertions mismatch (return/assignment cases).Docstrings say “return does NOT generate diagnostics” and “assignment with Date() generates diagnostics,” yet assertions don’t verify absence on return lines and rely on global counts/ranges.
- Add assertNoDiagnosticOnLine for return lines.
- For Date() assignment, assert presence on the specific lines; for returns, assert absence.
If line numbers aren’t constant, assert by message substring or by locating the exact Range from the fixture.
268-276: Weak “isNotEmpty” tests — refactor to focused assertions.testEdgeCases, testEdgeCasesExtended, and testStringLiterals only check non-empty diagnostics. Replace with targeted expectations (presence/absence on specific lines or counts) or remove to reduce noise.
Also applies to: 338-350, 369-376
324-336: Latin method coverage: also assert non-Structure Insert still reports.Great that you check DataStructure.Insert. Add a counterexample where Insert is called on a non-Structure to ensure no over-suppression.
131-150: Configuration tests: add assertions that invalid/empty/null authorizedDates don’t break filtering.Since configure() filters invalid dates, assert that:
- invalid dates are not included (e.g., "invalid" doesn’t prevent "00020101" from reporting),
- null/empty reverts to defaults,
- duplicates are handled (Set semantics).
If helpful, assert the diagnostic count difference before/after configuration.
Also applies to: 382-408, 414-424
247-261: Test coverage incomplete: non-Structure property assignments not verified.The test method
testStructurePropertyAssignment()validates Structure property suppression and simple variable reporting, but lacks explicit coverage for non-Structure object properties (e.g.,ОтборЭлемента.ПравоеЗначение = "00020101"). While the current assertions indirectly cover this viaSIMPLE_ASSIGNMENT_LINE, adding an explicit assertion for a non-Structure property assignment would strengthen test coverage.Two approaches:
- Extend the fixture to include
ОтборЭлемента.ПравоеЗначение = "00020101"and add a corresponding assertion.- Create an alternate fixture using
getDiagnostics(String simpleFileName)(supported by the harness).The current test is functionally adequate but incomplete.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(8 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Analyse
🔇 Additional comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
39-47: No off-by-one errors found.Line constants in the test match the actual fixture exactly (lines 73, 74, 76, 84, 85 for the respective constant values). The original review's concern about off-by-one shifts is incorrect.
Likely an incorrect or invalid review comment.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
522-544: No changes needed; review comment is based on unavailable APIs.The VariableSymbol API does not expose type predicates like getType(), typeOf(), or getTypeName(). The available methods are:
getKind()→ VariableKind (scope: LOCAL, PARAMETER, DYNAMIC, etc.—not value type)getDescription()→ Optional (text metadata from comments, no type fields)The current implementation at lines 522-544 already uses the only viable approach: parsing description text for structure keywords plus name-based heuristics. There are no "actual type predicates" available to replace this logic.
Likely an incorrect or invalid review comment.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
86-88: Critical: paramPattern excludes valid dates (years 4000–9999).
"[0123]{1}\\d{7}"and"[0123]{1}\\d{13}"wrongly reject strings starting with 4–9, so"90010101"and"99991231235959"are ignored entirely.Apply:
- private static final Pattern paramPattern = CaseInsensitivePattern.compile( - "\"[0123]{1}\\d{7}\"|\"[0123]{1}\\d{13}\"" - ); + private static final Pattern paramPattern = Pattern.compile( + "\"\\d{8}\"|\"\\d{14}\"" + );Optionally, drop the pre-filter and rely on isValidDate(String) for correctness.
80-85: Default config includes unsupported 12‑digit date.
DEFAULT_AUTHORIZED_DATEScontains000101010000(12 digits), but validators accept only 8 or 14 digits. Afterconfigure(), this entry is silently discarded, creating inconsistent behavior between defaults and configured state.Choose one:
- Align defaults with validators (recommended):
- private static final String DEFAULT_AUTHORIZED_DATES = "00010101,00010101000000,000101010000"; + private static final String DEFAULT_AUTHORIZED_DATES = "00010101,00010101000000";
- Or add 12‑digit support consistently:
- Allow 12 in isValidDateString and isValidDate (parse HHMM without seconds).
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
421-444: Over-broad suppression: property assignment matches any object, not only Structure.
accessProperty() != nullis true for arbitrary object properties, so diagnostics can be suppressed outside Structures (e.g.,Obj.Prop = "00020101"). Re-opened; still present in current code.Tighten by verifying the base object is a Structure (symbol/type when available; heuristic fallback). Also make the method non-static so it can use
documentContext.- private static boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expr) { + private boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expr) { if (expr == null) { return false; } @@ - var accessProperty = acceptor.accessProperty(); - return accessProperty != null; + var accessProperty = acceptor.accessProperty(); + if (accessProperty == null) { + return false; + } + // Extract base object from "Base.Property[.Sub...]" + String acceptorText = acceptor.getText(); + int dotIdx = acceptorText.indexOf('.'); + if (dotIdx <= 0) { + return false; + } + String baseName = acceptorText.substring(0, dotIdx).trim(); + return isVariableOfStructureType(baseName); }Add a test ensuring a non‑Structure property assignment still reports MagicDate. Example (fixture snippet):
ОтборЭлемента.ПравоеЗначение = "00020101"; // should report
🧹 Nitpick comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
453-474: Prefer AST for receiver/method instead of parsing call text.
isStructureMethodCall/isStructureObjectCallparsecallTextand split on the first '.', which breaks on chains likeObj.Sub.Insert(...)and is locale/format fragile.
- Extract the receiver from the parser nodes (e.g., member/access nodes) and resolve its type via
SymbolTree.- Keep the bilingual method check via
STRUCTURE_METHOD_PATTERN, but match against the actual method identifier node, notStringtext. This reduces false positives/negatives in nested expressions.Also applies to: 483-500
203-216: Duplicate checks for simple date assignment.
insideSimpleDateAssignment(...)ends up delegating toinsideAssignmentWithDateMethodForSimpleDate(...), andshouldGenerateDiagnostic(...)evaluates both, duplicating work.
- Remove
isSimpleAssignmentfromshouldGenerateDiagnosticand rely onisDateMethod, or inline a single helper to avoid repeated AST walks.Also applies to: 346-359
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
244-255: Add negative check: non‑Structure property assignment must still report.Great that structure property/method cases are suppressed. Add an explicit non‑Structure property assignment in the fixture and assert that it DOES produce a diagnostic to guard against over-broad suppression.
Example to add to fixture:
ОбъектНеСтруктура.Свойство = "00020101"; // should reportThen assert presence on that line.
318-340: Consolidate superficial “isNotEmpty” tests or make them assertive.Several tests only assert
isNotEmpty(). Either merge them into one sanity test or add precise expectations (ranges/lines) to increase value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(8 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
DiagnosticMetadata(54-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build
- GitHub Check: Analyse
🔇 Additional comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
39-47: Fix off-by-one error in SIMPLE_ASSIGNMENT_LINE constant; verify Latin constants manually.Based on fixture inspection:
SIMPLE_ASSIGNMENT_LINE = 76is off-by-one; the actual assignmentПростаяПеременная = "00040101";is on line 77, not 76.STRUCTURE_INSERT_LINE,STRUCTURE_PROPERTY_LINE,STRUCTURE2_INSERT_LINE, andSTRUCTURE2_PROPERTY_LINEconstants are correct.- Please confirm
LATIN_INSERT_LINE = 93andLATIN_INSERT_CYRILLIC_LINE = 94against the fixture file, as the Latin section in the fixture begins after line 107.
128-147: Incorrect premise about the map mutation concern.The review comment states that
getDefaultConfiguration()"may return a shared map," but this is inaccurate. The method creates a newMapon each invocation viaCollectors.toMap()inDiagnosticInfo.javaline 272-274, so mutations to the local configuration variable intestConfigure()do not leak through that mechanism.However, there is a legitimate test isolation concern:
MagicDateDiagnosticTestlacks@DirtiesContextwhile many peer diagnostic test classes in the codebase explicitly use it (e.g.,MetadataObjectNameLengthDiagnosticTest,TimeoutsInExternalResourcesDiagnosticTest). The real risk is thatdiagnosticInstancestate mutations intestConfigure()could affect subsequent test methods if the bean is reused across the test context. The appropriate fix would be to add@DirtiesContextat the class level, consistent with other diagnostic tests, rather than wrapping the configuration map.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
257-440: Many test methods only assertisNotEmpty()without verifying specific behavior.Several test methods (
testEdgeCases,testEdgeCasesExtended,testStringLiterals,testVisitStringMethod,testInvalidDateFormats,testStructureMethodCallEdgeCases,testSymbolResolutionEdgeCases) only callgetDiagnostics()and assertisNotEmpty(). These don't verify specific edge cases or behavior—they're essentially duplicates of the main test.Consider either removing these or refactoring them to assert specific scenarios (e.g., specific input patterns, expected diagnostic locations, or configuration behaviors). Tests should validate distinct behavior rather than repeat generic checks.
272-396: Minor test duplication: null configuration tested twice.Both
testInvalidConfiguration(line 280) andtestNullConfiguration(lines 372-381) test the null configuration scenario. Consider consolidating related configuration tests to reduce duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(8 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build
🔇 Additional comments (11)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java (1)
55-64: LGTM! Helper method correctly extracts shared AST traversal logic.The
findAssignmentContexthelper properly usesTrees.getRootParentto locate the assignment context, eliminating duplication between MagicDateDiagnostic and MagicNumberDiagnostic. The protected static visibility makes it accessible to diagnostic subclasses.Based on past review comments
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
49-93: LGTM! Helper methods improve test readability.The three helper methods (
hasDiagnosticOnLine,assertNoDiagnosticOnLine,assertHasDiagnosticOnLine) provide clear, reusable utilities for verifying diagnostic presence on specific lines. Good Javadoc documentation.
244-255: LGTM! Test correctly verifies structure property assignment suppression.The test properly validates the PR's core feature: diagnostics are suppressed for structure property assignments (
Структура.Вставить,Структура.СвойствоДаты) but still fire for simple variable assignments. Good use of helper methods for clear assertions.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (8)
46-67: LGTM! Comprehensive class documentation.The Javadoc clearly documents when the diagnostic is suppressed (structure property assignments, structure method calls, simple date assignments, returns, authorized dates) and lists supported date formats. This helps users understand the diagnostic's behavior.
107-132: LGTM! Enhanced configuration handling with proper validation.The configure method now includes null checks, validates date strings with
isValidDateString, and filters out invalid entries. This prevents configuration errors from breaking the diagnostic.
194-216: LGTM! Centralized suppression logic.The
shouldGenerateDiagnosticmethod consolidates all suppression checks in one place, making the logic clear and maintainable. Properly checks for structure property assignments, structure method calls, returns, and date method assignments.
233-267: LGTM! Date validation with correct bounds.The validation properly uses
MAX_YEAR_BY_1Cconstant for year bounds and strict inequalities for time components (hh < 24,mm < 60,ss < 60). Handles both 8-digit (date only) and 14-digit (date+time) formats correctly.
446-474: LGTM! Bilingual structure method detection with type checking.The method now uses
STRUCTURE_METHOD_PATTERNto match bothВставитьandInsert(case-insensitive), and delegates toisStructureObjectCallfor proper type verification via the symbol tree. This addresses previous concerns about language limitations and over-broad suppression.Based on past review comments
502-530: LGTM! Symbol tree-based type detection with safe fallback.The method properly uses
SymbolTreeto resolve variable types and checks the description for structure indicators. Returns false if the symbol tree is unavailable or the variable isn't found, which is a safe default that avoids false suppression.
338-359: LGTM! Refactored to use AST-based detection.The method now properly delegates to
insideAssignmentWithDateMethodForSimpleDatefor precise AST-based checking, eliminating the previous text-based heuristic that caused false positives. Correctly excludes structure property assignments and method calls first.Based on past review comments
414-444: Property assignment suppression remains over-broad without type checking.The method suppresses diagnostics for any property assignment (e.g.,
Obj.Property = "00020101") without verifying thatObjis actually a Structure. WhileisVariableOfStructureTypeexists (line 510) and is correctly used inisStructureObjectCallfor method calls, it is not used inisStructurePropertyAssignmentfor property assignments.This means property assignments to non-Structure objects—or even undefined variables like
ОтборЭлемента.ПравоеЗначениеon line 32—are also suppressed, creating an asymmetry with method call handling. Either add type checking toisStructurePropertyAssignmentsimilar toisStructureObjectCall, or document why all property assignments are intentionally suppressed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
80-81: Remove the unsupported 12-digit value from DEFAULT_AUTHORIZED_DATES.The constant includes "000101010000" (12 digits), but the isValidDateString method only accepts 8 or 14 digit strings (line 141). During configuration, this 12-digit value is silently filtered out (lines 124–131), contradicting the documented supported formats. Remove the 12-digit value to eliminate the confusing inconsistency:
- private static final String DEFAULT_AUTHORIZED_DATES = "00010101,00010101000000,000101010000"; + private static final String DEFAULT_AUTHORIZED_DATES = "00010101,00010101000000";
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
417-440: Over-broad suppression: property assignment without Structure type check.isStructurePropertyAssignment() suppresses diagnostics for any property assignment, not only Structure properties. This masks cases like Obj.Date = "20250101". Add a Structure-type check using the base identifier of the lValue, consistent with isStructureMethodCall/isVariableOfStructureType.
Apply:
private static boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expr) { @@ - var accessProperty = acceptor.accessProperty(); - return accessProperty != null; + var accessProperty = acceptor.accessProperty(); + if (accessProperty == null) { + return false; + } + // Verify base object is Structure: <Base>.<Property> = ... + var lValueText = lValue.getText(); + int dot = lValueText.indexOf('.'); + if (dot <= 0) { + return false; + } + var objectName = lValueText.substring(0, dot); + // Delegate to existing resolver + return new MagicDateDiagnostic().isVariableOfStructureType(objectName); }Note: if calling a non-static method from static context is undesired, move isVariableOfStructureType to static or a shared util.
🧹 Nitpick comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (3)
199-212: Redundant checks: isSimpleAssignment duplicates isDateMethod.insideSimpleDateAssignment now delegates to insideAssignmentWithDateMethodForSimpleDate; you also compute isDateMethod. Drop one to reduce AST traversals.
- boolean isDateMethod = insideAssignmentWithDateMethodForSimpleDate(expressionContext); - boolean isSimpleAssignment = insideSimpleDateAssignment(expressionContext); - - return !isStructureProperty && !isReturn && !isDateMethod && !isSimpleAssignment; + boolean isDateMethod = insideAssignmentWithDateMethodForSimpleDate(expressionContext); + return !isStructureProperty && !isReturn && !isDateMethod;Optionally remove insideSimpleDateAssignment if no longer used.
388-408: Deep parent-walk is brittle; prefer Trees.getRootParent.insideAssignmentWithDateMethodForSimpleDate chains multiple parent hops. Use Trees.getRootParent to reach GlobalMethodCall and Assignment for clarity and resilience.
- return expression - .map(BSLParserRuleContext::getParent) - .filter(context -> context.getChildCount() == 1) - .map(BSLParserRuleContext::getParent) - .filter(context -> context.getChildCount() == 1) - .map(BSLParserRuleContext::getParent) - .map(BSLParserRuleContext::getParent) - .filter(BSLParser.GlobalMethodCallContext.class::isInstance) - .map(BSLParser.GlobalMethodCallContext.class::cast) - .filter(context -> methodPattern.matcher(context.methodName().getText()).matches()) - .map(BSLParserRuleContext::getParent) - .filter(context -> context.getChildCount() == 1) - .map(BSLParserRuleContext::getParent) - .filter(context -> context.getChildCount() == 1) - .map(BSLParserRuleContext::getParent) - .filter(context -> context.getChildCount() == 1) - .map(BSLParserRuleContext::getParent) - .filter(BSLParser.AssignmentContext.class::isInstance) - .isPresent(); + return expression + .map(expr -> (BSLParser.GlobalMethodCallContext) + Trees.getRootParent((BSLParserRuleContext) expr, BSLParser.RULE_globalMethodCall)) + .filter(ctx -> methodPattern.matcher(ctx.methodName().getText()).matches()) + .map(ctx -> Trees.getRootParent(ctx, BSLParser.RULE_assignment)) + .filter(BSLParser.AssignmentContext.class::isInstance) + .isPresent();
324-332: Duplicate expression-extraction helpers.getExpressionFromString duplicates getExpression logic. Consider extracting a common helper accepting BSLParserRuleContext and reusing it.
Also applies to: 308-316
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
95-122: Brittle golden-count and dense range assertions.Hard-coding hasSize(40) and many ranges ties tests to fixture editing. Prefer focused scenario tests: assert on specific lines of interest (helpers already exist) and avoid global counts.
257-266: Superficial tests asserting only non-empty diagnostics.Several tests only check isNotEmpty(), adding little value and increasing noise. Consolidate or rewrite to assert precise suppression/triggering for specific lines.
Also applies to: 318-327, 402-407, 414-418, 425-429, 436-440
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(8 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/FunctionOutParameterDiagnostic.java (1)
DiagnosticMetadata(41-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build
🔇 Additional comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (2)
128-147: Configured authorizedDates include unsupported 12-digit format.testConfigure passes "000101010000" (12 digits), which the validator now rejects. Align the test with supported formats or extend validation to include 12-digit DATETIME.
39-48: The proposed changes in the review comment are incorrect and should not be applied.The constants STRUCTURE_INSERT_LINE, STRUCTURE_PROPERTY_LINE, STRUCTURE2_INSERT_LINE, and STRUCTURE2_PROPERTY_LINE are all correct as currently written. The fixture file confirms:
- Line 73:
Структура.Вставить()✓- Line 74:
Структура.СвойствоДаты =✓- Line 84:
ПеремСтрДаты.Вставить()✓- Line 85:
ПеремСтрДаты.СвойствоДаты =✓However, there are actual line number errors elsewhere:
- SIMPLE_ASSIGNMENT_LINE = 76 should be 77 (fixture line 77 has the magic date assignment)
- LATIN_INSERT_LINE = 93 should be 111 (fixture line 111 has
DataStructure.Insert())- LATIN_INSERT_CYRILLIC_LINE = 94 should be 112 (fixture line 112 has the Cyrillic version)
Likely an incorrect or invalid review comment.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
322-327: Consider strengthening assertions in edge-case and coverage tests.Several test methods (
testEdgeCasesExtended,testStringLiterals,testInvalidDateFormats,testStructureMethodCallEdgeCases,testSymbolResolutionEdgeCases) only assertisNotEmpty()without verifying specific behavior or diagnostic locations.While these may improve code coverage metrics, they don't validate correctness. Consider either:
- Adding specific assertions about which lines should/shouldn't have diagnostics
- Consolidating these into a single smoke test if they're just ensuring no crashes
Also applies to: 346-351, 403-420, 426-431
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
383-406: Consider adding type validation for property assignments.The method suppresses diagnostics for any property assignment (e.g.,
Obj.Date = "00020101"), regardless of whetherObjis actually a Structure type. WhileisStructureMethodCall()validates types viaisVariableOfStructureType(), property assignments lack this check.This broader suppression may be acceptable if the semantic context of any property name is deemed sufficient, but for consistency with the PR objective ("при присваивании значений к свойствам структуры"), consider using
isVariableOfStructureType()to validate the base object type.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(8 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: Analyse
🔇 Additional comments (8)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
64-93: LGTM: Well-structured test helpers.The helper methods provide clear, focused assertions for checking diagnostics on specific lines. The use of descriptive error messages improves test failure diagnostics.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (7)
46-67: Excellent documentation.The Javadoc clearly describes the diagnostic's purpose, suppression contexts, and supported date formats. This will help users understand when false positives are expected vs. real issues.
86-97: LGTM: Patterns correctly support validation and bilingual detection.The
paramPatternnow accepts any 8- or 14-digit date (delegating validation toisValidDate()), andSTRUCTURE_METHOD_PATTERNproperly handles both Russian and English method names case-insensitively.
107-132: LGTM: Robust configuration handling.The method properly handles null configuration, validates date strings, and filters invalid entries. The defensive programming approach prevents configuration errors from causing runtime issues.
180-193: LGTM: Centralized suppression logic improves maintainability.The
shouldGenerateDiagnostic()method consolidates all suppression conditions in one place, making the logic transparent and reducing the risk of inconsistent checks across different code paths.
216-244: LGTM: Date validation is now correct.The validation properly uses
MAX_YEAR_BY_1Cconstant, enforces strict time component bounds (< 24/60/60), and handles both 8- and 14-digit date formats correctly.
300-321: LGTM: Precise AST-based detection.The method correctly delegates to
insideAssignmentWithDateMethodForSimpleDate()for accurate detection, and explicitly excludes structure-related contexts to avoid false suppressions.
415-494: LGTM: Robust structure method validation with type checking.The implementation properly:
- Detects structure method calls bilingually via pattern matching
- Extracts the base object identifier
- Uses
SymbolTreeto resolve and validate the variable is actually a Structure type- Handles null cases defensively throughout
This precise type checking reduces false positives effectively.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
458-487: Consider adding type verification for consistency with method call checking.The method
isStructurePropertyAssignment()suppresses diagnostics for ANY property assignment (e.g.,Object.Property = value) without verifying the object is actually a Structure type. In contrast,isStructureMethodCall()(lines 500-597) usesisVariableOfStructureType()to verify the object type via symbol tree before suppressing.This inconsistency could lead to over-broad suppression. For example,
ОтборЭлемента.ПравоеЗначение = "00020101"would be suppressed even ifОтборЭлементаis not a Structure.Suggestion: Add type verification similar to
isStructureMethodCall():private static boolean isStructurePropertyAssignment(BSLParser.ExpressionContext expr) { if (expr == null) { return false; } var assignment = getRootParent(expr, BSLParser.RULE_assignment); if (!(assignment instanceof BSLParser.AssignmentContext)) { return false; } var assignmentContext = (BSLParser.AssignmentContext) assignment; var lValue = assignmentContext.lValue(); if (lValue == null || lValue.isEmpty()) { return false; } var acceptor = lValue.acceptor(); if (acceptor == null) { return false; } var accessProperty = acceptor.accessProperty(); - return accessProperty != null; + if (accessProperty == null) { + return false; + } + + // Extract object name and verify it's a Structure type + String objectName = acceptor.getText(); + if (objectName != null) { + int dotIndex = objectName.indexOf('.'); + if (dotIndex > 0) { + objectName = objectName.substring(0, dotIndex); + return isVariableOfStructureType(objectName); + } + } + return false; }Note: This would require making
isVariableOfStructureType()accessible (currently it requiresdocumentContext, so the method signature would need adjustment).Alternatively, if the intent is to suppress diagnostics for ALL property assignments (not just structures), consider renaming the method to
isAnyPropertyAssignment()to reflect its actual behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(6 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
DiagnosticMetadata(54-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build
- GitHub Check: Analyse
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
🔇 Additional comments (10)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (3)
27-93: LGTM! Well-structured test infrastructure.The helper methods and constants significantly improve test readability and maintainability. The Collections import is correctly used at line 274.
95-255: Excellent test coverage for core functionality.The tests properly verify:
- Structure property assignments are suppressed (lines 248-252)
- Structure method calls (Insert/Вставить) are suppressed (lines 248, 251)
- Simple variable assignments still generate diagnostics (line 254)
This aligns perfectly with the PR objectives.
257-431: Comprehensive edge case coverage.The tests effectively cover:
- Bilingual method support (Вставить/Insert)
- Invalid/null/empty configurations
- Different structure variable names
- Symbol resolution scenarios
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (7)
24-31: LGTM! All imports are properly used.The imports for SymbolTree, VariableSymbol, and Trees are correctly utilized in the type resolution logic.
45-108: Excellent documentation and bilingual pattern support.The Javadoc clearly documents suppression contexts, and the updated
paramPatterncorrectly allows all valid 1C years (1-9999). The bilingualSTRUCTURE_METHOD_PATTERNproperly supports both Russian and English code.
116-151: Robust configuration validation.The null guards and
isValidDateString()filtering ensure the diagnostic handles invalid configurations gracefully.
154-220: Well-architected central diagnostic logic.The
shouldGenerateDiagnostic()method effectively centralizes suppression logic, making the code more maintainable and reducing duplication betweenvisitConstValue()andvisitString().
243-286: Date validation logic is correct.The time bounds correctly allow 0-23 hours, 0-59 minutes, and 0-59 seconds. The
parseInt()method safely handles leading zeros and invalid inputs.
362-455: AST navigation logic is well-implemented.The refactored
insideSimpleDateAssignment()correctly delegates to AST-based checking, avoiding the text-based false positives mentioned in past reviews. ThenavigateToParent()helper provides safe traversal with proper null checks.
500-597: Excellent implementation with proper type verification.The structure method call detection correctly:
- Uses symbol tree resolution to verify object types
- Supports bilingual method names (Вставить/Insert)
- Handles edge cases with comprehensive null checks
- Gracefully degrades when symbol information is unavailable
This implementation properly addresses past review feedback about over-broad suppression.
johnnyshut
force-pushed
the
deature/3496
branch
from
7c807eb to
8a7f27c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
257-266: Consider adding specific assertions to edge case tests.Several test methods (
testEdgeCases,testEdgeCasesExtended,testStringLiterals,testStructureMethodCallEdgeCases,testSymbolResolutionEdgeCases) only assert that diagnostics are not empty without verifying specific behavior.Consider enhancing these tests with targeted assertions that verify the actual edge cases they claim to test (e.g., specific line numbers, expected diagnostic messages, or absence of diagnostics in certain scenarios).
Also applies to: 318-327, 342-351, 410-420, 422-431
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (8)
docs/diagnostics/MagicDate.md(1 hunks)docs/diagnostics/MagicNumber.md(2 hunks)docs/en/diagnostics/MagicDate.md(1 hunks)docs/en/diagnostics/MagicNumber.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
- docs/diagnostics/MagicDate.md
- docs/diagnostics/MagicNumber.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
DiagnosticMetadata(54-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, macOS-latest)
🔇 Additional comments (6)
docs/en/diagnostics/MagicNumber.md (1)
33-56: LGTM! Clear documentation of exception cases.The new "Exceptions" section effectively documents when the diagnostic is suppressed for structure property assignments and method calls. The examples are clear and align with the PR objectives.
docs/en/diagnostics/MagicDate.md (1)
38-60: LGTM! Consistent documentation with clear examples.The "Exceptions" section mirrors the MagicNumber documentation structure and provides clear examples specific to date literals. Well done.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractVisitorDiagnostic.java (1)
55-64: LGTM! Good refactoring to eliminate duplication.Extracting the
findAssignmentContexthelper to the base class is the right approach. The method is correctly implemented usingTrees.getRootParentand will be shared by both MagicNumber and MagicDate diagnostics.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
45-66: Excellent refactoring with centralized suppression logic.The comprehensive Javadoc (lines 45-66) clearly documents the diagnostic's purpose and exception cases. The
shouldGenerateDiagnosticmethod (lines 200-220) nicely centralizes all suppression checks in one place, making the logic easy to understand and maintain. The implementation successfully addresses the PR objectives of suppressing false positives for structure property assignments.Also applies to: 200-220
567-597: No issues found with the double.getDescription()call—it's correct API usage.The chain is intentional:
VariableSymbol.getDescription()returnsOptional<VariableDescription>, and the inner.map(description -> description.getDescription())calls theSourceDefinedSymbolDescription.getDescription()method (which returnsString) on eachVariableDescriptionobject. This is proper Optional chaining.The type-checking logic via
SymbolTreeis sound and properly prevents false positives.src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
240-255: LGTM! Well-structured test for the core feature.The
testStructurePropertyAssignmentmethod effectively verifies the PR's main objective: suppressing diagnostics for structure property assignments while still triggering for simple variable assignments. The helper methods provide clear, descriptive assertions.
|
@johnnyshut можешь над кавереджом в magic date поработать? Такое чувство, что visit String больше не вызывается, все обрабатывается на уровне visitConstValue с ранним возвратом. |
Да, Ты прав, хочу убрать лишнее и тесты плохо сделал |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
25-25: Remove unusedVariableSymbolimport.The
VariableSymbolimport is not referenced anywhere in the file. Despite being marked as addressed in previous reviews, it remains unused—getVariableSymbol()at line 478 returnsOptional<VariableSymbol>but the type is never explicitly referenced in this file.Apply this diff:
-import com.github._1c_syntax.bsl.languageserver.context.symbol.VariableSymbol;
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
276-280: Consider strengtheningtestStructureWithMetadata.The test currently only asserts
diagnosticsis not null, which is a minimal check that doesn't verify any specific behavior related to structure metadata. Consider either:
- Adding assertions that verify diagnostic behavior when structures lack metadata (e.g., whether diagnostics are generated or suppressed), or
- Removing the test if it's a placeholder
Example enhancement:
@Test void testStructureWithMetadata() { List<Diagnostic> diagnostics = getDiagnostics(); - assertThat(diagnostics).isNotNull(); + // When structure metadata is missing, diagnostics should still be generated for non-structure contexts + assertThat(diagnostics).hasSize(49); + // Or verify specific line behavior: + // assertHasDiagnosticOnLine(diagnostics, <line_with_undefined_structure>, "Undefined structure variable"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(5 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
DiagnosticMetadata(54-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build
- GitHub Check: Analyse
🔇 Additional comments (11)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (5)
116-140: LGTM! Robust configuration handling.The null checks and validation via
isValidDateStringensure the diagnostic handles edge cases gracefully. The fallback toDEFAULT_AUTHORIZED_DATESwhen the config is empty or null is correct.
191-202: LGTM! Clear decision logic.The method correctly consolidates all suppression checks and returns
true(generate diagnostic) only when none of the exception contexts apply. The null check provides a safe default.
363-375: LGTM! Defensive AST navigation.The null checks and child-count validation ensure safe traversal through the parse tree. This prevents NPE and enforces the expected single-child path constraint.
160-183: LGTM! Clean separation of validation and decision logic.The refactored
visitConstValueclearly separates format validation from the diagnostic decision, delegating toshouldGenerateDiagnostic. This improves maintainability and testability.
475-490: Verify variable resolution scope and document intent.The scope limitation is confirmed:
isVariableOfStructureTypereceives only the variable name string and must use module-level scope viasymbolTree.getModule()(line 478). This means structure variables declared within local procedures or functions cannot be resolved, relying instead on the fallback heuristic check for"структура"or"structure"in the variable's description.The
ExpressionContextis available at the entry point (isStructureMethodCallline 422) but is converted to text and lost by the time scope resolution occurs. If intentional, add a comment explaining that only module-level structure assignments are suppressed. If not intentional, thread theExpressionContextthrough the call chain and useSymbolTree.getMethodSymbol()or equivalent to resolve the proper scope.src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (6)
27-27: LGTM!Collectionsimport is correctly used.The import is utilized in
testConfigurationEdgeCasesat line 219 for creating an empty configuration map.
71-100: LGTM! Well-designed test helpers.The helper methods
hasDiagnosticOnLine,assertNoDiagnosticOnLine, andassertHasDiagnosticOnLineprovide clear, reusable utilities for line-specific diagnostic assertions with descriptive error messages. This improves test maintainability.
106-129: LGTM! Comprehensive baseline test.The test establishes a baseline of 49 diagnostics and verifies specific ranges. This ensures the diagnostic behavior is stable across changes.
135-154: LGTM! Tests configuration with edge cases.The test verifies that authorized dates correctly suppress diagnostics (49 → 26) and that the configuration parser handles whitespace and empty strings robustly.
171-182: LGTM! Comprehensive structure suppression test.The test verifies the PR's core feature—suppressing diagnostics for structure property assignments and method calls—including bilingual support (
InsertvsВставить). The assertions clearly demonstrate the expected suppression behavior.
217-243: LGTM! Thorough edge case coverage.The test validates configuration handling for empty maps, null configuration, empty/null
authorizedDatesstring, and mixed valid/invalid dates. The assertions confirm the diagnostic maintains stable behavior (49 diagnostics) and correctly applies valid dates from mixed input.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
210-230: Consider adding explicit lower bound checks for time components.While
parseIntreturns 0 on error (preventing negative values), adding explicit>= 0checks forhh,mm, andsswould make the validation more defensive and self-documenting:- return hh <= MAX_HOUR && mm <= MAX_MINUTE && ss <= MAX_SECOND; + return (hh >= 0 && hh <= MAX_HOUR) && (mm >= 0 && mm <= MAX_MINUTE) && (ss >= 0 && ss <= MAX_SECOND);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
DiagnosticMetadata(40-164)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
DiagnosticMetadata(54-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build
- GitHub Check: Analyse
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
🔇 Additional comments (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (6)
45-66: LGTM! Excellent documentation.The comprehensive Javadoc clearly explains the diagnostic purpose, supported formats, and suppression contexts. This aligns perfectly with the PR objective to document when diagnostics should not fire for structure-related operations.
94-108: LGTM! Well-structured constants.The bilingual structure method pattern and date validation constants improve code maintainability and address the bilingual support requirement from past reviews.
122-140: LGTM! Robust configuration handling.The null checks and validation filtering ensure that only valid date strings are accepted during configuration, preventing runtime issues from malformed config.
160-202: LGTM! Clean validation and decision logic.The visitor correctly validates string dates and delegates the suppression decision to
shouldGenerateDiagnostic, which properly implements the PR's objective to skip diagnostics for structure property assignments and method calls.
292-367: LGTM! Well-structured context detection.The context checking methods correctly identify structure-related contexts and exclude them from diagnostics. The
navigateToParenthelper provides defensive AST traversal by validating single-child nodes at each level, ensuring we're in the expected syntactic context.
376-478: LGTM! Comprehensive structure detection.The structure detection logic correctly:
- Identifies structure property assignments via AST analysis
- Validates structure method calls using bilingual pattern matching
- Resolves variable types via SymbolTree with heuristic description checking
The implementation properly addresses the PR objective to suppress false positives for structure-related operations.
johnnyshut
force-pushed
the
deature/3496
branch
from
7bb70ca to
848d374
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
138-155: Проверка времени допускает 24:00:00 и 60 минут/секундВ
isValidDate(String)время валидируется условием:return hh <= 24 && mm <= 60 && ss <= 60;Такой диапазон позволяет некорректные значения (24 часа, 60 минут/секунд). Логичнее (и ближе к реальному диапазону времени) использовать строгие верхние границы:
- return hh <= 24 && mm <= 60 && ss <= 60; + return (hh >= 0 && hh < 24) && (mm >= 0 && mm < 60) && (ss >= 0 && ss < 60);
61-63: PARAM_PATTERN излишне ограничивает диапазон годов и дублирует проверку isValidDateСейчас регекс разрешает годы только с первой цифрой 0–3 (
"[0123]{1}\\d{7}"), так что строки с датами40000101,99991231и т.п. считаются «не датой» и вообще не анализируются, хотя ниже есть полноценная проверка вisValidDate(String). Имеет смысл сделать шаблон более широким (например, просто\\d{8}и\\d{14}) и переложить проверку диапазона года и корректности даты наisValidDate, как она уже делает сейчас.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
438-451: checkInsideStructurePropertyAssignment подавляет диагностику для любого присваивания свойства, не только структурыСейчас метод проверяет только синтаксис:
return lValue != null && lValue.getChildCount() > 0 && lValue.getChild(0) instanceof BSLParser.AccessPropertyContext;То есть любое выражение вида
Объект.Поле = ...считается «присваиванием свойства структуры» и приводит к подавлению диагностики. Это ровно тот случай, за который раньше уже прилетал комментарий (ложное подавление дляОтборЭлемента.ПравоеЗначение = Дата(...)и прочих не‑Структура объектов) — теперь проблема переехала в базовый класс и влияет и на MagicNumber, и на MagicDate.Имеет смысл сузить условие до настоящих структур, используя доступный семантический анализ:
- через
documentContext.getSymbolTree()иDiagnosticHelper.isStructureType(...) / isFixedStructureType(...),- либо переиспользуя уже существующую логику определения типа переменной, чтобы убедиться, что левая часть действительно ссылается на
Структура/Structure.Без такой проверки мы теряем диагностирование дат/чисел в обычных полях произвольных объектов.
🧹 Nitpick comments (5)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractDiagnosticTest.java (1)
42-73: Generic-based constructor works for direct subclasses; consider future-proofingThe no-arg constructor correctly infers
diagnosticClassfor direct subclasses (FooDiagnosticTest extends AbstractDiagnosticTest<FooDiagnostic>), and keeps the explicitClass<T>constructor as a fallback.If you ever introduce intermediate base test classes in this hierarchy,
getGenericSuperclass()may no longer be the parameterizedAbstractDiagnosticTest, and this will start throwing. Not a problem today, but you might later want to walk up the superclass chain until you find theAbstractDiagnosticTestparameterization and includegetClass().getName()in the exception message for easier debugging.docs/diagnostics/MagicDate.md (1)
8-15: Minor RU wording tweak for consistencyThe semantics of the “Диагностика не срабатывает для” list are correct, but the bullet
- Return statement (например, \Возврат '20250101'`)`mixes English and Russian. Consider rephrasing in Russian, for example:
- Оператора `Возврат` (например, `Возврат '20250101'`)to keep the locale consistent.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java (1)
24-93: Tests align with new configuration and suppression behavior; consider explicit negative casesThe updated tests correctly:
- Rely on the new generic-based base class constructor.
- Reconfigure via
authorizedNumbersandallowMagicIndexesand assert updated diagnostic counts/ranges.To make the new behavior less implicit, you could add explicit assertions that there are no diagnostics for:
- structure/correspondence method calls like
Структура.Вставить("Ключ", 20); and- structure property assignments,
using the existing BSL fixture (e.g., by asserting that ranges for those lines are absent, if the assertion helper supports that). This would guard the main regression fix from future refactors.
Based on learnings
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
24-149: MagicNumber logic matches the new behavior; tiny readability tweak possibleThe refactoring looks consistent with the stated goals:
visitNumericnow:
- Filters out authorized numbers via
isAllowed.- Skips any numeric inside a
DefaultValueContext, as required.- Delegates to
isWrongExpressionotherwise.
isWrongExpression:
- Uses
mayBeNumberAccess(ctx)to respectallowMagicIndexes.- Suppresses diagnostics when
insideStructureOrCorrespondence(...)is true, covering both structures and correspondences.- Still treats literals in return expressions as magic numbers (
insideReturnStatement), which matches the docs for MagicNumber.As a micro cleanup,
isAllowedcan be simplified without changing semantics:- private boolean isAllowed(String s) { - for (String elem : this.authorizedNumbers) { - if (s.compareTo(elem) == 0) { - return false; - } - } - return true; - } + private boolean isAllowed(String s) { + return !authorizedNumbers.contains(s); + }This relies on the fact that
authorizedNumbersstores trimmed literal strings already, so the equality semantics remain the same.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
89-105: insideStructureOrCorrespondence: хорошо, но стоит явно документировать приоритет проверокКомбинация из пяти проверок (
Insertструктуры, конструктор структуры, присваивание свойству,Insertсоответствия по значению и по ключу) сейчас зашита простым||. Для читаемости и обслуживания (особенно с учётом AST-анализа типов) было бы полезно рядом в JavaDoc коротко описать приоритет/назначение каждой ветки и явно привести примеры кода, для которых каждая из них срабатывает.Это не баг, скорее вопрос удобства сопровождения, особенно учитывая, что этот базовый класс теперь используется несколькими диагностиками.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (11)
docs/diagnostics/MagicDate.md(2 hunks)docs/diagnostics/MagicNumber.md(2 hunks)docs/en/diagnostics/MagicDate.md(1 hunks)docs/en/diagnostics/MagicNumber.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractDiagnosticTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/diagnostics/MagicNumber.md
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-07-04T19:35:42.937Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:2-4
Timestamp: 2024-07-04T19:35:42.937Z
Learning: Test resources in the repository may contain code that intentionally triggers diagnostics to demonstrate their functionality.
Applied to files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2025-10-27T08:05:10.072Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3555
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java:163-166
Timestamp: 2025-10-27T08:05:10.072Z
Learning: В проекте bsl-language-server используется собственная версия ANTLR4, в которой интерфейс TerminalNode переопределяет метод getParent() с возвращаемым типом RuleNode вместо ParseTree (как в стандартной версии ANTLR4).
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2024-07-04T19:35:26.747Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The file `DoubleNegativesDiagnostic.bsl` is a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
🔇 Additional comments (4)
docs/en/diagnostics/MagicNumber.md (1)
8-12: Non-trigger list matches implementation and configurationThe new “The diagnostic does not trigger for” bullets correctly describe current behavior: default parameter values, structure/correspondence contexts, and index usages controlled via
allowMagicIndexes. This keeps the doc in sync withMagicNumberDiagnostic.configureandmayBeNumberAccess.docs/en/diagnostics/MagicDate.md (1)
8-15: Clear enumeration of MagicDate non-trigger casesThe added list of contexts where MagicDate does not fire (default parameter values, structures/correspondences, simple assignments, returns, and
Date()assignments) is precise and matches the diagnostic’s intended filters. Good alignment between documentation and behavior.src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java (1)
83-85: isCorrespondenceType is consistent with existing helpersThe new
isCorrespondenceTypemirrors the existingisStructureType/isFixedStructureTypehelpers (simpleequalsIgnoreCasechecks for localized and English names), which is exactly what the shared magic-value logic needs for correspondences.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
123-127: Verify whether the suppression logic inshouldAddDiagnosticis intentional or over-suppressingThe review concern is valid: the current logic suppresses diagnostics for all dates inside simple assignments and return statements, regardless of context. This is broader than the specific
insideAssignmentWithDateMethodForSimpleDate()check.Test data file not accessible to verify test coverage for the specific cases mentioned (
Перем ДатаНачала = Дата("20200101");andВозврат Дата("20200101");).Verify manually that:
- The 17 diagnostics currently reported by the test represent the intended behavior
- Test cases cover both scenarios you want to flag and scenarios you want to suppress
- The broad suppression in lines 126-127 (checking
insideSimpleAssignmentandinsideReturnStatement) matches the original MagicDate diagnostic intentIf simple assignments and returns should be flagged, consider renaming
shouldAddDiagnostictoshouldSkipDiagnosticand inverting the condition logic for clarity.
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
196-226: getVariableTypeFromAST всё ещё уязвим к NPE/IndexOutOfBounds при отсутствии присваивания или нестандартном выраженииСейчас код предполагает, что:
findVariableAssignment(...)всегда что‑то найдёт,- у найденного
assignmentвсегда естьexpression(),expression.member(0)всегда существует,и сразу делает:
var assignment = findVariableAssignment(callStatement, variableName); var expression = assignment.expression(); var firstMember = expression.member(0);Это ломается, если:
- переменная параметр процедуры/функции, глобалка или инициализируется в другом блоке/ветке —
assignmentбудетnull;- присваивание не через
Новый <Тип>/нетexpression()—assignment.expression()может бытьnull;- в
expressionнет ни одногоmember()—member(0)кидаетIndexOutOfBoundsException.В результате любая диагностика, использующая этот метод, может падать на вполне валидном BSL‑коде.
Предлагаю обезопасить метод ранними выходами (и заодно защититься от отсутствия IDENTIFIER):
private Optional<Boolean> getVariableTypeFromAST(BSLParser.CallStatementContext callStatement) { - var identifier = callStatement.IDENTIFIER(); - var variableName = identifier.getText(); - var assignment = findVariableAssignment(callStatement, variableName); - var expression = assignment.expression(); - - var firstMember = expression.member(0); + var identifier = callStatement.IDENTIFIER(); + if (identifier == null) { + return Optional.empty(); + } + + var variableName = identifier.getText(); + var assignment = findVariableAssignment(callStatement, variableName); + if (assignment == null || assignment.expression() == null) { + return Optional.empty(); + } + + var expression = assignment.expression(); + if (expression.member() == null || expression.member().isEmpty()) { + return Optional.empty(); + } + + var firstMember = expression.member(0); if (firstMember == null || firstMember.complexIdentifier() == null) { return Optional.empty(); }Такая защита делает анализ устойчивым к любым «нестандартным» случаям и гарантирует, что диагностика максимум вернёт
empty, но не упадёт.
🧹 Nitpick comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (3)
267-313: Поиск присваивания в блоке берёт первое, а не ближайшее предыдущее — можно улучшить точность анализа
findAssignmentInCodeBlockиfindAssignmentInStatementвозвращают первое подходящее присваивание до нужной позиции:for (var statement : codeBlock.statement()) { ... var assignment = findAssignmentInStatement(statement, beforeLine, beforeChar, variableName); if (assignment != null) { return assignment; } }Это значит, что при нескольких присваиваниях одной переменной в блоке будет выбрано самое раннее, а не «последнее перед вызовом». В случаях:
Стр = Новый Структура; ... Стр = 0; // переопределили тип Стр.Вставить("Поле", 20);анализ всё равно решит, что переменная — структура, хотя последнее присваивание уже другое.
Функционально это не баг (просто более оптимистичная эвристика), но для повышения точности можно пройти блок полностью и помнить «последнее подходящее» присваивание:
- for (var statement : codeBlock.statement()) { - ... - var assignment = findAssignmentInStatement(statement, beforeLine, beforeChar, variableName); - if (assignment != null) { - return assignment; - } - } - - return null; + BSLParser.AssignmentContext lastAssignment = null; + for (var statement : codeBlock.statement()) { + if (statement == null) { + continue; + } + + var assignment = findAssignmentInStatement(statement, beforeLine, beforeChar, variableName); + if (assignment != null) { + lastAssignment = assignment; + } + } + + return lastAssignment;Это сохранит текущую сложность, но даст более реалистичный тип переменной.
359-403: checkInsideStructureConstructor: стоит чуть усилить проверку первого параметра для робастностиЛогика в целом корректная: поднимаемся до
CallParamContext, убеждаемся, что этоНовый Структура/ФиксированнаяСтруктура, пропускаем первый параметр‑строку и считаем остальные параметрами значений.Единственное хрупкое место — доступ к
firstParam.getTokens().get(0):var firstParam = callParams.get(0); var firstToken = firstParam.getTokens().get(0); ... return firstToken.getType() == BSLParser.STRING;Если по каким‑то причинам первый параметр пустой/разобран атипично, возможен
IndexOutOfBoundsException. Вероятность небольшая, но добавить защиту почти ничего не стоит:- var firstParam = callParams.get(0); - var firstToken = firstParam.getTokens().get(0); + var firstParam = callParams.get(0); + var tokens = firstParam.getTokens(); + if (tokens == null || tokens.isEmpty()) { + return false; + } + var firstToken = tokens.get(0);Так метод станет безопаснее на любых нестандартных деревьях, не меняя основную логику.
411-422: checkInsideStructurePropertyAssignment трактует любое присваивание свойству как «структуру» — убедитесь, что это ожидаемое расширениеСейчас
checkInsideStructurePropertyAssignmentпроверяет только факт присваивания вlValue, где первый ребёнок —AccessPropertyContext:return lValue != null && lValue.getChildCount() > 0 && lValue.getChild(0) instanceof BSLParser.AccessPropertyContext;Это покрывает
МояСтруктура.МоёПоле = 20;, но также и любые другие объекты с точечной нотацией (Объект.Сумма = 20;,ЭтотОбъект.Поле = 20;и т.п.). Фактически метод рассматривает любое присваивание свойству как «структуру с говорящим именем поля» и отключает диагностику.Если вы действительно хотите обобщить поведение MagicNumber/MagicDate на все объекты (а не только
Структура/ФиксированнаяСтруктура), то всё ок. Если же требования — именно про структуры, можно дополнительно ограничить проверку по типу (по аналогии сgetVariableTypeFromAST), чтобы не отключать диагностику на произвольных объектах.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build
🔇 Additional comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java (1)
108-110: Новый isCorrespondenceType реализован корректно и согласован с остальными type‑helper’амиРеализация симметрична
isStructureType/isFixedStructureType, использует те же приёмы (getText(), case‑insensitive сравнение) и не добавляет новых рисков.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
89-105: insideStructureOrCorrespondence аккуратно агрегирует все частные проверкиКомпозиция из частных проверок (
Insert, конструктор структуры, присваивание свойству,Insertдля соответствий) выглядит аккуратно и расширяема, при этом каждая ветка обёрнута вOptional.map(...).orElse(false), что даёт безопасное поведение при отсутствииexpression.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (6)
50-75: Consider documenting the expected AST structure.Both
getExpressionmethods assume a specific AST hierarchy with exactly one child at each level. While returningOptional.empty()on mismatch is safe for a diagnostic, consider adding inline comments or defensive logging to clarify the expected structure, which would help maintainers understand why the traversal might fail if the parser grammar evolves.
167-176: Extract magic number to a named constant.The iteration limit
10on Line 169 should be extracted to a descriptive constant likeMAX_CALL_STATEMENT_TRAVERSAL_DEPTHto improve maintainability and make the constraint explicit.Apply this diff:
+ private static final int MAX_CALL_STATEMENT_TRAVERSAL_DEPTH = 10; + /** * Находит CallStatementContext для данного MethodCallContext. * * @param methodCall контекст вызова метода * @return CallStatementContext или null, если не найден */ private BSLParser.CallStatementContext findCallStatement(BSLParser.MethodCallContext methodCall) { var current = methodCall.getParent(); - for (int i = 0; i < 10 && current != null; i++) { + for (int i = 0; i < MAX_CALL_STATEMENT_TRAVERSAL_DEPTH && current != null; i++) { if (current instanceof BSLParser.CallStatementContext callStatementContext) { return callStatementContext; } current = current.getParent(); } return null; }
219-228: Replace Object array with a type-safe record or class.Returning
Object[]fromgetCallParamInforequires callers to manually cast elements (see Lines 505, 506, 555, 560, 561), which is error-prone and bypasses compile-time type safety.Consider defining a record at the class level:
private record CallParamInfo( BSLParser.CallParamContext context, int index, BSLParser.CallParamListContext listContext ) {}Then refactor the method:
- private Object[] getCallParamInfo(BSLParser.ExpressionContext expression) { + private Optional<CallParamInfo> getCallParamInfo(BSLParser.ExpressionContext expression) { var callParam = expression.getParent(); if (callParam instanceof BSLParser.CallParamContext callParamContext) { var callParamList = (BSLParser.CallParamListContext) callParamContext.getParent(); var callParams = callParamList.callParam(); var paramIndex = callParams.indexOf(callParamContext); - return new Object[]{callParamContext, paramIndex, callParamList}; + return Optional.of(new CallParamInfo(callParamContext, paramIndex, callParamList)); } - return new Object[0]; + return Optional.empty(); }And update callers to use the record's accessors instead of array indexing.
283-304: Extract magic number to a named constant.The iteration limit
50on Line 285 should be extracted to a descriptive constant likeMAX_VARIABLE_ASSIGNMENT_TRAVERSAL_DEPTHto improve maintainability.Apply this diff:
+ private static final int MAX_VARIABLE_ASSIGNMENT_TRAVERSAL_DEPTH = 50; + /** * Находит AssignmentContext для переменной с указанным именем. * Ищет в пределах того же блока кода (CodeBlockContext), поднимаясь по AST вверх. * * @param startNode начальный узел для поиска * @param variableName имя переменной * @return AssignmentContext или null, если не найден */ private BSLParser.AssignmentContext findVariableAssignment(BSLParserRuleContext startNode, String variableName) { var current = startNode.getParent(); - for (int i = 0; i < 50 && current != null; i++) { + for (int i = 0; i < MAX_VARIABLE_ASSIGNMENT_TRAVERSAL_DEPTH && current != null; i++) { if (current instanceof BSLParser.AssignmentContext assignmentContext) { var lValue = assignmentContext.lValue(); if (lValue != null) { var identifier = lValue.IDENTIFIER(); if (identifier != null && identifier.getText().equalsIgnoreCase(variableName)) { return assignmentContext; } } } current = current.getParent(); }
406-419: Extract magic number to a named constant.The iteration limit
5on Line 409 should be extracted to a descriptive constant likeMAX_CALL_PARAM_TRAVERSAL_DEPTHfor consistency with other traversal limits.Apply this diff:
+ private static final int MAX_CALL_PARAM_TRAVERSAL_DEPTH = 5; + /** * Проверяет, находится ли выражение в конструкторе структуры. * Параметры конструктора структуры: первый - строка с именами полей, остальные - значения. * * @param expression выражение для проверки * @return true, если выражение находится в конструкторе структуры */ private boolean checkInsideStructureConstructor(BSLParser.ExpressionContext expression) { var current = expression.getParent(); BSLParser.CallParamContext callParamContext = null; - for (int i = 0; i < 5 && current != null; i++) { + for (int i = 0; i < MAX_CALL_PARAM_TRAVERSAL_DEPTH && current != null; i++) { if (current instanceof BSLParser.CallParamContext paramContext) { callParamContext = paramContext; break; } current = current.getParent(); }
477-490: Extract magic number to use the same constant.The iteration limit
5on Line 483 should use the same constant suggested for Line 409 (MAX_CALL_PARAM_TRAVERSAL_DEPTHor a more generalMAX_PARENT_TRAVERSAL_DEPTH) to maintain consistency across similar traversal patterns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build
- GitHub Check: Benchmark
- GitHub Check: Analyse
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
🔇 Additional comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (5)
89-105: LGTM!The method clearly delegates to specific context checkers and properly handles Optional chaining with
orElse(false). The comprehensive JavaDoc matches the implementation.
113-145: LGTM!The logic correctly identifies the second parameter of structure's
Insertmethod by negating the correspondence check on Line 141, ensuring that only structure insertions (not correspondence) are flagged.
237-273: LGTM! The NPE issue has been fixed.The defensive null and empty checks added on Lines 241-247 and 250-252 address the critical issue from the previous review. The method now safely returns
Optional.empty()when the variable assignment or AST structure is incomplete, preventing NPE and IndexOutOfBoundsException.
499-552: LGTM!The correspondence detection logic correctly identifies both the first parameter (key) and second parameter (value) of the
Insertmethod for correspondence types by checkingvariableType.get()returns true (Lines 520, 548). This appropriately allows magic values in both positions since the method name provides context.
578-605: LGTM!The generic context checking utilities provide a clean, reusable abstraction with proper use of Optional filtering. The
insideContextmethod effectively eliminates code duplication for common context checks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
61-63: Неконсистентная валидация года и времени в PARAM_PATTERN / isValidDateСейчас есть два расхождения:
PARAM_PATTERNжёстко ограничивает первую цифру года[0123], тогда какisValidDate(String)разрешает годы доMAX_YEAR_BY_1C(9999). В результате строки с годом4000–9999вообще не проходят валидацию и никогда не анализируются как дата.- Проверка времени допускает значения
hh <= 24 && mm <= 60 && ss <= 60, что делает валидными строки с заведомо невозможным временем (24:00:00,12:60:00,12:00:60).Это даёт и ложные отрицания (валидные даты с годом ≥4000 игнорируются), и ложные положительные (некорректное время считается валидным шаблоном даты для диагностики).
Предлагаю:
- Разрешить любую цифру в году, а границы года оставить на ответственности
isValidDate:- Ужесточить границы времени до полуинтервала.
- private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( - "\"[0123]{1}\\d{7}\"|\"[0123]{1}\\d{13}\"" - ); + private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( + "\"\\d{8}\"|\"\\d{14}\"" + ); @@ - var hh = parseInt(strDate.substring(8, 10)); - var mm = parseInt(strDate.substring(10, 12)); - var ss = parseInt(strDate.substring(12, 14)); - return hh <= 24 && mm <= 60 && ss <= 60; + var hh = parseInt(strDate.substring(8, 10)); + var mm = parseInt(strDate.substring(10, 12)); + var ss = parseInt(strDate.substring(12, 14)); + return (hh >= 0 && hh < 24) && (mm >= 0 && mm < 60) && (ss >= 0 && ss < 60);Also applies to: 138-155
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
89-108: Подавление поcheckInsideStructurePropertyAssignmentсейчас срабатывает для любых свойств, не только структур
insideStructureOrCorrespondence(...)используетcheckInsideStructurePropertyAssignment, который проверяет только форму lValue:var lValue = assignment.lValue(); return lValue != null && lValue.getChildCount() > 0 && lValue.getChild(0) instanceof BSLParser.AccessPropertyContext;Т.е. любая запись вида
Объект.Свойство = 42(не обязательно структура/соответствие) попадает под “внутри структуры/соответствия” и выкидывается из диагностики MagicNumber/MagicDate. Это существенно шире описанной в PR цели про “свойства структуры” и повторяет по сути ранее найденную проблему в MagicNumber (когда подавлялись все dotted‑lvalue).Если это не намеренное расширение области подавления, стоит:
- Либо сузить проверку, дополнив её определением типа получателя (по аналогии с тем, как вы делаете для
ВставитьчерезgetVariableTypeFromAST), и возвращатьtrueтолько если левый объект — структура или фиксированная структура;- Либо, как минимум, явно задокументировать, что теперь любые объектные свойства считаются контекстом, дающим смысл числам/датам.
Вариант AST‑базированной проверки типа:
- Выделить из lValue “базовый” идентификатор (до первого
.),- Найти для него присваивание
Новый Структура/...так же, как вgetVariableTypeFromAST,- Использовать уже существующие
DiagnosticHelper.isStructureType/isFixedStructureTypeи только в этом случае считать контекст структурным.Also applies to: 511-522
🧹 Nitpick comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
96-121: Дублирование проверкиDefaultValueContextмежду MagicDate и MagicNumber
isInsideDefaultValue(ConstValueContext)реализует обход предков в поискеDefaultValueContext. ВMagicNumberDiagnostic.visitNumericесть практически идентичный ручной обход.Имеет смысл вынести эту проверку в общий helper (например, в
AbstractMagicValueDiagnostic) и переиспользовать в обоих диагностиках, чтобы не разъезжалась логика обработки значений по умолчанию и не приходилось править две реализации.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
108-115: Небольшие дублирования/стиль: default-value и доступ по индексуЕсть пара моментов, которые можно слегка подчистить:
- Проверка на
DefaultValueContextвvisitNumericдублирует идеюisInsideDefaultValueизMagicDateDiagnostic. Имеет смысл вынести общий helper вAbstractMagicValueDiagnosticи переиспользовать его в обоих классах.mayBeNumberAccess(NumericContext ctx)уже поднимается по дереву доAccessIndexContext, так что дальше вisWrongExpressionусловие!isNumericExpression(expr) || insideCallParam(expr)можно читать как чисто “нуменические выражения вне индексов/структур/return/параметров” — возможно, стоит добавить короткий комментарий, чтобы было очевидно, что индексы обрабатываются раньше.Это не блокеры, а косметика для читаемости и единообразия.
Also applies to: 135-147
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (2)
240-254: Optional для типа переменной ухудшает читаемость
getVariableTypeFromAST/determineVariableTypeкодируют тип переменной какOptional<Boolean>:
Optional.of(true)— соответствие,Optional.of(false)— структура.Дальше по коду приходится ментально раскодировать
variableType.get()/!variableType.get()и понимать, что именно означает булево значение, что затрудняет сопровождение.Рассмотрите вариант заменить это на маленький enum, например:
enum StructureKind { STRUCTURE, CORRESPONDENCE }и возвращать
Optional<StructureKind>. Тогда места использования (вродеcheckInsideStructureInsertOrAdd/checkInsideCorrespondenceInsert) будут существенно самодокументируемее.Also applies to: 287-295
222-231: ИспользованиеObject[]в getCallParamInfo ухудшает типобезопасность
getCallParamInfoвозвращает “сырую” тройкуObject[]{callParamContext, paramIndex, callParamList}, далее по индексу распаковываем разные типы (CallParamContext,Integer,CallParamListContext).Это работает, но:
- Нарушает типобезопасность (логика завязана на порядковые индексы и касты),
- Усложняет чтение кода в местах использования (
(Integer) callParamInfo[1],(BSLParser.CallParamListContext) callParamInfo[2]).Можно слегка улучшить читаемость и безопасность, введя простой вспомогательный класс/record:
private static final class CallParamInfo { final BSLParser.CallParamContext param; final int index; final BSLParser.CallParamListContext list; // ctor, getters... }и возвращая
Optional<CallParamInfo>вместоObject[]. Это необязательная правка, но заметно уменьшит “магические индексы” в этом участке.Also applies to: 552-605
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-10-27T08:05:10.072Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3555
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java:163-166
Timestamp: 2025-10-27T08:05:10.072Z
Learning: В проекте bsl-language-server используется собственная версия ANTLR4, в которой интерфейс TerminalNode переопределяет метод getParent() с возвращаемым типом RuleNode вместо ParseTree (как в стандартной версии ANTLR4).
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2024-07-04T19:35:26.747Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The file `DoubleNegativesDiagnostic.bsl` is a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build
- GitHub Check: Benchmark
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
87-105: Логика ранних выходов в visitConstValue соответствует целям PRПоследовательность проверок в
visitConstValueвыглядит корректно и хорошо согласована с новой базой:
- Фильтрация по
isAcceptedиisValidDateдля строк.- Ранний выход для значений по умолчанию параметров (
isInsideDefaultValue).- Ранний выход для всех контекстов структуры/соответствия через
insideStructureOrCorrespondence(expressionContext)— это как раз устраняет FPs, описанные в PR.- Дополнительный гард
shouldAddDiagnostic(простые присваивания,return, вызовыДата/Date) повторяет прежнюю семантику MagicDate.С точки зрения поведения диагностики к этому блоку вопросов нет.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
86-106: Логика MagicNumber поверх новой базовой абстракции выглядит корректнойНесколько моментов по текущему поведению:
- Обработка default-значений через восхождение к
DefaultValueContextдо вызоваisWrongExpressionкорректно исключает параметры с дефолтами из диагностики.isWrongExpressionсначала отрабатываетmayBeNumberAccess(ctx)и аккуратно ловит индексы (AccessIndexContext) независимо от формы выражения — это чуть шире прежней проверки по непосредственному parent и выглядит более устойчиво.- Далее используется общий
getExpression(...)иinsideStructureOrCorrespondence(expression), что централизует подавление диагностики для структур/соответствий и соответствует целям PR.- Обработка
insideReturnStatement(expression)как “всегда неправильного” выражения осознанно ужесточает диагностику:Возврат 5;теперь будет считаться магическим числом (кроме явно разрешённых-1, 0, 1). Если это ожидаемое поведение — реализация ок, но это стоит держать в голове как изменение semantics относительно старого варианта.В целом, связка
visitNumeric+isWrongExpressionвыглядит консистентной и хорошо использует новый базовый класс.Also applies to: 117-133
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
305-321: Поиск присваивания переменной выглядит аккуратным и с разумными ограничителямиЛогика
findVariableAssignment/findAssignmentInCodeBlock/findAssignmentInStatementаккуратно:
- Ограничивает подъём по AST (
for (int i = 0; i < 50 && current != null; i++)),- Ограничивает поиск в блоке только statement’ами, чьи присваивания находятся до позиции использования (
isAssignmentBeforePosition),- Сравнивает имена переменных с учётом регистронезависимости (
equalsIgnoreCase).Для целей эвристического определения типа структуры/соответствия это выглядит достаточно безопасно и не даёт очевидных проблем по производительности на типичных модулях.
Also applies to: 331-377
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (4)
116-148: Add defensive checks for AST structure assumptions.Lines 128 and 136 perform unchecked casts assuming the parent nodes are always
CallParamListContextandDoCallContextrespectively. While this matches the expected BSL grammar structure, addinginstanceofchecks would make the code more resilient to unexpected AST variations or future grammar changes.Apply this diff to add defensive checks:
private boolean isSecondParameterOfStructureInsert(BSLParser.CallParamContext callParam) { - var callParamList = (BSLParser.CallParamListContext) callParam.getParent(); + var parent = callParam.getParent(); + if (!(parent instanceof BSLParser.CallParamListContext callParamList)) { + return false; + } + var callParams = callParamList.callParam(); var paramIndex = callParams.indexOf(callParam); if (paramIndex != 1) { return false; } - var doCall = (BSLParser.DoCallContext) callParamList.getParent(); + var listParent = callParamList.getParent(); + if (!(listParent instanceof BSLParser.DoCallContext doCall)) { + return false; + } + var methodCall = isInsertMethod(doCall);
305-414: LGTM!The variable assignment search logic is well-structured with appropriate null checks and bounded iteration limits to prevent infinite loops. The case-insensitive variable name comparison correctly handles BSL's semantics.
Optional improvement: Consider extracting the magic number
50on line 307 to a named constant likeMAX_AST_TRAVERSAL_DEPTHfor better maintainability.
460-475: Add defensive checks for AST structure assumptions.Lines 461-462 perform unchecked casts to
CallParamListContextandDoCallContext. Similar to the earlier issue on lines 128 and 136, addinginstanceofchecks would improve resilience.Apply this diff to add defensive checks:
private static BSLParser.TypeNameContext getStructureTypeName(BSLParser.CallParamContext callParamContext) { - var callParamList = (BSLParser.CallParamListContext) callParamContext.getParent(); - var doCall = (BSLParser.DoCallContext) callParamList.getParent(); - var newExpression = doCall.getParent(); + var parent = callParamContext.getParent(); + if (!(parent instanceof BSLParser.CallParamListContext callParamList)) { + return null; + } + + var listParent = callParamList.getParent(); + if (!(listParent instanceof BSLParser.DoCallContext doCall)) { + return null; + } + + var newExpression = doCall.getParent(); if (!(newExpression instanceof BSLParser.NewExpressionContext)) { return null; }
552-622: Add defensive checks for repeated AST cast patterns.Lines 564-565, 591-593, and 619-620 all perform unchecked casts to
CallParamListContextandDoCallContext. This pattern repeats throughout the file.Consider either:
- Adding
instanceofchecks before each cast (as shown in earlier comments), or- Creating a helper method to safely navigate this common AST pattern:
private static Optional<BSLParser.DoCallContext> getDoCallFromCallParam(BSLParser.CallParamContext callParam) { var parent = callParam.getParent(); if (!(parent instanceof BSLParser.CallParamListContext callParamList)) { return Optional.empty(); } var listParent = callParamList.getParent(); if (!(listParent instanceof BSLParser.DoCallContext doCall)) { return Optional.empty(); } return Optional.of(doCall); }Then use it throughout the file:
var doCall = getDoCallFromCallParam(callParamContext).orElse(null); if (doCall == null) { return false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build
- GitHub Check: Analyse
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
🔇 Additional comments (8)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (8)
1-41: LGTM!The file header, imports, and class declaration are well-structured. The case-insensitive pattern for matching "Вставить/Insert" method names is appropriate for the BSL language context.
50-75: LGTM!Both
getExpressionoverloads use safe Optional chaining with appropriate filters to verify AST structure before casting. The defensive checks ensure that unexpected AST shapes return empty rather than throwing exceptions.
89-108: LGTM!The orchestration logic is clear and efficient, delegating to specific check methods and short-circuiting on the first match. The comprehensive Javadoc accurately documents all scenarios covered.
156-199: LGTM!These helper methods demonstrate good defensive programming with appropriate
instanceofchecks and bounded iteration to prevent infinite loops. The pattern-based method name matching correctly handles BSL's case-insensitive nature.
240-254: LGTM! Past issue properly addressed.The defensive null checks added on lines 244-246 correctly address the NPE/IndexOutOfBounds risk flagged in the previous review. The method now safely returns
Optional.empty()when the assignment or expression is null, rather than throwing an exception.
262-279: LGTM!Excellent defensive programming with appropriate null checks and bounds checking before accessing the member list. The method safely returns null when unable to extract the type name from the expression.
511-543: LGTM!The property assignment detection logic includes appropriate null checks and child count verification before accessing the AST structure. The bounded loop in
findAssignmentprevents potential infinite traversal.
631-658: LGTM!The generic context-checking utilities demonstrate good abstraction and code reuse. The use of Optional chaining with type filters provides safe and concise AST context detection.
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Show resolved
Hide resolved
johnnyshut
force-pushed
the
deature/3496
branch
from
79745a8 to
7e5499d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
61-63: Исправить regex для строковых дат и граничные условия времени вisValidDate.
PARAM_PATTERNсейчас ограничивает первую цифру года диапазоном[0-3], тогда как далее вы явно допускаете годы доMAX_YEAR_BY_1C = 9999. В результате строка"40000101"(и любые годы 4xxx–9xxx) никогда не считается датой, и диагностика для таких валидных дат не сработает.Проверка времени
hh <= 24 && mm <= 60 && ss <= 60допускает недопустимые значения (24:00:00, 12:60:00 и т.п.), помечая их как валидные даты. Логично требоватьhh < 24,mm < 60,ss < 60(при этом вход — только цифры, так что нижняя граница 0 выполнена автоматически, но её можно явно указать для читаемости).Предлагаю такой минимальный патч:
- private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( - "\"[0123]{1}\\d{7}\"|\"[0123]{1}\\d{13}\"" - ); + private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( + "\"\\d{8}\"|\"\\d{14}\"" + ); @@ - var hh = parseInt(strDate.substring(8, 10)); - var mm = parseInt(strDate.substring(10, 12)); - var ss = parseInt(strDate.substring(12, 14)); - return hh <= 24 && mm <= 60 && ss <= 60; + var hh = parseInt(strDate.substring(8, 10)); + var mm = parseInt(strDate.substring(10, 12)); + var ss = parseInt(strDate.substring(12, 14)); + return hh >= 0 && hh < 24 && mm >= 0 && mm < 60 && ss >= 0 && ss < 60;Это оставляет проверку диапазона годов в одном месте (
isValidDate(String)черезMAX_YEAR_BY_1C) и делает фильтрацию строковых представлений более предсказуемой.Для самопроверки можно быстро убедиться, что тесты покрывают эти случаи:
#!/bin/bash # Проверяем, есть ли в тестах примеры с годом >= 4000 и граничным временем 24:00:00 echo "=== Поиск строкового литерала с годом >= 4000 ===" rg -n '"40000101"' src/test/resources/diagnostics/MagicDateDiagnostic.bsl || \ echo 'Нет теста с литералом "40000101"' echo echo "=== Поиск литерала с временем 24:00:00 ===" rg -n '"20250101240000"' src/test/resources/diagnostics/MagicDateDiagnostic.bsl || \ echo 'Нет теста для времени 24:00:00'Also applies to: 138-155
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (2)
88-88: Consider renaming for clarity.The variable name
checkedis ambiguous—it could mean "already checked" or "the value being checked." ConsidernumericTextorvaluefor better readability.- var checked = ctx.getText(); + var numericText = ctx.getText(); - if (checked != null && isAllowed(checked)) { + if (numericText != null && isAllowed(numericText)) { var current = ctx.getParent();
91-99: Consider adding a clarifying comment.The traversal to detect default parameter values is correct, but a brief comment explaining the rationale (parameter names provide context, making magic numbers acceptable) would improve maintainability.
var current = ctx.getParent(); var isDefaultValue = false; + // Skip diagnostic for default parameter values - the parameter name provides context while (current != null) { if (current instanceof BSLParser.DefaultValueContext) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2025-10-27T08:05:10.072Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3555
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java:163-166
Timestamp: 2025-10-27T08:05:10.072Z
Learning: В проекте bsl-language-server используется собственная версия ANTLR4, в которой интерфейс TerminalNode переопределяет метод getParent() с возвращаемым типом RuleNode вместо ParseTree (как в стандартной версии ANTLR4).
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
📚 Learning: 2024-07-04T19:35:26.747Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The file `DoubleNegativesDiagnostic.bsl` is a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build
🔇 Additional comments (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
87-105: Хорошая централизация фильтров контекста вvisitConstValue.Рефакторинг выглядит аккуратно: проверка
isInsideDefaultValue(ctx), далееinsideStructureOrCorrespondence(expressionContext)и финальныйshouldAddDiagnostic(expressionContext)дают понятную последовательность ранних выходов и чётко отражают цель PR — не триггерить диагностику внутри значений по умолчанию и структур/соответствий. Явных логических проблем здесь не вижу, поведение согласовано с новым базовымAbstractMagicValueDiagnostic.Also applies to: 112-127
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (5)
24-30: LGTM!The new imports support the configuration method and list-based parameter handling. All imports are used appropriately.
48-48: Good refactoring to share magic-value detection logic.Extending
AbstractMagicValueDiagnosticallows reuse of structure/correspondence context detection across both MagicNumber and MagicDate diagnostics.
65-76: LGTM!The configuration method correctly handles both
allowMagicIndexesandauthorizedNumbersparameters, clearing the list before repopulation to ensure a clean state.
135-147: LGTM!The array-index detection correctly returns early when
allowMagicIndexesis true, and properly traverses ancestors to detectAccessIndexContextwhen the check is needed.
117-133: Base class implementation verified—all patterns correctly detected.The
AbstractMagicValueDiagnostic.insideStructureOrCorrespondence()method properly handles both property assignments and method calls through five comprehensive checks:
Structure property assignments:
checkInsideStructurePropertyAssignment()identifies them by checking if the assignment's left-value is anAccessPropertyContext(e.g.,Structure.Property = 20).Structure constructor calls:
checkInsideStructureConstructor()validates constructor invocations.Structure.Insert/Add (2nd param):
checkInsideStructureInsertOrAdd()validates the second parameter by checking!variableType.get()(ensuring it's NOT a Correspondence).Correspondence.Insert (2nd param):
checkInsideCorrespondenceInsert()validates by checkingvariableType.get()(ensuring it IS a Correspondence).Correspondence.Insert (1st param):
checkInsideCorrespondenceInsertFirstParam()handles the first parameter case.The logic correctly distinguishes Structure from Correspondence types via
getVariableTypeFromCallStatement(), which returnstruefor Correspondence andfalsefor Structure. TheisWrongExpression()method inMagicNumberDiagnosticcorrectly suppresses diagnostics when inside these contexts, allowing magic numbers where they're expected per 1C semantics.
johnnyshut
force-pushed
the
deature/3496
branch
from
2bfeb16 to
c246ca1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
55-61: Регулярка и проверка времени противоречат заявленной валидации дат
PARAM_PATTERNсейчас ограничивает первый символ года диапазоном[0–3]:private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( "\"[0123]{1}\\d{7}\"|\"[0123]{1}\\d{13}\"" );При этом дальше в
isValidDate(String)вы явно допускаете годы доMAX_YEAR_BY_1C = 9999. В результате строки вида"40000101"или"99991231"никогда не будут распознаны как дата (regex их не пропускает), и диагностика для таких "магических дат" просто не сработает, хотя формально они валидны для 1С.
- В проверке времени:
var hh = parseInt(strDate.substring(8, 10)); var mm = parseInt(strDate.substring(10, 12)); var ss = parseInt(strDate.substring(12, 14)); return hh <= 24 && mm <= 60 && ss <= 60;допускаются значения
24:60:60, что заведомо некорректно для времени. Если цель — действительно валидировать дату/время, разумнее проверять строгие границы< 24,< 60,< 60.Предлагаемая правка (адаптируйте под ваш стиль):
- private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( - "\"[0123]{1}\\d{7}\"|\"[0123]{1}\\d{13}\"" - ); + private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( + "\"\\d{8}\"|\"\\d{14}\"" + ); @@ - var year = parseInt(strDate.substring(0, 4)); - if (year < 1 || year > MAX_YEAR_BY_1C) { + var year = parseInt(strDate.substring(0, 4)); + if (year < 1 || year > MAX_YEAR_BY_1C) { return false; } @@ - var hh = parseInt(strDate.substring(8, 10)); - var mm = parseInt(strDate.substring(10, 12)); - var ss = parseInt(strDate.substring(12, 14)); - return hh <= 24 && mm <= 60 && ss <= 60; + var hh = parseInt(strDate.substring(8, 10)); + var mm = parseInt(strDate.substring(10, 12)); + var ss = parseInt(strDate.substring(12, 14)); + return hh >= 0 && hh < 24 && mm >= 0 && mm < 60 && ss >= 0 && ss < 60;Это сделает поведение
isValidDateсогласованным с объявленным диапазоном годов и более реалистичным для времени.Дополнительно, аналогично MagicNumberDiagnostic, сейчас
isInsideDefaultValue(ctx)полностью отключает диагностику для дат в значениях по умолчанию параметров. Если это не было целевой задачей PR, имеет смысл отдельно переосмыслить эту часть логики.Also applies to: 137-154
🧹 Nitpick comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (4)
50-78: ThegetChildCount() == 1filter is fragile and may silently fail on AST variations.Both
getExpressionmethods rely on exact child counts at each AST level. If the parser structure changes or handles edge cases differently (e.g., with parentheses, type casts, or comments), the filter chain returnsOptional.empty()without indicating why.Consider adding diagnostic logging or comments documenting the expected AST structure, and verify this logic against diverse code samples.
</review_comment_end>
225-234: ReplaceObject[]return type with a proper record or data class.Returning
Object[]with mixed types is a code smell that sacrifices type safety and readability. Callers must cast array elements, which is error-prone.Consider introducing a record:
private record CallParamInfo( BSLParser.CallParamContext callParam, int paramIndex, BSLParser.CallParamListContext callParamList ) {} private static Optional<CallParamInfo> getCallParamInfo(BSLParser.ExpressionContext expression) { var callParam = expression.getParent(); if (callParam instanceof BSLParser.CallParamContext callParamContext) { var callParamList = (BSLParser.CallParamListContext) callParamContext.getParent(); var callParams = callParamList.callParam(); var paramIndex = callParams.indexOf(callParamContext); return Optional.of(new CallParamInfo(callParamContext, paramIndex, callParamList)); } return Optional.empty(); }Then update callers to use
callParamInfo.paramIndex()instead of casting(Integer) callParamInfo[1].
</review_comment_end>
308-324: Extract magic number loop limits as named constants.The traversal limit of 50 (line 310) and similar limits elsewhere (5 at line 448, 5 at line 545, 10 at line 175) are hardcoded without explanation. Extracting these as named constants would improve maintainability and document their purpose.
Example:
private static final int MAX_ASSIGNMENT_SEARCH_DEPTH = 50; private static final int MAX_CALL_PARAM_SEARCH_DEPTH = 5; private static final int MAX_CALL_STATEMENT_SEARCH_DEPTH = 10;</review_comment_end>
37-669: Consider breaking down this large abstract class for better maintainability.With 670 lines and 30+ methods, this class has high cognitive complexity. While consolidating shared logic is valuable, consider whether some of the AST traversal utilities (e.g., finding assignments, checking contexts) could be extracted into separate helper classes or a utility package.
This would improve testability, reusability, and maintainability without sacrificing the benefits of shared logic.
</review_comment_end>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (7)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractDiagnosticTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-07-04T19:35:42.937Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:2-4
Timestamp: 2024-07-04T19:35:42.937Z
Learning: Test resources in the repository may contain code that intentionally triggers diagnostics to demonstrate their functionality.
Applied to files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-10-27T08:05:10.072Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3555
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java:163-166
Timestamp: 2025-10-27T08:05:10.072Z
Learning: В проекте bsl-language-server используется собственная версия ANTLR4, в которой интерфейс TerminalNode переопределяет метод getParent() с возвращаемым типом RuleNode вместо ParseTree (как в стандартной версии ANTLR4).
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2024-07-04T19:35:26.747Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The file `DoubleNegativesDiagnostic.bsl` is a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build
- GitHub Check: Analyse
🔇 Additional comments (6)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractDiagnosticTest.java (1)
42-43: Реализация no-arg конструктора через рефлексию выглядит корректнойТип
Tаккуратно выводится из параметризованного суперкласса,diagnosticClassинициализируется во всех конструкторах, при невозможности вывести тип вы кидаете явныйIllegalStateException. Для текущего использования (прямые наследники вродеMagicNumberDiagnosticTest extends AbstractDiagnosticTest<MagicNumberDiagnostic>) этого более чем достаточно.На будущее только стоит помнить, что при введении дополнительного промежуточного уровня наследования или сырых типов этот конструктор начнёт падать — это ожидаемое и, в целом, полезное fail-fast поведение.
Also applies to: 59-73
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java (1)
27-31: Тесты хорошо зафиксировали новую семантику MagicNumberDiagnosticПереход на базовый
AbstractDiagnosticTestбез явного конструктора, настройка черезdiagnosticInstance.configure(config)и обновлённые ожидания по количеству и диапазонам диагностик выглядят согласованно с изменённой логикой (авторизованные числа, флагallowMagicIndexes, учёт структур/соответствий).По самому тестовому код-стайлу и использованию API замечаний нет.
Also applies to: 34-50, 52-69, 71-93
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
27-31: Покрытие MagicDateDiagnostic обновлено корректноТесты отражают новую конфигурацию и поведение диагностики: проверяются как базовые диапазоны, так и разбор сложной строки
authorizedDates(пробелы, двойные запятые). Использование базового тестового класса иdiagnosticInstance.configure(config)выглядит аккуратно, явных логических проблем по ожиданиям нет.Also applies to: 34-57, 59-78
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
86-106: Behavioral change for default parameter values is confirmed—please verify intent and update tests accordingly.The refactored code has inverted the handling of magic numbers in default parameter values:
- Old logic (
parent.getParent() instanceof DefaultValueContext || isWrongExpression): added diagnostic if number was in a default value OR was a wrong expression.- New logic (
!isDefaultValue && isWrongExpression): skips diagnostic entirely if number appears anywhere inside a DefaultValueContext.This reversal is real (confirmed via git diff) but is not mentioned in the commit message and appears unrelated to the PR's stated goal (refactoring for structures/correspondence support).
Please confirm whether this change is intentional. If so, update the commit message and test expectations; if not, restore the original logic to preserve the diagnostic for magic numbers in default parameter values.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (2)
243-257: Null safety checks properly implemented.The method now correctly guards against null
assignmentandexpression(lines 247-249), addressing the critical issue from previous reviews. The defensive programming here prevents NPE when variables lack local assignments.</review_comment_end>
487-512: Bounds checking properly implemented.The method now correctly checks for empty token lists (lines 507-511) and uses pattern matching for type-safe casting (lines 491-494), addressing the major issue from previous reviews. This prevents
IndexOutOfBoundsExceptionwhen parameters lack tokens.</review_comment_end>
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
55-63: Loosen PARAM_PATTERN and tighten time bounds inisValidDate.Two small correctness issues in the date handling:
PARAM_PATTERN year restriction
PARAM_PATTERNonly matches strings whose first digit is[0-3]:private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile( "\"[0123]{1}\\d{7}\"|\"[0123]{1}\\d{13}\"" );This excludes syntactically valid 1C years
4000–9999, even thoughisValidDate(String)allows up toMAX_YEAR_BY_1C(9999). It would be more consistent to allow any 8/14‑digit sequences and letisValidDateenforce the year bounds:
- private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile(
- ""[0123]{1}\d{7}"|"[0123]{1}\d{13}""
- );
- private static final Pattern PARAM_PATTERN = CaseInsensitivePattern.compile(
- ""\d{8}"|"\d{14}""
- );
Time component bounds
The final check:
return hh <= 24 && mm <= 60 && ss <= 60;treats
24:60:60as valid and doesn’t enforce non‑negative ranges. For realistic datetime validation this should probably be:
- return hh <= 24 && mm <= 60 && ss <= 60;
- return (hh >= 0 && hh < 24)
&& (mm >= 0 && mm < 60)&& (ss >= 0 && ss < 60);Both changes only narrow what is considered a “valid date literal” and make it consistent with 1C’s datetime domain and your
MAX_YEAR_BY_1Cconstant.Also applies to: 128-154, 156-163, 170-173
🧹 Nitpick comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (3)
249-333: Variable type inference is intentionally narrow; consider documenting or enriching it.
getVariableTypeFromASTandfindVariableAssignmentonly inferStructure/Correspondencewhen the variable is assigned viaНовый Структура/Соответствиеin the same code block. Parameters, globals, or variables initialized elsewhere fall back toOptional.empty(), soinsideStructureOrCorrespondencewill not recognize theirВставить/Insertcalls as structural/correspondence contexts.Functionally this is safe (returns
Optional.empty()and degrades to “no special context”), but it means the FP fix doesn’t apply when the structure/correspondence comes from anything other than a localNewassignment.If that limitation is acceptable, it would help to document it in the Javadoc for
getVariableTypeFromAST/insideStructureOrCorrespondence. If you want broader coverage, you could later extend this to usesymbolTree-based type info similarly to diagnostics likeCreateQueryInCycleDiagnostic.
225-243: Replace rawObject[]result with a typed carrier to avoid fragile index casts.
getCallParamInforeturns anObject[]whose elements are implicitly[CallParamContext, Integer index, CallParamListContext], and callers downcast by position. This works but is brittle and non‑self‑documenting.Consider introducing a small private static record or class, e.g.:
- private static Object[] getCallParamInfo(BSLParser.ExpressionContext expression) { + private record CallParamInfo( + BSLParser.CallParamContext param, + int index, + BSLParser.CallParamListContext list + ) {} + + private static CallParamInfo getCallParamInfo(BSLParser.ExpressionContext expression) { var callParam = expression.getParent(); if (callParam instanceof BSLParser.CallParamContext callParamContext) { var callParamListParent = callParamContext.getParent(); if (!(callParamListParent instanceof BSLParser.CallParamListContext callParamList)) { - return new Object[0]; + return null; } var callParams = callParamList.callParam(); var paramIndex = callParams.indexOf(callParamContext); - return new Object[]{callParamContext, paramIndex, callParamList}; + return new CallParamInfo(callParamContext, paramIndex, callParamList); } - return new Object[0]; + return null; }Call sites can then use null checks and named accessors instead of array indices, improving readability and type safety.
80-107: Property-assignment detection currently matches any property, not just Structures.
insideStructureOrCorrespondencetreatscheckInsideStructurePropertyAssignmentas a “structure/correspondence context”, but the implementation only checks that the LHS is anAccessPropertyContexton an assignment; it does not verify that the receiver is actually ofСтруктура/Structuretype (or a correspondence).This means assignments like
SomeObject.SomeProperty = 20250101;will also suppress magic‑value diagnostics, not only structure properties. That might be exactly what you want (property name as sufficient context in general), but the Javadoc and method names still talk specifically about structures.If the intent is “any property assignment gives enough semantic context”, please update comments/naming to reflect that. If you want to keep the behavior strictly for structures, you’ll need a type check similar to the
DiagnosticHelper/symbolTree approaches used elsewhere and in earlier iterations ofMagicDateDiagnostic.Also applies to: 530-546
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
176-201: AST walk ininsideAssignmentWithDateMethodForSimpleDateis safe but quite brittle.The method assumes a very specific parent chain:
Expression → CallParam → CallParamList → DoCall → GlobalMethodCall(Date/Дата) → ComplexId → Member → Expression → Assignment.Thanks to the final
instanceof GlobalMethodCallContextandMETHOD_PATTERNchecks, incorrect shapes simply returnfalse, so you won’t getClassCastException. However, this pattern is still fragile:
- It relies on fixed ancestor distances rather than tree rules (e.g.,
Trees.getAncestorByRuleIndexlike in the base class).- Any future grammar tweaks around global calls or argument lists could silently change behavior.
If you want to make this more resilient, consider aligning it with the style used in
AbstractMagicValueDiagnostic:
- Use
Trees.getAncestorByRuleIndexor small helper methods to locateGlobalMethodCallContextandAssignmentContextinstead of hardcoded parent chains.- Optionally check that the expression actually corresponds to the first/only parameter using the same
CallParamhelpers as for structures/correspondences.Not urgent for this PR, but worth a follow‑up cleanup to keep diagnostics robust to grammar evolution.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
75-83: visitConstValue guard chain correctly wires in structure/correspondence suppression.The new
visitConstValueflow:
- validates string‑encoded dates via
isValidDate,- skips default parameter values with
isInsideDefaultValue,- uses
getExpression(ctx)+insideStructureOrCorrespondence(...)to suppress diagnostics in structure and correspondence contexts,- then centralizes remaining context checks in
shouldAddDiagnostic.This aligns well with the PR goal of eliminating false positives for structure/correspondence usage while keeping the rest of the diagnostic behavior localized in one place.
Also applies to: 85-109
johnnyshut
force-pushed
the
deature/3496
branch
from
1d04968 to
bb29a3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
274-288: Consider refactoring Object[] return to a type-safe record.The method returns
Object[]with mixed types[CallParamContext, Integer, CallParamListContext], requiring callers to cast elements (lines 627, 633, 694, 700). This pattern is brittle and not type-safe.Consider introducing a record for better type safety:
private record CallParamInfo( BSLParser.CallParamContext param, int index, BSLParser.CallParamListContext list ) {} private static Optional<CallParamInfo> getCallParamInfo(BSLParser.ExpressionContext expression) { var callParamOpt = getCallParam(expression); if (callParamOpt.isEmpty()) { return Optional.empty(); } var callParamContext = callParamOpt.get(); var callParamListOpt = getCallParamList(callParamContext); if (callParamListOpt.isEmpty()) { return Optional.empty(); } var callParamList = callParamListOpt.get(); var callParams = callParamList.callParam(); var paramIndex = callParams.indexOf(callParamContext); return Optional.of(new CallParamInfo(callParamContext, paramIndex, callParamList)); }Then update callers to use the record fields instead of array indexing and casting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
🔇 Additional comments (10)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (10)
1-78: LGTM! Well-structured helper methods with defensive Optional patterns.The
getExpressionoverloads use proper Optional chains with defensive filters and type checks. The case-insensitive INSERT_METHOD_PATTERN is appropriate for BSL syntax.
80-111: LGTM! Clean orchestration with proper separation of concerns.The method efficiently checks multiple conditions with early returns and delegates to focused helper methods.
113-158: LGTM! Defensive checks properly implemented.The method now uses Optional-returning helpers (getCallParamList, getDoCall) which safely handle instanceof checks, addressing previous review concerns.
160-231: LGTM! Safe Optional-based helpers with proper type guards.The Optional-returning helper methods (getCallParamList, getDoCall, getCallParam) provide type-safe parent traversal with instanceof checks. The 10-iteration limit in findCallStatement is reasonable for typical AST depths.
313-352: LGTM! Excellent defensive programming throughout.The methods properly guard against null and empty cases at each step, with clean separation between type extraction and determination logic.
354-471: LGTM! Robust AST traversal with proper bounds and null checks.The variable assignment search methods use reasonable iteration limits (50 levels), proper null guards, and case-insensitive name matching appropriate for BSL syntax.
473-538: LGTM! Safe type checks and defensive null guards.The structure constructor detection uses proper instanceof checks (line 527) before casting (line 531) and returns null appropriately when conditions aren't met. Based on learnings about BSLParserRuleContext.
547-572: LGTM! Bounds check properly implemented.The token list bounds check (lines 567-569) has been properly added as recommended in previous reviews. The instanceof pattern matching (line 552) is clean and safe.
574-612: LGTM! Proper null and type checks for property assignments.The assignment detection correctly checks for null lValue and uses instanceof to verify AccessPropertyContext, with reasonable iteration limits.
709-744: LGTM! Clean generic utility methods with proper type safety.The utility methods use appropriate generic bounds (
Class<? extends BSLParserRuleContext>) and proper null checks for safe context verification.
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (2)
283-297: Replace rawObject[]ingetCallParamInfowith a typed helper for safety and clarity
getCallParamInforeturns a loosely-typedObject[]and callers rely on positional casts ((Integer) callParamInfo[1],(CallParamListContext) callParamInfo[2]). This is brittle and hard to read, especially in a class that already has non-trivial AST traversal logic. This concern was raised earlier and is still present in the current version.Consider introducing a small typed holder (e.g. a
recordor static inner class) and returningOptional<CallParamInfo>instead of anObject[]. Conceptually:// Near getCallParamInfo private record CallParamInfo( BSLParser.CallParamContext callParamContext, int paramIndex, BSLParser.CallParamListContext callParamList ) {} private static Optional<CallParamInfo> getCallParamInfo(BSLParser.ExpressionContext expression) { var callParamOpt = getCallParam(expression); if (callParamOpt.isEmpty()) { return Optional.empty(); } var callParamContext = callParamOpt.get(); var callParamListOpt = getCallParamList(callParamContext); if (callParamListOpt.isEmpty()) { return Optional.empty(); } var callParamList = callParamListOpt.get(); var callParams = callParamList.callParam(); var paramIndex = callParams.indexOf(callParamContext); return Optional.of(new CallParamInfo(callParamContext, paramIndex, callParamList)); }And then in
checkInsideCorrespondenceInsertandisFirstParameterOfInsertMethod:var callParamInfoOpt = getCallParamInfo(expression); if (callParamInfoOpt.isEmpty()) { return false; } var callParamInfo = callParamInfoOpt.get(); var paramIndex = callParamInfo.paramIndex(); // ... var callParamList = callParamInfo.callParamList();This keeps the logic the same but eliminates unchecked casts and magic indices.
Also applies to: 630-647, 679-697
306-320: Add null check forIDENTIFIER()to avoid NPE ingetVariableTypeFromAST
getVariableTypeFromASTassumescallStatement.IDENTIFIER()is always non-null and immediately callsgetText(). For call statements without an identifier prefix (e.g. a directВставить(...)/Insert(...)call),IDENTIFIER()may benull, which would crash the diagnostic with an NPE. This was pointed out earlier and still applies to the current code.A small guard fixes it safely:
private static Optional<Boolean> getVariableTypeFromAST(BSLParser.CallStatementContext callStatement) { - var identifier = callStatement.IDENTIFIER(); - var variableName = identifier.getText(); + var identifier = callStatement.IDENTIFIER(); + if (identifier == null) { + return Optional.empty(); + } + var variableName = identifier.getText(); var assignment = findVariableAssignment(callStatement, variableName); if (assignment == null || assignment.expression() == null) { return Optional.empty(); } @@ - var typeName = extractTypeNameFromExpression(assignment.expression()); + var typeName = extractTypeNameFromExpression(assignment.expression());With this change, unknown or non-standard call forms simply lead to
Optional.empty()instead of throwing.
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
397-417: Consider searching assignments from bottom to top to use the nearest one
findAssignmentInCodeBlockiterates statements in forward order and returns the first matching assignment before the current position. This can pick an older assignment instead of the most recent one, which may misclassify the variable type if it’s reassigned multiple times in the same block.This is not a correctness bug (it only affects heuristic precision), but reversing the search order would likely improve results:
- for (var statement : codeBlock.statement()) { + var statements = codeBlock.statement(); + for (var i = statements.size() - 1; i >= 0; i--) { + var statement = statements.get(i); if (statement == null) { continue; }Keeping
isAssignmentBeforePositionstill ensures assignments afterbeforeNodeare excluded.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
80-111: Good centralization of structure/correspondence context checks
insideStructureOrCorrespondencecleanly orchestrates all the specific context predicates (insert, constructor, property assignment, correspondence key/value). The early returns and null-guard make it straightforward and reuse-friendly for both MagicNumber and MagicDate diagnostics.
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
320-334: Add missing null check for IDENTIFIER() to prevent NPE.Line 321 retrieves an identifier that can be null when the call statement lacks an identifier prefix (e.g., a direct function call without a variable). Line 322 then calls
getText()without checking, which will throwNullPointerException.Apply this diff to add the defensive null check:
private static Optional<Boolean> getVariableTypeFromAST(BSLParser.CallStatementContext callStatement) { var identifier = callStatement.IDENTIFIER(); + if (identifier == null) { + return Optional.empty(); + } var variableName = identifier.getText(); var assignmentOpt = findVariableAssignment(callStatement, variableName);
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
172-179: Consider extracting AST traversal depth limits as named constants.Multiple methods use hardcoded loop limits (5, 10, 50) when traversing the AST hierarchy. While these limits prevent infinite loops, defining them as named constants with documentation would clarify the rationale and make future adjustments easier.
Example:
private static final int MAX_CALL_STATEMENT_DEPTH = 10; private static final int MAX_ASSIGNMENT_SEARCH_DEPTH = 50; private static final int MAX_PARAM_CONTEXT_DEPTH = 5;Also applies to: 386-402, 526-532, 629-635
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
🔇 Additional comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (3)
571-596: Defensive checks properly implemented.The instanceof check (lines 575-578) and the isEmpty() check before accessing tokens (lines 592-595) correctly prevent ClassCastException and IndexOutOfBoundsException, addressing concerns from previous reviews.
285-289: Well-designed record improves type safety.The
CallParamInforecord is a clean solution that replaces the previousObject[]approach, providing type safety and better code readability. This properly addresses the feedback from previous reviews.
38-38: Well-structured abstract base with comprehensive safety checks.The class appropriately centralizes complex AST traversal logic for magic value detection. The consistent use of
Optional, safe casting withinstanceofchecks, and clear method organization demonstrate good defensive programming practices.Also applies to: 188-222
|
@nixel2007 поправил, но, gh плохо, по моему |
…еден рефакторинг существующих диагностик. Введен AbstractMagicValueDiagnostic в качестве базового класса для обработки диагностик магических значений, инкапсулирующих общую логику проверки выражений внутри структур и соответствий. MagicDateDiagnostic и MagicNumberDiagnostic обновлены для расширения этого нового класса, упрощения их реализации и повышения возможности повторного использования кода. Ключевые изменения включают: - Создание AbstractMagicValueDiagnostic с методами для обработки контекста выражений. - Рефакторинг MagicDateDiagnostic и MagicNumberDiagnostic для использования новых методов базового класса, что повышает удобство поддержки и ясность кода. - Корректировки диагностических проверок для использования общей функциональности, предоставляемой AbstractMagicValueDiagnostic.
- Автоматическое определение класса диагностики через рефлексию в AbstractDiagnosticTest - Удаление явных конструкторов из тестов MagicDateDiagnostic и MagicNumberDiagnostic - Добавление тестовых случаев для проверки структур и соответствий - Обновление ожидаемых количеств диагностик в тестах
Обновлены файлы документации MagicDate.md и MagicNumber.md, добавлены разделы об исключениях, поясняющие, что магические даты и числа, используемые в структурах и соответствиях, не считаются ошибками. Приведены примеры использования в контексте структур данных.
johnnyshut
force-pushed
the
deature/3496
branch
from
1c86342 to
eac58f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (1)
128-154: Date literal validation still inconsistent with MAX_YEAR and time bounds.
PARAM_PATTERNrestricts the first year digit to[0-3], whileisValidDate(String)allows any year up toMAX_YEAR_BY_1C(currently 9999). This means literals like"40000101"will be rejected by the pattern and skipped entirely, even though your year check would accept them. Also,hh <= 24 && mm <= 60 && ss <= 60allows24:60:60, which is not a valid time.Consider:
- Relaxing
PARAM_PATTERNto accept any 8- or 14-digit date literal and lettingisValidDateenforce year bounds.- Tightening the time check to
0 <= hh && hh < 24,0 <= mm && mm < 60,0 <= ss && ss < 60.This keeps literal parsing and semantic validation aligned.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java (1)
86-101: MagicNumber now correctly delegates structural context decisions to the shared helper, but inherits its over-broad property suppression.The new
visitNumericandisWrongExpressionlogic nicely separate:
- index-specific behavior (
mayBeNumberAccesscontrolled byallowMagicIndexes),- structural/correspondence contexts via
insideStructureOrCorrespondence,- default parameter values and return statements.
However, because
insideStructureOrCorrespondencecurrently treats any property assignment as “structure-like” (seeAbstractMagicValueDiagnostic.checkInsideStructurePropertyAssignment), MagicNumber will also skip diagnostics for non-structure property assignments (e.g. arbitrary object fields). Once the base is tightened to only recognize real structures/correspondences, this method will behave as the docs describe.No changes are needed here beyond whatever adjustment you make in the shared base.
Also applies to: 117-147
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
320-376: AST-based variable type inference is careful but may be heavy; consider caching if it becomes hot.The
getVariableTypeFromAST→findVariableAssignment→findAssignmentInCodeBlockchain safely handles missing expressions and unexpected structures, but it re-scans code blocks for each value occurrence. On large modules with many magic values this can become noticeable.If profiling ever shows this as a hotspot, consider caching inferred structure/correspondence types per variable (e.g., per
CodeBlockContext), so repeated calls for the same variable don’t walk the AST repeatedly.Also applies to: 386-432
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/resources/diagnostics/MagicDateDiagnostic.bslis excluded by!src/test/resources/**src/test/resources/diagnostics/MagicNumberDiagnostic.bslis excluded by!src/test/resources/**
📒 Files selected for processing (11)
docs/diagnostics/MagicDate.md(1 hunks)docs/diagnostics/MagicNumber.md(2 hunks)docs/en/diagnostics/MagicDate.md(1 hunks)docs/en/diagnostics/MagicNumber.md(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractDiagnosticTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/en/diagnostics/MagicDate.md
- docs/diagnostics/MagicNumber.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/DiagnosticHelper.java
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java
📚 Learning: 2025-10-27T08:05:10.072Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3555
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java:163-166
Timestamp: 2025-10-27T08:05:10.072Z
Learning: В проекте bsl-language-server используется собственная версия ANTLR4, в которой интерфейс TerminalNode переопределяет метод getParent() с возвращаемым типом RuleNode вместо ParseTree (как в стандартной версии ANTLR4).
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2024-07-04T19:35:26.747Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:20-21
Timestamp: 2024-07-04T19:35:26.747Z
Learning: The file `DoubleNegativesDiagnostic.bsl` is a test resource intended to demonstrate the functionality of the DoubleNegatives diagnostic.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnostic.java
📚 Learning: 2024-07-04T19:35:42.937Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3308
File: src/test/resources/diagnostics/DoubleNegativesDiagnostic.bsl:2-4
Timestamp: 2024-07-04T19:35:42.937Z
Learning: Test resources in the repository may contain code that intentionally triggers diagnostics to demonstrate their functionality.
Applied to files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: Benchmark
- GitHub Check: Analyse
- GitHub Check: build
🔇 Additional comments (8)
docs/en/diagnostics/MagicNumber.md (1)
33-59: Exceptions section matches implemented behavior.The new examples for Structure, FixedStructure, and Correspondence correctly document that magic numbers in these contexts are ignored, consistent with the new shared logic in
AbstractMagicValueDiagnostic.src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractDiagnosticTest.java (1)
42-43: Reflection-based generic resolution is reasonable for current test hierarchy.The no-arg constructor correctly infers
diagnosticClassfromAbstractDiagnosticTest<ConcreteDiagnostic>and keeps the explicitClass<T>constructor as a fallback. This should simplify most diagnostic tests without breaking existing ones, as long as subclasses keep a concrete type argument.Also applies to: 59-73
docs/diagnostics/MagicDate.md (1)
39-65: Russian exceptions documentation aligns with new date behavior.The “Исключения” section correctly reflects that magic dates used as structure fields, constructor arguments, and correspondence keys/values are not reported, matching the new shared logic.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicNumberDiagnosticTest.java (1)
27-31: Updated expectations look consistent with new magic-number rules.The reduced diagnostic counts and adjusted ranges, including index-related cases and the extra ranges in
testIndexes, match the new behavior (structure/correspondence suppression andallowMagicIndexes). Using a localconfigfromgetDefaultConfiguration()and reconfiguring the instance is appropriate here.Also applies to: 38-49, 54-69, 73-92
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnosticTest.java (1)
27-29: MagicDate test expectations updated coherently with suppression rules.The new diagnostic counts and ranges, together with configuration via the local
configmap, match the intended behavior: authorized dates and structure/correspondence contexts are no longer reported, while other usages still are.Also applies to: 38-38, 56-57, 61-63, 67-67, 77-77
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
44-79: Expression/context helpers are well-factored and reused consistently.The
getExpressionoverloads and genericinsideContexthelpers provide a clean, reusable way to locate surroundingExpressionContext, assignments, and return statements, which simplifies both MagicNumber and MagicDate implementations.Also applies to: 716-750
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MagicDateDiagnostic.java (2)
85-104: Const-value flow and structure/default suppression are consistent with MagicNumber.The updated
visitConstValuecorrectly:
- validates string dates,
- skips diagnostic inside default parameter values,
- delegates structure/correspondence context checks to
insideStructureOrCorrespondence,- and centralizes “should we report?” via
shouldAddDiagnostic.This matches the documented exceptions for structures/correspondences and mirrors MagicNumber’s treatment of default values and contexts.
Also applies to: 111-127
176-201: AST-based detection ofДата/Date(...)assignments is precise and safer than text matching.
insideAssignmentWithDateMethodForSimpleDatenow walks the AST (callParam → callParamList → doCall → GlobalMethodCall → AssignmentContext) and usesMETHOD_PATTERNinstead of brittle substring checks. This is a solid improvement over earlier text-based heuristics forДата/Dateusages.
.../java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
Outdated
Show resolved
Hide resolved
Вынесены магические числа (10, 50, 5) в именованные константы для улучшения читаемости кода и устранения замечаний линтера.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (2)
320-334: Critical NPE risk: Missing null check for IDENTIFIER().Line 322 calls
identifier.getText()without checking ifIDENTIFIER()returned null. This will throw NPE for call statements without an identifier prefix (e.g., direct function calls or global calls without a variable reference).Apply this diff to add the defensive null check:
private static Optional<Boolean> getVariableTypeFromAST(BSLParser.CallStatementContext callStatement) { var identifier = callStatement.IDENTIFIER(); + if (identifier == null) { + return Optional.empty(); + } var variableName = identifier.getText(); var assignmentOpt = findVariableAssignment(callStatement, variableName);Based on past review comments.
604-615: Major issue: Over-broad suppression for all property assignments.
checkInsideStructurePropertyAssignment()only verifies that the left-hand side is anAccessPropertyContext, without confirming the receiver is aСтруктура/StructureorФиксированнаяСтруктура/FixedStructure. This suppresses MagicNumber/MagicDate diagnostics for all property assignments—includingObject.SomeField = 42,Variable.Property = Date(), etc.—not just structure properties.This contradicts the PR objective ("при работе со структурами") and the documented scope. All comparable checks validate receiver type:
checkInsideStructureConstructor()usesDiagnosticHelper.isStructureType()/isFixedStructureType()checkInsideCorrespondenceInsert()validates viagetVariableTypeFromCallParamList()Extract the receiver from
lValueand apply type inference (e.g., viagetVariableTypeFromAST()orextractTypeNameFromExpression()) to confirm it is a structure-like type before returning true.Based on past review comments.
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (1)
38-751: Consider extracting helper classes to reduce complexity.At 752 lines with ~30 methods, this class has high cognitive complexity. While the consolidation may be intentional for an abstract base, consider extracting cohesive groups into helper classes:
- AST navigation utilities (findMethodCall, findCallStatement, getCallParamList, etc.) →
StructureAstNavigator- Type inference logic (getVariableTypeFromAST, extractTypeNameFromExpression, determineVariableType) →
StructureTypeInference- Variable assignment search (findVariableAssignment, findAssignmentInCodeBlock) →
VariableAssignmentFinderThis would improve maintainability and testability of individual components.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: Analyse
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: Benchmark
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMagicValueDiagnostic.java (3)
48-83: LGTM! Clean Optional-based extraction logic.The
getExpression()methods use clean Optional chains with proper null checks and instanceof filtering. The defensive null check at line 73 prevents NPE whenconstValueis null.
97-111: LGTM! Well-structured dispatcher method.The
insideStructureOrCorrespondence()method clearly delegates to specific check methods with comprehensive documentation. The early-return pattern is appropriate for this OR-logic scenario.
571-596: LGTM! Exemplary defensive implementation.
isValidStructureConstructorParameter()demonstrates proper defensive programming:
- instanceof check before casting (line 576)
- Type validation using
DiagnosticHelper.isFixedStructureType()(line 586)- Empty check before accessing token list (line 592)
This is the correct pattern that should be applied to
checkInsideStructurePropertyAssignment()(lines 604-615) to validate receiver types.
|
2 participants
Описание
Исправлены ложные срабатывания диагностик MagicNumber и MagicDate при работе со структурами. Теперь диагностики не срабатывают при присваивании значений к свойствам структуры, так как имя свойства уже дает смысл значению.
Связанные задачи
Closes #3496
Чеклист
Общие
Для диагностик
Дополнительно
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.