-
Notifications
You must be signed in to change notification settings - Fork 113
Возможность предварительной компиляции скрипта для ускорения запуска #1616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Возможность предварительной компиляции скрипта для ускорения запуска #1616
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds MessagePack dependency and module persistence for precompiled runtime modules, a ModuleSerializer for save/load, a CLI "-build" behavior to emit .osc files, a build behavior that compiles and saves modules, an engine overload to create processes from IExecutableModule, and ExecuteScriptBehavior branching to load .osc or source. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as BehaviorSelector
participant Build as BuildPrecompiledBehavior
participant Hosted as HostedScriptEngine
participant Serializer as ModuleSerializer
participant Disk
rect rgb(220, 240, 255)
note over User,CLI: Build flow (-build)
User->>CLI: invoke -build script.os
CLI->>Build: Create & Execute
Build->>Hosted: Load source from file
Build->>Hosted: Compile -> IExecutableModule
Build->>Serializer: SaveModule(module, script.osc)
Serializer->>Disk: write MessagePack bytes
Build-->>User: exit code (0/1)
end
rect rgb(255, 235, 220)
note over User,CLI: Execute flow (script.osc)
User->>CLI: run script.osc
CLI->>Hosted: ExecuteScriptBehavior
Hosted->>Serializer: LoadModule(script.osc)
Serializer->>Disk: read MessagePack bytes
Serializer-->>Hosted: return IExecutableModule
Hosted->>Hosted: CreateProcess(host, module)
Hosted-->>User: start process / run script
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/oscript/ExecuteScriptBehavior.cs (1)
56-84: Consider refactoring duplicated error handling.Both the precompiled and interpreted branches have identical exception handling logic (lines 66-70 and 79-83). Additionally, catching generic
Exceptionis quite broad and may mask specific error types that should be handled differently.Consider extracting the common error handling or being more specific about caught exceptions:
-bool isPrecompiled = Path.GetExtension(_path).Equals(".osc", StringComparison.OrdinalIgnoreCase); - -Process process; -if (isPrecompiled) -{ - try - { - var module = ModuleSerializer.LoadModule(_path); - process = hostedScript.CreateProcess(this, module); - } - catch (Exception e) - { - ShowExceptionInfo(e); - return 1; - } -} -else +Process process; +try { - try + bool isPrecompiled = Path.GetExtension(_path).Equals(".osc", StringComparison.OrdinalIgnoreCase); + if (isPrecompiled) { - var source = hostedScript.Loader.FromFile(_path); - process = hostedScript.CreateProcess(this, source); + var module = ModuleSerializer.LoadModule(_path); + process = hostedScript.CreateProcess(this, module); } - catch (Exception e) + else { - ShowExceptionInfo(e); - return 1; + var source = hostedScript.Loader.FromFile(_path); + process = hostedScript.CreateProcess(this, source); } } +catch (Exception e) +{ + ShowExceptionInfo(e); + return 1; +}src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (1)
43-58: Consider wrapping file I/O exceptions with more context.The file I/O operations in both
SaveModuleandLoadModulecan fail for various reasons (permissions, disk space, corruption), but the exceptions won't provide context about what was being saved or loaded.Wrap I/O exceptions with contextual information:
public static void SaveModule(IExecutableModule module, string filePath) { var serializableModule = new PrecompiledRuntimeModule { // ... properties ... }; - byte[] bytes = MessagePackSerializer.Serialize(serializableModule); - File.WriteAllBytes(filePath, bytes); + try + { + byte[] bytes = MessagePackSerializer.Serialize(serializableModule); + File.WriteAllBytes(filePath, bytes); + } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed to save precompiled module to '{filePath}'", ex); + } } public static IExecutableModule LoadModule(string filePath) { - byte[] bytes = File.ReadAllBytes(filePath); - return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes); + try + { + byte[] bytes = File.ReadAllBytes(filePath); + return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes); + } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed to load precompiled module from '{filePath}'", ex); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/OneScript.Core/OneScript.Core.csproj(1 hunks)src/ScriptEngine.HostedScript/HostedScriptEngine.cs(1 hunks)src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs(1 hunks)src/oscript/BehaviorSelector.cs(1 hunks)src/oscript/BuildPrecompiledBehavior.cs(1 hunks)src/oscript/ExecuteScriptBehavior.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
🧬 Code graph analysis (4)
src/ScriptEngine.HostedScript/HostedScriptEngine.cs (3)
src/ScriptEngine.HostedScript/Process.cs (2)
Process(13-61)Process(21-31)src/StandaloneRunner/ProcessLoader.cs (1)
Process(25-57)src/ScriptEngine/ScriptingEngine.cs (1)
Initialize(98-106)
src/oscript/BuildPrecompiledBehavior.cs (5)
src/oscript/BehaviorSelector.cs (4)
AppBehavior(15-35)AppBehavior(37-59)AppBehavior(61-74)AppBehavior(76-100)src/oscript/ConsoleHostBuilder.cs (1)
ConsoleHostBuilder(16-56)src/ScriptEngine.HostedScript/HostedScriptEngine.cs (2)
Initialize(59-71)SetGlobalEnvironment(143-148)src/oscript/DoNothingHost.cs (1)
DoNothingHost(14-37)src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (2)
ModuleSerializer(41-65)SaveModule(43-58)
src/oscript/ExecuteScriptBehavior.cs (7)
src/ScriptEngine.HostedScript/HostedScriptEngine.cs (3)
Process(93-118)Process(120-125)Process(150-156)src/ScriptEngine.HostedScript/Process.cs (2)
Process(13-61)Process(21-31)src/StandaloneRunner/ProcessLoader.cs (1)
Process(25-57)src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (1)
ModuleSerializer(41-65)src/ScriptEngine.HostedScript/IHostApplication.cs (1)
ShowExceptionInfo(15-15)src/oscript/ConsoleHostImpl.cs (1)
ShowExceptionInfo(56-65)src/StandaloneRunner/StandaloneApplicationHost.cs (1)
ShowExceptionInfo(24-27)
src/oscript/BehaviorSelector.cs (1)
src/oscript/BuildPrecompiledBehavior.cs (2)
BuildPrecompiledBehavior(15-53)BuildPrecompiledBehavior(19-22)
🔇 Additional comments (2)
src/oscript/BehaviorSelector.cs (1)
50-50: LGTM!The
-buildcommand registration follows the established pattern and correctly maps toBuildPrecompiledBehavior.Create.src/OneScript.Core/OneScript.Core.csproj (1)
12-14: MessagePack 3.1.4 is current, secure, and compatible—no issues found.Version 3.1.4 is the latest available. The resolved target framework is net8.0, which is supported by MessagePack 3.1.4. Regarding security, the package has no known vulnerabilities for this version; earlier vulnerability advisories are for older versions and have all been patched in prior releases.
| try | ||
| { | ||
| var module = compiler.Compile(source, hostedScript.Engine.NewProcess()); | ||
| ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc")); | ||
| } | ||
| catch (ScriptException e) | ||
| { | ||
| Output.WriteLine(e.Message); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete exception handling for compilation errors.
The code catches ScriptException but not CompilerException. The compiler.Compile call in HostedScriptEngine.CreateProcess can throw CompilerException for syntax/compilation errors (see lines 112-116 in HostedScriptEngine.cs). This means compilation errors won't be caught here and will propagate as unhandled exceptions.
Catch CompilerException as well:
try
{
var module = compiler.Compile(source, hostedScript.Engine.NewProcess());
ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc"));
}
+catch (CompilerException e)
+{
+ Output.WriteLine(e.Message);
+ return 1;
+}
catch (ScriptException e)
{
Output.WriteLine(e.Message);
return 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try | |
| { | |
| var module = compiler.Compile(source, hostedScript.Engine.NewProcess()); | |
| ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc")); | |
| } | |
| catch (ScriptException e) | |
| { | |
| Output.WriteLine(e.Message); | |
| return 1; | |
| } | |
| try | |
| { | |
| var module = compiler.Compile(source, hostedScript.Engine.NewProcess()); | |
| ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc")); | |
| } | |
| catch (CompilerException e) | |
| { | |
| Output.WriteLine(e.Message); | |
| return 1; | |
| } | |
| catch (ScriptException e) | |
| { | |
| Output.WriteLine(e.Message); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
In src/oscript/BuildPrecompiledBehavior.cs around lines 34 to 43, the try/catch
only handles ScriptException but not CompilerException thrown by
compiler.Compile, so add handling for CompilerException as well (either a
separate catch for CompilerException that writes e.Message to Output and returns
1, or a multi-exception catch that handles both ScriptException and
CompilerException the same way) to ensure compilation errors are reported and
the method returns 1 instead of letting the exception propagate.
| public Process CreateProcess(IHostApplication host, IExecutableModule module) | ||
| { | ||
| Initialize(); | ||
| var bslProcess = _engine.NewProcess(); | ||
| return InitProcess(bslProcess, host, module); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing SetGlobalEnvironment call may cause runtime issues.
The new overload skips the SetGlobalEnvironment(host, src) call present in the original CreateProcess method (line 96). This means _globalCtx.ApplicationHost and _globalCtx.CodeSource remain uninitialized, which could cause null reference exceptions if the global context accesses these properties during script execution.
Consider whether the global environment should be set. If a SourceCode instance is unavailable for precompiled modules, you might need to create a placeholder or refactor the global context initialization:
public Process CreateProcess(IHostApplication host, IExecutableModule module)
{
Initialize();
+ SetGlobalEnvironment(host, module.Source);
var bslProcess = _engine.NewProcess();
return InitProcess(bslProcess, host, module);
}Alternatively, if SetGlobalEnvironment requires a different approach for precompiled modules, document why it's omitted or add a placeholder implementation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ScriptEngine.HostedScript/HostedScriptEngine.cs around lines 120-125, the
new CreateProcess overload initializes a process but omits calling
SetGlobalEnvironment(host, src), leaving _globalCtx.ApplicationHost and
_globalCtx.CodeSource uninitialized; fix by ensuring the global environment is
set before InitProcess — either call SetGlobalEnvironment(host, src) with an
appropriate SourceCode instance (create a minimal/placeholder SourceCode when
module is precompiled), or explicitly set _globalCtx.ApplicationHost = host and
_globalCtx.CodeSource = placeholderSource before returning the process, and
document the chosen approach if omission is intentional.
| [MessagePackObject] | ||
| public class PrecompiledRuntimeModule : IExecutableModule | ||
| { | ||
| [Key(0)] | ||
| public IList<BslAnnotationAttribute> ModuleAttributes { get; set; } | ||
|
|
||
| [Key(1)] | ||
| public IList<BslScriptFieldInfo> Fields { get; set; } | ||
|
|
||
| [Key(2)] | ||
| public IList<BslScriptPropertyInfo> Properties { get; set; } | ||
|
|
||
| [Key(3)] | ||
| public IList<BslScriptMethodInfo> Methods { get; set; } | ||
|
|
||
| [Key(4)] | ||
| public BslScriptMethodInfo ModuleBody { get; set; } | ||
|
|
||
| [Key(5)] | ||
| public SourceCode Source { get; set; } | ||
|
|
||
| [Key(6)] | ||
| public IDictionary<Type, object> Interfaces { get; set; } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing namespace declaration pollutes global namespace.
Both PrecompiledRuntimeModule and ModuleSerializer classes lack a namespace declaration, placing them in the global namespace. This is poor practice and can lead to naming conflicts.
Add an appropriate namespace:
+namespace ScriptEngine.Machine
+{
+
[MessagePackObject]
public class PrecompiledRuntimeModule : IExecutableModule
{
// ... properties ...
}
public class ModuleSerializer
{
// ... methods ...
}
+
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs around lines 16 to 39,
the PrecompiledRuntimeModule (and the related ModuleSerializer class in the same
file) are declared in the global namespace; wrap these class declarations inside
an appropriate namespace (for example: namespace ScriptEngine.Machine) to avoid
polluting the global namespace and reduce naming conflicts; update the file by
adding the namespace declaration and adjusting indentation or usings as needed
so both classes are enclosed within that namespace.
🧩 Analysis chain
Verify MessagePack serialization compatibility of complex types.
PrecompiledRuntimeModule serializes several complex types (e.g., SourceCode, BslScriptMethodInfo, BslScriptFieldInfo, BslScriptPropertyInfo, BslAnnotationAttribute) that may not have MessagePack serialization support. If these types lack the necessary attributes or custom serializers, serialization will fail at runtime.
Run the following script to check if these types have MessagePack attributes:
If these types don't have MessagePack support, you'll need to either:
- Add MessagePack attributes to them
- Create custom MessagePack formatters
- Use a different serialization approach
🏁 Script executed:
#!/bin/bash
# Search for MessagePack attributes on the types used in PrecompiledRuntimeModule
echo "Checking SourceCode:"
ast-grep --pattern $'[MessagePackObject]
class SourceCode {
$$$
}'
echo -e "\nChecking BslScriptMethodInfo:"
ast-grep --pattern $'[MessagePackObject]
class BslScriptMethodInfo {
$$$
}'
echo -e "\nChecking BslScriptFieldInfo:"
ast-grep --pattern $'[MessagePackObject]
class BslScriptFieldInfo {
$$$
}'
echo -e "\nChecking BslScriptPropertyInfo:"
ast-grep --pattern $'[MessagePackObject]
class BslScriptPropertyInfo {
$$$
}'
echo -e "\nChecking BslAnnotationAttribute:"
ast-grep --pattern $'[MessagePackObject]
class BslAnnotationAttribute {
$$$
}'Length of output: 148
I need to verify this more comprehensively. The ast-grep pattern didn't find results, which could mean these types either lack the attribute or use a different structure. Let me search more broadly for these type definitions and any MessagePack configuration.
🏁 Script executed:
#!/bin/bash
# Search for the type definitions and MessagePack support
echo "=== Searching for SourceCode type definition ==="
rg -n "class SourceCode" --type cs
echo -e "\n=== Searching for BslScriptMethodInfo definition ==="
rg -n "class BslScriptMethodInfo" --type cs
echo -e "\n=== Searching for BslScriptFieldInfo definition ==="
rg -n "class BslScriptFieldInfo" --type cs
echo -e "\n=== Searching for BslScriptPropertyInfo definition ==="
rg -n "class BslScriptPropertyInfo" --type cs
echo -e "\n=== Searching for BslAnnotationAttribute definition ==="
rg -n "class BslAnnotationAttribute" --type cs
echo -e "\n=== Searching for custom MessagePack formatters ==="
rg -n "IMessagePackFormatter" --type cs -A2
echo -e "\n=== Searching for MessagePackObject or MessagePack attributes in the codebase ==="
rg -n "MessagePackObject|MessagePack" --type cs -C1 | head -50Length of output: 2589
I found the type definitions. Now I need to check each one directly to see if they have MessagePack attributes:
🏁 Script executed:
#!/bin/bash
# Check the actual type definitions for MessagePack attributes
echo "=== SourceCode type ==="
head -30 src/OneScript.Language/Sources/SourceCode.cs
echo -e "\n=== BslScriptMethodInfo type ==="
head -40 src/OneScript.Core/Contexts/BslScriptMethodInfo.cs
echo -e "\n=== BslScriptFieldInfo type ==="
head -40 src/OneScript.Core/Contexts/BslScriptFieldInfo.cs
echo -e "\n=== BslScriptPropertyInfo type ==="
head -40 src/OneScript.Core/Contexts/BslScriptPropertyInfo.cs
echo -e "\n=== BslAnnotationAttribute type ==="
head -30 src/OneScript.Core/Contexts/BslAnnotationAttribute.csLength of output: 5949
Now let me check if there's any MessagePack resolver configuration or dynamic registration that might support these types:
🏁 Script executed:
#!/bin/bash
# Check for MessagePack resolver setup or dynamic type registration
echo "=== Searching for MessagePack resolver configuration ==="
rg -n "CompositeResolver|StaticCompositeResolver|FormatterProvider|RegisterFormatter" --type cs
echo -e "\n=== Searching for MessagePack code generation or generated types ==="
rg -n "generated|Generated|Generate" --type cs -i src/ScriptEngine/Machine/ | head -20
echo -e "\n=== Checking if there's a MessagePack config or setup file ==="
find . -name "*.cs" -type f | xargs grep -l "MessagePack.*Setup\|SetupFormatter" 2>/dev/nullLength of output: 190
Add MessagePack support to complex types in PrecompiledRuntimeModule.
Verification confirms that SourceCode, BslScriptMethodInfo, BslScriptFieldInfo, BslScriptPropertyInfo, and BslAnnotationAttribute lack MessagePack serialization attributes. When MessagePackSerializer.Serialize() is called on PrecompiledRuntimeModule instances (line 56), serialization will fail at runtime for these unsupported types.
Add [MessagePackObject] attributes and [Key(n)] annotations to each of these five types, or implement custom IMessagePackFormatter implementations for them.
🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs around lines 16 to 39,
the complex member types (SourceCode, BslScriptMethodInfo, BslScriptFieldInfo,
BslScriptPropertyInfo, BslAnnotationAttribute) are not MessagePack-annotated
causing MessagePackSerializer.Serialize() to fail; add MessagePack support by
decorating each of these types with [MessagePackObject] and adding stable
[Key(n)] attributes for all serializable members (or alternatively implement and
register custom IMessagePackFormatter implementations for each type), ensuring
keys are deterministic and compatible with existing serialized layouts so
PrecompiledRuntimeModule serializes successfully.
| public class ModuleSerializer | ||
| { | ||
| public static void SaveModule(IExecutableModule module, string filePath) | ||
| { | ||
| var serializableModule = new PrecompiledRuntimeModule | ||
| { | ||
| ModuleAttributes = module.ModuleAttributes, | ||
| Fields = module.Fields, | ||
| Properties = module.Properties, | ||
| Methods = module.Methods, | ||
| ModuleBody = module.ModuleBody, | ||
| Source = module.Source, | ||
| Interfaces = module.Interfaces | ||
| }; | ||
|
|
||
| byte[] bytes = MessagePackSerializer.Serialize(serializableModule); | ||
| File.WriteAllBytes(filePath, bytes); | ||
| } | ||
|
|
||
| public static IExecutableModule LoadModule(string filePath) | ||
| { | ||
| byte[] bytes = File.ReadAllBytes(filePath); | ||
| return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add format versioning and validation to prevent compatibility issues.
The serialization format lacks versioning, meaning future changes to PrecompiledRuntimeModule structure will break compatibility with existing .osc files without any clear error message. Additionally, there's no validation of the deserialized data.
Consider adding format versioning and validation:
[MessagePackObject]
public class PrecompiledRuntimeModule : IExecutableModule
{
+ [Key(7)]
+ public int FormatVersion { get; set; } = 1;
+
[Key(0)]
public IList<BslAnnotationAttribute> ModuleAttributes { get; set; }
// ... other properties ...
}
public class ModuleSerializer
{
+ private const int CURRENT_FORMAT_VERSION = 1;
+
public static void SaveModule(IExecutableModule module, string filePath)
{
+ if (module == null)
+ throw new ArgumentNullException(nameof(module));
+
var serializableModule = new PrecompiledRuntimeModule
{
+ FormatVersion = CURRENT_FORMAT_VERSION,
ModuleAttributes = module.ModuleAttributes,
// ... other properties ...
};
byte[] bytes = MessagePackSerializer.Serialize(serializableModule);
File.WriteAllBytes(filePath, bytes);
}
public static IExecutableModule LoadModule(string filePath)
{
+ if (!File.Exists(filePath))
+ throw new FileNotFoundException($"Precompiled module not found: {filePath}");
+
byte[] bytes = File.ReadAllBytes(filePath);
- return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes);
+ var module = MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes);
+
+ if (module.FormatVersion != CURRENT_FORMAT_VERSION)
+ throw new InvalidOperationException(
+ $"Incompatible precompiled module format. Expected version {CURRENT_FORMAT_VERSION}, got {module.FormatVersion}");
+
+ return module;
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs around lines 41 to 65,
the serializer currently writes raw MessagePack bytes with no format version or
validation; add a small framing header that prefixes serialized data with a
fixed magic string and a uint8/uint16 format version constant when saving, and
on load read and validate the magic and version before deserializing (reject
unknown versions with a clear exception or handle via a version switch for
backward compatibility). After deserialization perform basic validation of
required fields (non-null ModuleAttributes/Methods/ModuleBody etc.) and throw a
descriptive exception if validation fails; update SaveModule/LoadModule to use
the header, increment the version constant for future changes, and ensure error
messages provide the magic/version and a hint about incompatibility.
|
Сделай, пожалуйста, статус черновика в пулреквесте, чтобы было понятно, когда wip закончился |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/oscript/BuildPrecompiledBehavior.cs (1)
34-43: Compilation errors fromcompiler.Compileare not handled (CompilerExceptionis missing).
compiler.Compilecan throwCompilerExceptionfor syntax/compilation errors, but onlyScriptExceptionis caught here. In such cases-buildwill crash instead of printing a message and returning non‑zero.Предлагаемый патч:
try { var module = compiler.Compile(source, hostedScript.Engine.NewProcess()); ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc")); } + catch (CompilerException e) + { + Output.WriteLine(e.Message); + return 1; + } catch (ScriptException e) { Output.WriteLine(e.Message); return 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/oscript/BuildPrecompiledBehavior.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
🧬 Code graph analysis (1)
src/oscript/BuildPrecompiledBehavior.cs (5)
src/oscript/BehaviorSelector.cs (4)
AppBehavior(15-35)AppBehavior(37-59)AppBehavior(61-74)AppBehavior(76-100)src/oscript/ConsoleHostBuilder.cs (1)
ConsoleHostBuilder(16-56)src/ScriptEngine.HostedScript/HostedScriptEngine.cs (2)
Initialize(59-71)SetGlobalEnvironment(143-148)src/oscript/DoNothingHost.cs (1)
DoNothingHost(14-37)src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (2)
ModuleSerializer(41-65)SaveModule(43-58)
🔇 Additional comments (1)
src/oscript/BuildPrecompiledBehavior.cs (1)
48-52: FactoryCreatenow returns the correct behavior for-build.The factory correctly instantiates
BuildPrecompiledBehaviorwhen путь передан, and returnsnullotherwise, which aligns with howBehaviorSelectorwires-build.
| using System; | ||
| using System.IO; | ||
| using OneScript.Language; | ||
| using ScriptEngine.Compiler; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModuleSerializer may not resolve without the proper namespace import.
ModuleSerializer.SaveModule is used, but this file only imports ScriptEngine.Compiler. Given ModuleSerializer lives in ScriptEngine.Machine.PrecompiledRuntimeModule.cs, you likely need an explicit using ScriptEngine.Machine; (or a fully‑qualified name) to compile.
Рекомендуемый патч:
using System;
using System.IO;
using OneScript.Language;
using ScriptEngine.Compiler;
+using ScriptEngine.Machine;Also applies to: 36-37
🤖 Prompt for AI Agents
In src/oscript/BuildPrecompiledBehavior.cs around lines 8-12 (and also where
ModuleSerializer.SaveModule is called at lines 36-37), the code references
ModuleSerializer but lacks the namespace import; add "using
ScriptEngine.Machine;" at the top of the file (or replace the ModuleSerializer
references with the fully-qualified ScriptEngine.Machine.ModuleSerializer) so
the compiler can resolve ModuleSerializer.SaveModule.
@EvilBeaver Пересоздать его как draft? Или в текущем что-то проставить можно? |
Хочу попробовать сделать свою версию #1554 с прекомпиляцией скриптов. Только не автоматическую, а ручную
WIP, пока в исследовательских целях)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.