Repository navigation
Conversation
System::Exception is Ruby's Exception, so the Ruby class of a CLR type whose base is System.Exception had Exception as its superclass. That made a custom C# exception (.NET recommends deriving from Exception), AggregateException and ApplicationException and everything below it invisible to a bare rescue, rescue StandardError and the rescue modifier: a script calling such a library died where the same code with a SystemException (NotSupportedException, InvalidOperationException) was rescued. RubyContext now gives such a class StandardError as its superclass, so rescue, is_a?, === and ancestors agree, also for Ruby subclasses of it. System::Exception itself stays Ruby's Exception and Ruby's own exception classes keep their MRI superclasses: they are defined by the library initializers and never take this path. The types emitted for Ruby subclasses of Exception and IronRuby's StackUnwinders (throw, break, Thread#kill) are excluded, as they are not errors. The struct #hash recursion signal was the one internal exception that crosses user code (a user #hash method) and would have become a StandardError. It is a StackUnwinder now, so no rescue sees it, which is what MRI's throw does; before, rescue Exception caught it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ined A C# exception usually reaches Ruby inside a wrapper: a faulted task's AggregateException, reflection's TargetInvocationException, or re-raised by Thread#join. ClrExceptionRescue6 covers those paths. It also pins two consequences of the class deciding, as for any exception: - A wrapper is a StandardError whatever it carries, so a bare rescue now catches an AggregateException that holds a SystemExit, an Interrupt or a throw from a Ruby callback. A static hierarchy cannot look at the inner exception. On master such a wrapper reached only rescue Exception, and the exit or throw inside it was lost there as well. - A hosting API's SyntaxErrorException (IronRuby.create_engine.execute) is a CLR exception like any other. eval, require and load still raise Ruby's SyntaxError, a ScriptError, as in MRI. RubyContext asserted nothing when StandardError was not defined yet, so a class made that early would have kept Exception for good without anyone noticing. That cannot happen today, since the initializers define StandardError first, but a Debug.Assert now catches a future ordering change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DefineLibraryClass chose a CLR type's superclass from its base type directly when the library passed none, skipping the StandardError rule in GetOrCreateClassNoLock. A library extending a System.Exception subclass (ExtendClass with no Inherits) then produced an Exception subclass if it loaded first and a StandardError subclass if Ruby had touched the type first. Both paths now share GetClrSuperClassNoLock. The test also pins that System::SystemException === an application exception is true now (StandardError is SystemException's Ruby class), while the CLR type check stays false. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Superclass mapping and unwinding Src/Ruby/Runtime/RubyContext.cs, Src/Ruby/Builtins/RubyStruct.cs |
CLR class creation and library class creation use a shared superclass mapping rule. RecursiveHashException now derives from StackUnwinder. |
Rescue behavior tests Src/IronRuby.Tests/Runtime/ClrExceptionRescueTests.cs, Src/IronRuby.Tests/RubyTests.cs |
Added tests for Ruby rescue behavior, exception hierarchy, exceptions crossing C# and runtime boundaries, and library-defined CLR exception classes. Registered seven new tests. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related PRs
- Largo/ironruby#22: Changes exception processing for
StackUnwinder, which is also used by the recursive hash signal in this PR. - Largo/ironruby#47: Adds a CLR exception type affected by the superclass mapping rule in this PR.
- Largo/ironruby#66: Uses
StackUnwinderfor a fiber reclamation signal that bypasses Ruby rescue clauses.
Suggested reviewers: claude
Merge Risk: 
1939b
-
Direct CLR exception subclasses become catchable by bare rescue. No merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: Ruby rescue behavior for CLR exceptions that directly derive from System.Exception. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
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
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.
Summary
Problem. In IronRuby,
System::Exceptionis Ruby'sException, andStandardErrormaps toSystem.SystemException. A CLR class that is not a Ruby builtin takes the Ruby class of its CLR base type as its superclass (RubyContext.GetOrCreateClassNoLock). So every CLR exception that derives fromSystem.Exceptiondirectly hadExceptionas its superclass and was not aStandardError. That covers a custom C# exception (public class MyAppException : Exception, the base .NET recommends),AggregateException,ApplicationExceptionand everything below them.begin; lib_call; rescue => e; end,rescue StandardErrorand thex rescue ymodifier let these through, and the script died. The same code with aSystemException(NotSupportedException,InvalidOperationException) was rescued.Fix. When the base class of a new CLR class is Ruby's
Exception,GetOrCreateClassNoLocknow gives itStandardErroras its superclass. The fix is in the class hierarchy rather than a special case in the rescue test, sorescue,is_a?,===,ancestorsandsuperclassall agree. That includes Ruby subclasses such asclass E2 < MyAppExceptionandraise MyAppException.new("x")from Ruby. What stays as it was:System::Exceptionitself is unchanged.raise Exception,raise System::Exception, "m",System::Exception.newand a plainnew Exception()thrown from C# still get past a bare rescue, as in MRI.StandardError.Exception: the CLR types emitted for Ruby subclasses ofException(class Foo < Exception), and IronRuby'sStackUnwinders (throw, break, return, Thread#kill, fiber reclaim).ExtendClass(typeof(X), ..., null, ...)) goes through the same rule (GetClrSuperClassNoLock), so the class gets the same superclass whether the library loads before or after the type is first used.RubyStruct's internal recursive-#hashsignal unwinds through user#hashmethods. It is now aStackUnwinder, so no user rescue sees it, which matches MRI's throw. Before,rescue Exceptioncaught it.Behaviour changes (release notes):
AggregateExceptionandTargetInvocationExceptionwhatever they wrap. That includes aSystemExit, anInterruptor athrowfrom a Ruby callback run throughTask.Waitor reflection, and CLR test-framework assertions that derive fromException. On master these reached onlyrescue Exception.Microsoft::Scripting::SyntaxErrorException(IronRuby.create_engine.execute('1+')) is caught by a bare rescue.eval,requireandloadstill raise Ruby'sSyntaxError, which a bare rescue does not catch.System::SystemException === eandrescue System::SystemExceptionnow match such exceptions (StandardErrorisSystemException's Ruby class), while the CLR checkSystemException.IsInstanceOfType(e)stays false. Code that used arescue System::SystemException/rescue System::Exceptionpair to split runtime errors from application errors now lands everything in the first clause. A C# extension method declared onSystemExceptionanswersrespond_to?true on them, but calling it raisesTypeError(master:NoMethodError).< Exception(e.g.class System::AggregateException < Exception) now raises "superclass mismatch".< StandardErrorworks.new System.Exception(...)still gets past a bare rescue: that is Ruby'sException, like MRI'sraise Exception.Testing
New C# tests in
Src/IronRuby.Tests/Runtime/ClrExceptionRescueTests.cs(ClrExceptionRescue1-7, fixtures are nested types):rescue StandardError, the modifier and a method-level rescue catch C#-thrown custom, derived,ApplicationException-based, generic andAggregateExceptionexceptions.superclass,ancestors,is_a?,===and<agree.Exceptionand the classes outsideStandardError.Exception: emitted Ruby subclasses and StackUnwinders. A struct's recursive#hashis not seen by a user rescue.Task.Wait, reflection,Thread#join) and the deliberate wrapper and hosting-SyntaxErrorExceptionbehaviour.LibraryInitializerthat extends a fixture exception gives itStandardError, both when it defines the class and when the type was used first. This test fails ifDefineLibraryClassskips the rule.Measured on this branch against a pristine master build:
MyAppException, a subclass of it andAggregateExceptionget pastrescue => e,rescue StandardErrorand the modifier. On this branch all are caught. A plainSystem.Exceptionand Ruby's non-StandardErrorclasses still get past on both.Thread#killspecs. These are pre-existing: looping the kill/exit/terminate specs 40 times per build gave 6 failures and 1 hang on the branch, and 4 failures and 2 hangs on master, in the same examples.#hashrecursion: output identical on branch, master and CRuby 3.3.🤖 Generated with Claude Code
Summary by CodeRabbit
Exceptionare now generally treated asStandardError, so Ruby rescue clauses handle them consistently. Exceptions with special Ruby or runtime behavior retain their existing rescue semantics.