Let Compile fail with its own exception rather than someone else's (#894) - #895
Merged
Conversation
) Two ways compilation reported the impossible, neither of them the exception written down for it. A node the Linq converter has no case for threw AngouriBugException, which asks the caller to report a bug -- for a gap in coverage the library already knows about. So compiling floor(x), ceil(x), round(x), phi(x), gamma(x) or x! told the user to file an issue, and floor, ceil and round are nodes 2.0 itself added and never taught the compiler. #872 moved twelve other known gaps off AngouriBugException for this reason; these are the same thing. Linq.Expression's own refusals reached the caller unwrapped: an InvalidOperationException for `not x` compiled as a double, an ArgumentException for a Providedf whose condition is not boolean. The mismatch is real -- a boolean node has no double-valued compiled form -- but Docs/Usage/Exceptions.md tells a caller to write catch (AngouriMathBaseException) around a library call, and those are not under it, so they escaped the handler the documentation asks for. Both now throw UncompilableNodeException. The node set and the compiled output are unchanged; only the failure is. Teaching the compiler floor, ceil and round is worth doing and is not this. Found by work/crashcheck: on 1652 cases it reports 0 crashes and 0 hangs, and these 16 were every remaining finding. Measured after the change, the same 118 compile cases produce none. 6284 tests pass, 14 new ones covering both families.
Both changelog sections were inserted at the same anchor, so both are kept: the arccotan correction from #888 continues the inverse-trigonometric entry above it, and the Compile entry follows it.
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.
Closes #894.
Compilereported the impossible with two exceptions that were not the one documentedfor it.
What changed
Nothing about which nodes compile, or about the compiled output. Only what is thrown when
compilation is impossible.
<double, double>floor(x),ceil(x),round(x),phi(x),gamma(x),x!,(x+1)!/x!AngouriBugException: An unary node seems to be not addedUncompilableNodeExceptionnot x,x and 2,x or 2,x xor 2,x implies 2,x -> x + 1,(x -> x + 1) applied to 2InvalidOperationExceptionfromSystem.Linq.ExpressionsUncompilableNodeExceptionx provided 2ArgumentException: Argument must be booleanUncompilableNodeExceptionWhy each is wrong as it stands
AngouriBugExceptionmeans "an internal error occurred, report it". None of the first row is one:the converter has no case for the node, which is a gap in coverage. So a caller who compiled
floor(x)was asked to file a bug report — andfloor,ceilandroundare nodes 2.0 itselfadded (#809) and never taught the compiler, so that is a 2.0 feature asking to be reported as a
defect. #872 moved twelve other known gaps off
AngouriBugExceptionfor exactly this reason.Linq's exceptions are not under
AngouriMathBaseException.Docs/Usage/Exceptions.mdtells acaller that catching
AngouriMathBaseExceptioncatches everything the library throws, and theseescaped that handler entirely. The mismatch itself is real — a boolean node has no
double-valuedcompiled form — so the fix is to report it as what it is, not to make it compile.
What this deliberately does not do
It does not teach the compiler
floor,ceil,round,phi,gammaor the factorial. That isworth doing and is separate work; conflating the two would mean shipping a coverage change under a
correctness fix. This change makes the failure honest, and the issue says so.
Two small choices worth stating
The unary converter's message now names the node and points at
CompilationProtocol, which is thedocumented way to add a case:
"There is no compiled form for Floorf. Define a CompilationProtocol which overrides ConvertUnaryNode to add one."The Linq wrap is at the boundary in
IntoLinqCompiler.Compilerather than around each converter, andit is narrow —
InvalidOperationException,ArgumentException,NotSupportedException, andexplicitly not anything already deriving from
AngouriMathBaseException, so a library exceptionraised deeper down still reaches the caller as itself.
Measured on this branch, .NET 10
crashcheck,compileoperationcrashcheck, everything14 new regression cases in
UserInvalidExceptions, one per expression in the two families above.How it was found
work/crashcheck, added for P2 of this batch of work: every case runs in a child process of itsown, because a stack overflow cannot be caught — it kills the process, and a test run that dies
reports nothing about what it had not reached.
propcheckon stock master died that way inIntegrateByPartsPolynomial, so the suite was green and the library was not.The expressions come from the
Entitynode types by reflection, so a node added later is coveredwithout anyone remembering the file — which is what would have caught
floorandceilwhen theylanded. The harness also proves itself before reporting:
--selftestspawns a child whose only job isto overflow its own stack, and fails loudly if the parent does not report a crash, because "0 crashes"
from a detector that cannot see one is worth nothing.