fix #1675: возвращаемый тип ЗагрузитьСценарий()#1686
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR standardizes compilation error handling across three script-loading methods ( ChangesScript compilation error handling and LoadScript API change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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}
Contributor
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/OneScript.StandardLibrary/DynamicLoadingFunctions.cs (1)
76-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent exception handling: first branch not wrapped.
Line 78 (
externalContext == nullbranch) can also throwSyntaxErrorExceptionorCompilerExceptionbut is not wrapped in the try-catch. This means compilation errors will be handled differently depending on whetherexternalContextis provided, which is inconsistent withAttachScriptandLoadScript.🐛 Proposed fix to wrap both branches
var compiler = _engine.GetCompilerService(); - if (externalContext == null) - { - return _engine.AttachedScriptsFactory.LoadFromString(compiler, code, process); - } - else + try { - var extData = new ExternalContextData(); - - foreach (var item in externalContext) + if (externalContext == null) { - extData.Add(item.Key.ToString()!, item.Value); + return _engine.AttachedScriptsFactory.LoadFromString(compiler, code, process); } - - try - { - return _engine.AttachedScriptsFactory.LoadFromString(compiler, code, process, extData); - } - catch (Exception e) when (e is SyntaxErrorException || e is CompilerException) + else { - throw ScriptCompilingException(e); + var extData = new ExternalContextData(); + + foreach (var item in externalContext) + { + extData.Add(item.Key.ToString()!, item.Value); + } + + return _engine.AttachedScriptsFactory.LoadFromString(compiler, code, process, extData); } } + catch (Exception e) when (e is SyntaxErrorException || e is CompilerException) + { + throw ScriptCompilingException(e); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs` around lines 76 - 97, The null-branch return path using _engine.AttachedScriptsFactory.LoadFromString(compiler, code, process) isn't wrapped like the non-null branch, causing inconsistent handling of SyntaxErrorException/CompilerException; refactor so both paths call LoadFromString inside the same try/catch and rethrow via ScriptCompilingException(e), e.g., prepare extData only when externalContext != null but invoke _engine.AttachedScriptsFactory.LoadFromString(...) inside a single try block that catches (Exception e) when (e is SyntaxErrorException || e is CompilerException) and throws ScriptCompilingException(e).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs`:
- Around line 181-186: The ScriptCompilingException helper currently drops the
original exception when wrapping, so update the
ScriptCompilingException(Exception e) method to call the RuntimeException
constructor overload that accepts an inner exception: build the localized
message (using BilingualString.Localize as before) and pass it as the first
argument and pass e as the second argument to RuntimeException so the original
exception/stack trace is preserved.
---
Outside diff comments:
In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs`:
- Around line 76-97: The null-branch return path using
_engine.AttachedScriptsFactory.LoadFromString(compiler, code, process) isn't
wrapped like the non-null branch, causing inconsistent handling of
SyntaxErrorException/CompilerException; refactor so both paths call
LoadFromString inside the same try/catch and rethrow via
ScriptCompilingException(e), e.g., prepare extData only when externalContext !=
null but invoke _engine.AttachedScriptsFactory.LoadFromString(...) inside a
single try block that catches (Exception e) when (e is SyntaxErrorException || e
is CompilerException) and throws ScriptCompilingException(e).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 740ebbc4-0bd0-4806-a4bb-bec128ecee9b
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/DynamicLoadingFunctions.cstests/native-lib/test-native-use.os
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs (1)
178-182:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a loader-agnostic compilation error message.
This helper is shared by
AttachScript,LoadScriptFromString, andLoadScript, but the text still says "attached script". Compilation failures fromЗагрузитьСценарий*will therefore surface a misleading error.Suggested fix
private static RuntimeException ScriptCompilingException(Exception e) { return new RuntimeException(BilingualString.Localize( - "Ошибка компиляции подключаемого сценария:\n", - "Error compiling attached script:\n") + e.Message, e); + "Ошибка компиляции сценария:\n", + "Error compiling script:\n") + e.Message, e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs` around lines 178 - 182, The helper ScriptCompilingException currently hardcodes "attached script" in its message which is misleading for LoadScriptFromString/LoadScript/AttachScript consumers; change the localized strings passed to BilingualString.Localize in ScriptCompilingException to a loader-agnostic phrase such as "Ошибка компиляции сценария:\n" / "Error compiling script:\n" so all callers (AttachScript, LoadScriptFromString, LoadScript) receive a neutral error message while preserving the appended e.Message and exception chaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs`:
- Around line 178-182: The helper ScriptCompilingException currently hardcodes
"attached script" in its message which is misleading for
LoadScriptFromString/LoadScript/AttachScript consumers; change the localized
strings passed to BilingualString.Localize in ScriptCompilingException to a
loader-agnostic phrase such as "Ошибка компиляции сценария:\n" / "Error
compiling script:\n" so all callers (AttachScript, LoadScriptFromString,
LoadScript) receive a neutral error message while preserving the appended
e.Message and exception chaining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06b06c48-ac2a-4dd7-804b-f0bbdd2c6515
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs (1)
179-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a neutral compile-error prefix here.
This helper is shared by
AttachScript,LoadScriptFromString, andLoadScript, but the localized text says "attached script". That makes load failures surface the wrong operation name to the user.Proposed fix
return new RuntimeException(BilingualString.Localize( - "Ошибка компиляции подключаемого сценария:\n", - "Error compiling attached script:\n") + e.Message, e); + "Ошибка компиляции сценария:\n", + "Error compiling script:\n") + e.Message, e);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs` around lines 179 - 183, The helper ScriptCompilingException uses a localized message that says "attached script" which is incorrect for callers like AttachScript, LoadScriptFromString, and LoadScript; change the localized prefix to a neutral compile-error phrase (e.g., replace the bilingual strings "Ошибка компиляции подключаемого сценария:\n" / "Error compiling attached script:\n" with neutral alternatives such as "Ошибка компиляции сценария:\n" / "Error compiling script:\n") so all callers receive a correct, operation-agnostic error message while preserving the existing exception wrapping (return new RuntimeException(..., e)).
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs (1)
71-73: ⚡ Quick winAdd explicit null guards for
codeandpath.These public script entry points still forward concrete string parameters straight into the engine. Script calls omit them as
"", but direct CLR callers can still passnull, so it is better to fail fast here instead of depending on deeper validation.Based on learnings: in public script-exposed C# methods, omitted script arguments arrive as `""`, but direct calls may still pass `null`, so concrete string parameters should handle null explicitly.Proposed fix
public UserScriptContextInstance LoadScriptFromString(IBslProcess process, string code, StructureImpl externalContext = null) { + if (code == null) + throw new ArgumentNullException(nameof(code)); + var compiler = _engine.GetCompilerService(); try { if (externalContext == null) return _engine.AttachedScriptsFactory.LoadFromString(compiler, code, process); @@ public UserScriptContextInstance LoadScript(IBslProcess process, string path, StructureImpl externalContext = null) { + if (path == null) + throw new ArgumentNullException(nameof(path)); + var compiler = _engine.GetCompilerService(); try { if(externalContext == null) return _engine.AttachedScriptsFactory.LoadFromPath(compiler, path, process);Also applies to: 103-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs` around lines 71 - 73, Add explicit null-guards for public string parameters so callers get fast, clear failures: in LoadScriptFromString(IBslProcess process, string code, StructureImpl externalContext = null) check if code is null and throw ArgumentNullException(nameof(code)) before forwarding to the engine; do the same for the analogous method that takes a path (e.g., LoadScriptFromPath(IBslProcess process, string path, StructureImpl externalContext = null)) by checking path for null and throwing ArgumentNullException(nameof(path)) so direct CLR callers cannot pass null through to deeper validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs`:
- Around line 179-183: The helper ScriptCompilingException uses a localized
message that says "attached script" which is incorrect for callers like
AttachScript, LoadScriptFromString, and LoadScript; change the localized prefix
to a neutral compile-error phrase (e.g., replace the bilingual strings "Ошибка
компиляции подключаемого сценария:\n" / "Error compiling attached script:\n"
with neutral alternatives such as "Ошибка компиляции сценария:\n" / "Error
compiling script:\n") so all callers receive a correct, operation-agnostic error
message while preserving the existing exception wrapping (return new
RuntimeException(..., e)).
---
Nitpick comments:
In `@src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs`:
- Around line 71-73: Add explicit null-guards for public string parameters so
callers get fast, clear failures: in LoadScriptFromString(IBslProcess process,
string code, StructureImpl externalContext = null) check if code is null and
throw ArgumentNullException(nameof(code)) before forwarding to the engine; do
the same for the analogous method that takes a path (e.g.,
LoadScriptFromPath(IBslProcess process, string path, StructureImpl
externalContext = null)) by checking path for null and throwing
ArgumentNullException(nameof(path)) so direct CLR callers cannot pass null
through to deeper validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c5000e6-9538-48f7-81d7-34e65ccd4abe
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/DynamicLoadingFunctions.cs
Collaborator
Author
|
Не исправлено:
|
Mr-Rm
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ЗагрузитьСценарийтеперь возвращает тот же тип, что иЗагрузитьСценарийИзСтроки- который вернулаAttachedScriptsFactory.Load*Summary by CodeRabbit
Bug Fixes
Refactor
Tests