[WIP] Аннотация как значение параметра Аннотации #1576
Conversation
WalkthroughAdds support for nested annotations as parameter values across parsing, compilation, runtime representation, and reflection. Introduces BslAnnotationValue, updates parameter handling to allow null values, removes ConstantValueIndex propagation, adjusts parser and compiler flows to build nested annotation values, updates reflection to expand nested parameters, and adds tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Src as Source Code
participant Parser as Parser (DefaultBslParser)
participant AST as AST (Annotation/Parameter Nodes)
participant CG as CodeGen (StackMachineCodeGenerator)
participant Mod as Module Constants
participant RT as Runtime Values
participant Refl as Reflector
Src->>Parser: &Аннотация(П1=&ТожеАннотация(...))
Parser->>AST: BuildAnnotationDefinition()
Parser->>AST: BuildAnnotationParameters()
alt Parameter value is annotation
Parser->>AST: AnnotationParameterNode.AnnotationNode = nested
else Literal/absent
Parser->>AST: Name/Value set (Value may be null)
end
AST->>CG: AnnotatableNode with nested annotations
CG->>RT: MakeAnnotationParameterValueConstant()
alt Nested annotation
CG->>RT: Build BslAnnotationValue(Name)
loop For each nested param
CG->>RT: new BslAnnotationParameter(Name, Value|null)
RT-->>CG: Append to BslAnnotationValue.Parameters
end
CG->>Mod: RegisterAnnotationConst(BslAnnotationValue)
else Literal constant
CG->>Mod: GetConstNumber(literal)
else No constant
CG->>RT: null
end
CG->>RT: new BslAnnotationParameter(Name, runtimeValue)
RT->>Refl: BslAnnotationAttribute with parameters
Refl->>Refl: FillAnnotationParameters(parameters)
alt Value is BslAnnotationValue
Refl->>Refl: Create nested table via EmptyAnnotationsTable
Refl->>Refl: Recurse FillAnnotationParameters(...)
else Primitive/null
Refl->>Refl: Set "Значение" directly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/OneScript.Core/Types/BasicTypes.cs (1)
21-39: Register new BasicTypes.Annotation in DefaultTypeManager
In theDefaultTypeManager()constructor (src/ScriptEngine/Machine/DefaultTypeManager.cs), addRegisterTypeInternal(BasicTypes.Annotation);after the existing
RegisterTypeInternal(BasicTypes.Type)so that
GetTypeByFrameworkType(typeof(BslAnnotationValue))resolves correctly.src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (1)
22-35: Defensive copy and null-safety for ParametersThe ctor assigns the incoming IEnumerable directly, while SetParameters copies into a List. Make ctor behavior consistent and null-safe to prevent external mutation/NREs.
Apply:
- public BslAnnotationAttribute(string name, IEnumerable<BslAnnotationParameter> parameters) - { - Name = name; - Parameters = parameters; - } + public BslAnnotationAttribute(string name, IEnumerable<BslAnnotationParameter> parameters) + { + if (string.IsNullOrEmpty(name)) + throw new ArgumentNullException(nameof(name)); + Name = name; + Parameters = parameters != null + ? new List<BslAnnotationParameter>(parameters) + : Array.Empty<BslAnnotationParameter>(); + }Optionally also guard the single-arg ctor:
- public BslAnnotationAttribute(string name) + public BslAnnotationAttribute(string name) { - Name = name; + if (string.IsNullOrEmpty(name)) + throw new ArgumentNullException(nameof(name)); + Name = name; }
🧹 Nitpick comments (13)
src/ScriptEngine/Machine/AnnotationParameter.cs (1)
12-21: Optional: provide a constructor to enforce sane defaultsHelps avoid accidental ValueIndex=0 and clarifies intent.
Example:
public struct AnnotationParameter { public string Name; public int ValueIndex; [NonSerialized] public IValue RuntimeValue; public const int UNDEFINED_VALUE_INDEX = -1; + public AnnotationParameter(string name, int valueIndex = UNDEFINED_VALUE_INDEX, IValue runtimeValue = null) + { + Name = name; + ValueIndex = valueIndex; + RuntimeValue = runtimeValue; + }src/ScriptEngine/Machine/AnnotationDefinition.cs (1)
7-9: Remove unused usingSystem.Text is unused here.
-using System.Text;src/Tests/OneScript.Language.Tests/ParserTests.cs (1)
147-169: Strengthen assertions for nested annotation parametersCurrent test only checks the node kinds. Assert names and both positional and named nested-annotation parameters to prevent regressions.
[Fact] public void CheckBuild_Of_AnnotationAsValue() { var code = @" &Аннотация(Параметр = &ТожеАннотация(&СТожеПараметромАннотацией, П2 = &СТожеПараметромАннотацией)) Процедура Процедура1() Экспорт КонецПроцедуры"; var treeValidator = ParseModuleAndGetValidator(code); treeValidator.Is(NodeKind.MethodsSection); var methodNode = treeValidator.NextChild(); methodNode.Is(NodeKind.Method); var annotationNode = methodNode.NextChild(); annotationNode.Is(NodeKind.Annotation); + annotationNode.Equal("Аннотация"); var annotationParameter = annotationNode.NextChild(); annotationParameter.Is(NodeKind.AnnotationParameter); - annotationParameter - .NextChildIs(NodeKind.AnnotationParameterName) - .NextChildIs(NodeKind.Annotation); + // Параметр = <Аннотация> + annotationParameter + .NextChildIs(NodeKind.AnnotationParameterName).ChildItself().Equal("Параметр"); + var nestedAnno = annotationParameter.NextChildIs(NodeKind.Annotation); + nestedAnno.ChildItself().Equal("ТожеАннотация"); + // ТожеАннотация(&СТожеПараметромАннотацией, П2 = &СТожеПараметромАннотацией) + nestedAnno.HasChildNodes(2); + // 1) позиционный параметр-Аннотация + var nestedParam1 = nestedAnno.NextChild().Is(NodeKind.AnnotationParameter); + nestedParam1.NextChildIs(NodeKind.Annotation).NoMoreChildren(); + // 2) именованный параметр-Аннотация + var nestedParam2 = nestedAnno.NextChild().Is(NodeKind.AnnotationParameter); + nestedParam2.NextChildIs(NodeKind.AnnotationParameterName).ChildItself().Equal("П2"); + nestedParam2.NextChildIs(NodeKind.Annotation).NoMoreChildren(); }src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
40-44: Null value now allowed: align API and callersAllowing null is fine, but make the nullability explicit or guarded to avoid accidental NREs in downstream code that iterates parameters/reads Value.
Apply either:
- Mark Value as nullable in nullable-enabled contexts (BslPrimitiveValue?).
- Or keep non-nullable and document that null means “no literal provided”, ensuring all consumers handle it.
38-51: Validate parameter names earlyName is relied upon by reflection/rendering. Guard against null/empty names in BslAnnotationParameter to fail fast.
- public BslAnnotationParameter(string name, BslPrimitiveValue value = null) + public BslAnnotationParameter(string name, BslPrimitiveValue value = null) { - Name = name; + if (string.IsNullOrEmpty(name)) + throw new ArgumentNullException(nameof(name)); + Name = name; Value = value; }src/OneScript.Native/Compiler/CompilerHelpers.cs (1)
93-111: Nested annotation value handling: OK; simplify and harden castsLogic is correct. Small readability/safety wins: early returns and typed foreach to avoid repeated casts.
- private static BslAnnotationParameter MakeAnnotationParameter(AnnotationParameterNode param) + private static BslAnnotationParameter MakeAnnotationParameter(AnnotationParameterNode param) { - BslAnnotationParameter result; if (param.AnnotationNode != null) { - var runtimeValue = new BslAnnotationValue(param.AnnotationNode.Name); - foreach (var child in param.AnnotationNode.Children) - { - runtimeValue.Parameters.Add(MakeAnnotationParameter((AnnotationParameterNode)child)); - } - result = new BslAnnotationParameter(param.Name, runtimeValue); + var runtimeValue = new BslAnnotationValue(param.AnnotationNode.Name); + foreach (AnnotationParameterNode child in param.AnnotationNode.Children) + { + runtimeValue.Parameters.Add(MakeAnnotationParameter(child)); + } + return new BslAnnotationParameter(param.Name, runtimeValue); } - else if (param.Value.Type != LexemType.NotALexem) { - var runtimeValue = ValueFromLiteral(param.Value); - result = new BslAnnotationParameter(param.Name, runtimeValue); + return new BslAnnotationParameter(param.Name, ValueFromLiteral(param.Value)); } - else - { - result = new BslAnnotationParameter(param.Name, null); - } - - return result; + return new BslAnnotationParameter(param.Name, null); }src/Tests/OneScript.Core.Tests/CodeGenerationTests.cs (1)
58-88: Comment vs assertion mismatch on constants countThe Russian comment says only the first-level value ends up in constants, but the test asserts 3 constants. Please reconcile: fix the comment, or adjust the expectation if dedup/serialization rules differ.
Optional robustness tweaks (assert semantics, not counts):
- image.Constants.Should().HaveCount(3); + image.Constants.Should().NotBeEmpty();And consider asserting names and constant dedup for repeated nested annotation:
+ var p0 = parameterValue.Parameters.ElementAt(0); + var p1 = parameterValue.Parameters.ElementAt(1); + ((BslAnnotationValue)p0.Value).Name.Should().Be("СТожеПараметромАннотацией"); + ((BslAnnotationValue)p1.Value).Name.Should().Be("СТожеПараметромАннотацией"); + p0.ConstantValueIndex.Should().Be(p1.ConstantValueIndex);src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationParameterNode.cs (1)
20-34: Safer cast in else-branchOn future changes, a non-terminal child could slip in and break the hard cast. Use pattern matching.
- else - { - var node = (TerminalNode)child; + else if (child is TerminalNode node) + { if (child.Kind == NodeKind.AnnotationParameterName) { Name = node.Lexem.Content; } if (child.Kind == NodeKind.AnnotationParameterValue) { Value = node.Lexem; } }src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (1)
605-613: Nested annotation parameter parsing: OKEarly path for LexemType.Annotation is correct and integrates with AnnotationParameterNode.AnnotationNode.
Minor: consider TryParseNode wrapping to surface a false return if inner annotation parsing fails, mirroring error-driven control flow elsewhere.
tests/annotations.os (1)
23-47: LGTM: test covers nested annotation-as-value end-to-endGood assertions on types and recursive shape. Consider also asserting parameters count for the nested annotation (expected 2) to catch regressions earlier.
ПерваяАннотация = СтрокаМетода.Аннотации[0]; ПервыйПараметрПервойАннотации = ПерваяАннотация.Параметры[0]; + юТест.ПроверитьРавенство(ПервыйПараметрПервойАннотации.Значение.Параметры.Количество(), 2);src/OneScript.Core/Values/BslAnnotationValue.cs (1)
41-59: Make ToString robust for positional and name-only paramsRendering always as Name=Index prints “= -1” for name-only or missing values and prints a leading “=” for positional values. Safer formatting improves debugging and potential constant de-dup.
- if (Parameters.Count != 0) + if (Parameters.Count != 0) { var prefix = "("; foreach (var parameter in Parameters) { sb.Append(prefix); prefix = ","; - sb.Append(parameter.Name); - sb.Append("="); - sb.Append(parameter.ConstantValueIndex); + if (string.IsNullOrEmpty(parameter.Name)) + { + sb.Append(parameter.ConstantValueIndex); + } + else if (parameter.ConstantValueIndex == -1) + { + sb.Append(parameter.Name); + } + else + { + sb.Append(parameter.Name).Append("=").Append(parameter.ConstantValueIndex); + } } sb.Append(")"); }src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
1212-1236: Handle positional and name-only params in annotation presentationCurrent presentation always prints “Name=constNumber” and will produce “= -1” or “=number” without a name. Improve readability and future de-dup robustness.
- sb.Append(parameter.Name); - sb.Append("="); - sb.Append(constValue.constNumber); + if (!string.IsNullOrEmpty(parameter.Name) && constValue.constNumber != -1) + { + sb.Append(parameter.Name).Append("=").Append(constValue.constNumber); + } + else if (!string.IsNullOrEmpty(parameter.Name)) + { + sb.Append(parameter.Name); + } + else + { + sb.Append(constValue.constNumber); + }
1300-1306: De-duplicate annotation constants by (Type, Presentation) like other constsRegisterAnnotationConst always appends, so identical annotations produce duplicate entries. Reuse existing index if presentation matches.
- private int RegisterAnnotationConst(in ConstDefinition cDef, BslAnnotationValue value) - { - var idx = _constMap.Count; - _constMap.Add(cDef); - _module.Constants.Add(value); - return idx; - } + private int RegisterAnnotationConst(in ConstDefinition cDef, BslAnnotationValue value) + { + var idx = _constMap.IndexOf(cDef); + if (idx >= 0) + return idx; + idx = _constMap.Count; + _constMap.Add(cDef); + _module.Constants.Add(value); + return idx; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs(1 hunks)src/OneScript.Core/Types/BasicTypes.cs(2 hunks)src/OneScript.Core/Values/BslAnnotationValue.cs(1 hunks)src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationParameterNode.cs(1 hunks)src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs(2 hunks)src/OneScript.Native/Compiler/CompilerHelpers.cs(1 hunks)src/OneScript.StandardLibrary/Reflector.cs(1 hunks)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs(4 hunks)src/ScriptEngine/Machine/AnnotationDefinition.cs(1 hunks)src/ScriptEngine/Machine/AnnotationParameter.cs(1 hunks)src/ScriptEngine/Machine/Core.cs(1 hunks)src/Tests/OneScript.Core.Tests/CodeGenerationTests.cs(2 hunks)src/Tests/OneScript.Language.Tests/ParserTests.cs(1 hunks)tests/annotations.os(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
src/ScriptEngine/Machine/AnnotationParameter.cs (2)
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (1)
AnnotationParameter(50-58)src/OneScript.Core/Values/BslAnnotationValue.cs (1)
ToString(41-59)
src/ScriptEngine/Machine/AnnotationDefinition.cs (1)
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (1)
AnnotationParameter(50-58)
src/OneScript.Core/Types/BasicTypes.cs (3)
src/OneScript.Core/Types/TypeDescriptor.cs (3)
TypeDescriptor(14-79)TypeDescriptor(22-25)TypeDescriptor(27-33)src/ScriptEngine/Machine/DefaultTypeManager.cs (3)
TypeDescriptor(44-58)TypeDescriptor(78-97)TypeDescriptor(127-130)src/OneScript.Core/Values/BslAnnotationValue.cs (2)
BslAnnotationValue(18-60)BslAnnotationValue(20-22)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
src/OneScript.Native/Compiler/CompilerHelpers.cs (2)
BslAnnotationParameter(90-114)BslPrimitiveValue(22-34)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
BslAnnotationParameter(1172-1176)BslPrimitiveValue(1178-1198)
src/Tests/OneScript.Language.Tests/ParserTests.cs (1)
src/Tests/OneScript.Language.Tests/TreeValidatorExtensions.cs (1)
Is(66-69)
src/Tests/OneScript.Core.Tests/CodeGenerationTests.cs (1)
src/OneScript.Core/Values/BslAnnotationValue.cs (2)
BslAnnotationValue(18-60)BslAnnotationValue(20-22)
src/OneScript.Native/Compiler/CompilerHelpers.cs (5)
src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (1)
AnnotationNode(548-553)src/OneScript.Core/Values/BslAnnotationValue.cs (2)
BslAnnotationValue(18-60)BslAnnotationValue(20-22)src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationParameterNode.cs (2)
AnnotationParameterNode(12-44)AnnotationParameterNode(14-16)src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
BslAnnotationParameter(38-51)BslAnnotationParameter(40-44)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (1)
BslAnnotationParameter(1172-1176)
src/OneScript.StandardLibrary/Reflector.cs (6)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (2)
Set(91-95)Set(97-102)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
ValueTable(31-36)src/OneScript.Native/Compiler/CompilerHelpers.cs (3)
IEnumerable(50-56)IEnumerable(83-88)BslAnnotationParameter(90-114)src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
BslAnnotationParameter(38-51)BslAnnotationParameter(40-44)src/ScriptEngine/Machine/ValueFactory.cs (1)
ValueFactory(17-175)src/OneScript.Core/Values/BslAnnotationValue.cs (2)
BslAnnotationValue(18-60)BslAnnotationValue(20-22)
src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationParameterNode.cs (3)
src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (1)
AnnotationNode(548-553)src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationNode.cs (2)
AnnotationNode(13-24)AnnotationNode(15-21)src/OneScript.Language/SyntaxAnalysis/AstNodes/TerminalNode.cs (3)
TerminalNode(13-32)TerminalNode(17-20)TerminalNode(22-27)
src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (2)
src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationNode.cs (2)
AnnotationNode(13-24)AnnotationNode(15-21)src/OneScript.Language/SyntaxAnalysis/ConditionalDirectiveHandler.cs (1)
NextLexem(239-258)
src/OneScript.Core/Values/BslAnnotationValue.cs (5)
src/OneScript.Native/Compiler/CompilerHelpers.cs (2)
BslPrimitiveValue(22-34)BslAnnotationParameter(90-114)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
BslPrimitiveValue(1178-1198)BslAnnotationParameter(1172-1176)src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
BslAnnotationParameter(38-51)BslAnnotationParameter(40-44)src/OneScript.Core/Types/BasicTypes.cs (2)
BasicTypes(12-41)BasicTypes(29-40)src/ScriptEngine/Machine/AnnotationParameter.cs (1)
ToString(22-33)
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (4)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
BslAnnotationParameter(38-51)BslAnnotationParameter(40-44)src/OneScript.Native/Compiler/CompilerHelpers.cs (3)
BslAnnotationParameter(90-114)BslPrimitiveValue(22-34)Type(64-81)src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationParameterNode.cs (2)
AnnotationParameterNode(12-44)AnnotationParameterNode(14-16)src/OneScript.Core/Values/BslAnnotationValue.cs (3)
BslAnnotationValue(18-60)BslAnnotationValue(20-22)ToString(41-59)
🔇 Additional comments (9)
src/ScriptEngine/Machine/AnnotationDefinition.cs (1)
18-19: LGTM: lightweight holder with safe ParamCountStruct layout and null-safe ParamCount look good.
src/OneScript.Native/Compiler/CompilerHelpers.cs (1)
22-34: No action needed: default LexemType.NotALexem confirmed
NotALexem is the first member of the enum, so default(LexemType) == LexemType.NotALexem and ValueFromLiteral won’t see an uninitialized lexem.src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationParameterNode.cs (1)
42-43: API addition looks goodExposing AnnotationNode to represent nested annotation values is consistent with parser changes.
src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (2)
544-547: Refactor: BuildAnnotations → helper usage is cleanExtraction to BuildAnnotationDefinition improves readability and reuse.
548-553: BuildAnnotationDefinition is correct and side-effect safeConstructs node, advances, and delegates to parameter builder as expected.
src/OneScript.StandardLibrary/Reflector.cs (2)
166-171: Nice recursive expansion of nested annotation valuesCreating a nested annotations table via EmptyAnnotationsTable and FillAnnotationParameters is clean and matches tests’ expectations for СтрокаТаблицыЗначений in the “Значение” column.
Also applies to: 173-201
193-197: Null → Неопределено conversion is preserved
ValueTableRow.Set delegates to column.ValueType.AdjustValue(value), and AdjustValue(null) returns ValueFactory.Create() (Undefined), so name-only parameters without a literal still produce «Неопределено».src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
1174-1176: Parameter constant wiring looks correctTuple return simplifies flow; ConstantValueIndex is set consistently for both literals and nested annotations.
1243-1247: LGTM: constructor overload usageUsing new BslAnnotationAttribute(name, params) simplifies attribute construction.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/ScriptEngine/Machine/AnnotationParameter.cs (1)
28-33: Fix NRE in ToString when RuntimeValue is null; fall back to ValueIndex.If ValueIndex is defined but RuntimeValue is null, RuntimeValue.ToString() will throw. Use a safe fallback to ValueIndex.
- if (ValueIndex == UNDEFINED_VALUE_INDEX && RuntimeValue == null) + if (ValueIndex == UNDEFINED_VALUE_INDEX && RuntimeValue == null) { return Name; } - return string.Format("{0}=[{1}]", Name, RuntimeValue.ToString()); + return string.Format("{0}=[{1}]", Name, RuntimeValue != null ? RuntimeValue.ToString() : ValueIndex.ToString());
🧹 Nitpick comments (1)
src/ScriptEngine/Machine/AnnotationParameter.cs (1)
12-20: Default struct value leaves ValueIndex=0 (not UNDEFINED); ensure all creation sites initialize.A default-initialized struct has ValueIndex=0. If 0 is a valid constant index, unintended “[0]” may appear. Verify all construction paths set either ValueIndex or RuntimeValue.
Optionally add a helper for checks:
public const int UNDEFINED_VALUE_INDEX = -1; + public bool HasValue => ValueIndex != UNDEFINED_VALUE_INDEX || RuntimeValue != null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Reflector.cs(1 hunks)src/ScriptEngine/Machine/AnnotationParameter.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OneScript.StandardLibrary/Reflector.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-30T15:11:29.435Z
Learnt from: dmpas
PR: EvilBeaver/OneScript#1576
File: src/ScriptEngine/Machine/Core.cs:164-166
Timestamp: 2025-08-30T15:11:29.435Z
Learning: In src/ScriptEngine/Machine/ValueFactory.cs, the switch statement over DataType intentionally uses the default case to handle DataType.Annotation rather than requiring an explicit case branch. The default implementation is sufficient for this enum value.
Applied to files:
src/ScriptEngine/Machine/AnnotationParameter.cs
🧬 Code graph analysis (1)
src/ScriptEngine/Machine/AnnotationParameter.cs (2)
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (1)
AnnotationParameter(50-58)src/OneScript.Core/Values/BslAnnotationValue.cs (1)
ToString(41-59)
🔇 Additional comments (1)
src/ScriptEngine/Machine/AnnotationParameter.cs (1)
22-33: Confirm ToString format consistency with BslAnnotationValue.This prints Name=[RuntimeValue or ValueIndex], while BslAnnotationValue prints Name=ConstantValueIndex. Confirm this divergence is intentional for logs/debug output.
| [Fact] | ||
| public void AnnotationsAsValuesInCode() { | ||
| var code = @" | ||
| &Аннотация(Параметр = &ТожеАннотация(&СТожеПараметромАннотацией, П2 = &СТожеПараметромАннотацией)) |
There was a problem hiding this comment.
Парни, я, помнится, уже спрашивал зачем это, но можно еще спрошу? Мне такой синтаксис кажется крайне неудобным и вообще некрасивым. Если уж мы так хотим какие-то DSL сделать для entity и ОСени, может просто сделаем эти DSL удобными?
У нас и так параметр аннотации может быть довольно свободным по синтаксису:
- просто идентификатор
- константа bsl
- идентификатор=константа bsl
Мы можем тут еще что-то придумать, давайте не абстрактный пример разберем, а прикладной. Иначе мне прям очень не нравится появление еще одного примитивного типа "аннотация"
There was a problem hiding this comment.
Ну так прикладной пример у нас в ОСени, там есть "аннотации значения", как например &Деталька(), которая например подтянет тебе значение переменной окружения\конфига.
Вот мы и подумали что дать возможность пользователю переложить это самое значение из одной аннотации в другую лёгким движением руки это отличная идея.
Если нужен прям пример использования, то вот например в OneUnit есть аннотация &Таймаут(), там 1 параметр который собственно принимает число милисекунд за которое тест должен завершится или он покраснеет.
Ты как пользователь OneUnit можешь захотеть для какой-то группы тестов управлять таймаут динамически в зависимости от окружения, сейчас такое сделать не получится.
А с доработками из этого реквеста (и некоторых доработок ОСени конечно же) у тебя будет возможность сделать &Таймаут(&Деталька("ТаймаутДляДолгихТестов", ЗначениеПоУмолчанию = 500))
Что позволит тебе управлять таймаутом этих тестов либо через autumn-properties.json либо просто через переменную окружения ТаймаутДляДолгихТестов, ну и это достаточно быстро и понятно для конечного пользователя как мне видится.
Расскажи пожалуйста поподробнее что ты видишь в этом неудобного и некрасивого? И как вот этот пример на твой взгляд выглядел бы лучше?
There was a problem hiding this comment.
@EvilBeaver kindly ping
Если вдруг будет минутка, посмотри пожалуйста мой ответ выше.)
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/OneScript.Core/Contexts/BslAnnotationAttribute.cs (1)
38-50: BslAnnotationValue.ToString() will print type names without this ToString overrideBslAnnotationValue appends each parameter directly. Without an override here, it prints the CLR type name. Implement a compact “name=value” formatting.
public class BslAnnotationParameter { public BslAnnotationParameter(string name, BslPrimitiveValue value = null) { Name = name; Value = value; } public string Name { get; } public BslPrimitiveValue Value { get; } + public override string ToString() + { + if (Value == null) + return Name ?? string.Empty; + return string.IsNullOrEmpty(Name) ? Value.ToString() : $"{Name}={Value}"; + } }src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (1)
41-47: Null-safe handling of attributes without parametersattribute.Parameters can be null for parameterless annotations, causing NRE in Select(...).
- return new AnnotationDefinition - { - Name = attribute.Name, - Parameters = attribute.Parameters.Select(ToMachineDefinition).ToArray() - }; + return new AnnotationDefinition + { + Name = attribute.Name, + Parameters = (attribute.Parameters ?? Enumerable.Empty<BslAnnotationParameter>()) + .Select(ToMachineDefinition) + .ToArray() + };
♻️ Duplicate comments (1)
src/ScriptEngine/Machine/AnnotationParameter.cs (1)
23-35: ToString: NRE concern resolved; looks goodNull-check on RuntimeValue fixes the previously reported NRE risk.
🧹 Nitpick comments (3)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (1)
22-26: Defensive copy parameters in ctorTwo-arg ctor assigns the incoming enumerable directly; SetParameters makes a copy. Mirror that behavior here to avoid aliasing.
- public BslAnnotationAttribute(string name, IEnumerable<BslAnnotationParameter> parameters) - { - Name = name; - Parameters = parameters; - } + public BslAnnotationAttribute(string name, IEnumerable<BslAnnotationParameter> parameters) + { + Name = name; + Parameters = parameters != null ? new List<BslAnnotationParameter>(parameters) : null; + }src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
1178-1191: Avoid unused variable; keep side-effect explicitconstNumber is computed but unused. Use discard to signal intentional side-effect.
- var constNumber = CreateAnnotationConstDefinition(runtimeValue); + _ = CreateAnnotationConstDefinition(runtimeValue);
1206-1211: Inline trivial wrapper or document intentCreateAnnotationConstDefinition only forwards to RegisterAnnotationConst. Either inline or add a comment explaining the abstraction boundary.
- private int CreateAnnotationConstDefinition(BslAnnotationValue runtimeValue) - { - var result = RegisterAnnotationConst(runtimeValue); - return result; - } + // Kept as a customization point for future serialization/metadata steps. + private int CreateAnnotationConstDefinition(BslAnnotationValue runtimeValue) + => RegisterAnnotationConst(runtimeValue);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs(1 hunks)src/OneScript.Core/Values/BslAnnotationValue.cs(1 hunks)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs(4 hunks)src/ScriptEngine/Machine/AnnotationParameter.cs(1 hunks)src/ScriptEngine/Machine/Core.cs(0 hunks)src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs(1 hunks)src/ScriptEngine/Machine/StackRuntimeModule.cs(1 hunks)src/Tests/OneScript.Core.Tests/CodeGenerationTests.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/ScriptEngine/Machine/Core.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/OneScript.Core/Values/BslAnnotationValue.cs
- src/Tests/OneScript.Core.Tests/CodeGenerationTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-30T15:11:29.435Z
Learnt from: dmpas
PR: EvilBeaver/OneScript#1576
File: src/ScriptEngine/Machine/Core.cs:164-166
Timestamp: 2025-08-30T15:11:29.435Z
Learning: In src/ScriptEngine/Machine/ValueFactory.cs, the switch statement over DataType intentionally uses the default case to handle DataType.Annotation rather than requiring an explicit case branch. The default implementation is sufficient for this enum value.
Applied to files:
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cssrc/ScriptEngine/Machine/AnnotationParameter.cs
🧬 Code graph analysis (5)
src/ScriptEngine/Machine/StackRuntimeModule.cs (1)
src/OneScript.Core/Values/BslAnnotationValue.cs (2)
BslAnnotationValue(18-57)BslAnnotationValue(20-22)
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (3)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
BslAnnotationParameter(38-50)BslAnnotationParameter(40-44)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
BslAnnotationParameter(1172-1176)BslPrimitiveValue(1178-1204)src/OneScript.Native/Compiler/CompilerHelpers.cs (2)
BslAnnotationParameter(90-114)BslPrimitiveValue(22-34)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
BslAnnotationParameter(1172-1176)BslPrimitiveValue(1178-1204)src/OneScript.Native/Compiler/CompilerHelpers.cs (2)
BslAnnotationParameter(90-114)BslPrimitiveValue(22-34)
src/ScriptEngine/Machine/AnnotationParameter.cs (2)
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (1)
AnnotationParameter(50-57)src/OneScript.Core/Values/BslAnnotationValue.cs (1)
ToString(40-56)
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (4)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
BslAnnotationParameter(38-50)BslAnnotationParameter(40-44)src/OneScript.Native/Compiler/CompilerHelpers.cs (3)
BslAnnotationParameter(90-114)BslPrimitiveValue(22-34)Type(64-81)src/OneScript.Language/SyntaxAnalysis/AstNodes/AnnotationParameterNode.cs (2)
AnnotationParameterNode(12-44)AnnotationParameterNode(14-16)src/OneScript.Core/Values/BslAnnotationValue.cs (2)
BslAnnotationValue(18-57)BslAnnotationValue(20-22)
🔇 Additional comments (5)
src/ScriptEngine/Machine/AnnotationParameter.cs (1)
20-20: Remove unusedUNDEFINED_VALUE_INDEXin AnnotationParameter.cs
The constant is defined at src/ScriptEngine/Machine/AnnotationParameter.cs:20 and never referenced elsewhere—remove it to eliminate dead code.⛔ Skipped due to learnings
Learnt from: dmpas PR: EvilBeaver/OneScript#1576 File: src/ScriptEngine/Machine/Core.cs:164-166 Timestamp: 2025-08-30T15:11:29.435Z Learning: In src/ScriptEngine/Machine/ValueFactory.cs, the switch statement over DataType intentionally uses the default case to handle DataType.Annotation rather than requiring an explicit case branch. The default implementation is sufficient for this enum value.src/ScriptEngine/Machine/StackRuntimeModule.cs (1)
35-35: LGTM: annotation values storage addedInternal list for BslAnnotationValue is appropriate for constant registration use.
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (1)
83-85: Cast safety verified: RuntimeValue is always a BslPrimitiveValue
All assignments toAnnotationParameter.RuntimeValuecome fromparameter.Value, which is defined as aBslPrimitiveValuein its model. No otherIValuesubtypes are ever assigned toRuntimeValue. You can safely keep the direct cast.src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
15-15: No action on added usingOK to keep; not flagging unused imports here.
1271-1276: LGTM: annotation constants registrationStraightforward append; index semantics match Constants/MethodRefs patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/ScriptEngine/Machine/AnnotationParameter.cs (1)
17-20: NRE risk resolved in ToStringSwitching to Utils.NameAndValuePresentation eliminates null deref when RuntimeValue is null. Good fix.
🧹 Nitpick comments (3)
src/OneScript.Core/Commons/Utils.cs (1)
38-50: Minor: avoid List allocation and make intent explicitBranching is simpler and allocation-free.
Apply:
- public static string NameAndValuePresentation(string name, object value) - { - var list = new List<string>(); - if (!string.IsNullOrEmpty(name)) - { - list.Add(name); - } - if (value != null) - { - list.Add(value.ToString()); - } - return string.Join("=", list); - } + public static string NameAndValuePresentation(string name, object value) + { + if (!string.IsNullOrEmpty(name) && value != null) + return $"{name}={value}"; + if (!string.IsNullOrEmpty(name)) + return name; + return value?.ToString() ?? string.Empty; + }src/ScriptEngine/Machine/AnnotationParameter.cs (1)
11-16: Consider making the struct immutable with a ctorPublic mutable fields are easy to misuse. Optional: add a constructor and prefer init-only assignment at call sites.
Example:
public struct AnnotationParameter { public AnnotationParameter(string name, IValue runtimeValue) { Name = name; RuntimeValue = runtimeValue; } public string Name { get; } public IValue RuntimeValue { get; } }If helpful, I can draft the call-site updates.
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (1)
1178-1203: Skip empty child parameters to avoid empty slots in rendered annotationsIf a child parameter has no name and no value, BslAnnotationValue.ToString will emit an empty entry (e.g., “&(,)”). Guard and skip.
Apply:
- foreach (var child in param.AnnotationNode.Children) - { - var parameter = (AnnotationParameterNode)child; - var parameterValue = MakeAnnotationParameterValueConstant(parameter); - runtimeValue.Parameters.Add(new BslAnnotationParameter(parameter.Name, parameterValue)); - } + foreach (var child in param.AnnotationNode.Children) + { + var parameter = (AnnotationParameterNode)child; + var parameterValue = MakeAnnotationParameterValueConstant(parameter); + if (!string.IsNullOrEmpty(parameter.Name) || parameterValue != null) + { + runtimeValue.Parameters.Add(new BslAnnotationParameter(parameter.Name, parameterValue)); + } + }Optional micro: pre-size the list
// before loop runtimeValue.Parameters.Capacity = param.AnnotationNode.Children.Count;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/OneScript.Core/Commons/Utils.cs(1 hunks)src/OneScript.Core/Contexts/BslAnnotationAttribute.cs(3 hunks)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs(3 hunks)src/ScriptEngine/Machine/AnnotationDefinition.cs(1 hunks)src/ScriptEngine/Machine/AnnotationParameter.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ScriptEngine/Machine/AnnotationDefinition.cs
- src/OneScript.Core/Contexts/BslAnnotationAttribute.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/ScriptEngine/Machine/AnnotationParameter.cs (3)
src/ScriptEngine/Machine/StackRuntimeAdoptionExtensions.cs (1)
AnnotationParameter(50-57)src/OneScript.Core/Values/BslAnnotationValue.cs (1)
ToString(40-56)src/OneScript.Core/Commons/Utils.cs (2)
Utils(12-54)NameAndValuePresentation(38-50)
src/OneScript.Core/Commons/Utils.cs (2)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (1)
ToString(51-54)src/ScriptEngine/Machine/AnnotationParameter.cs (1)
ToString(17-20)
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (3)
src/OneScript.Core/Contexts/BslAnnotationAttribute.cs (2)
BslAnnotationParameter(39-56)BslAnnotationParameter(41-45)src/OneScript.Native/Compiler/CompilerHelpers.cs (3)
BslAnnotationParameter(90-114)BslPrimitiveValue(22-34)Type(64-81)src/OneScript.Core/Values/BslAnnotationValue.cs (2)
BslAnnotationValue(18-57)BslAnnotationValue(20-22)
🔇 Additional comments (2)
src/OneScript.Core/Commons/Utils.cs (1)
38-50: Centralized, null-safe formatting looks goodThis unifies parameter presentation and prevents NREs across call sites. LGTM.
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (1)
1173-1176: LGTM: unified path to produce runtime values for parametersThe split into MakeAnnotationParameter + MakeAnnotationParameterValueConstant keeps GetAnnotationParameters concise and readable.
Summary by CodeRabbit
New Features
Tests