Fix #6: Add eager/non-throwing validation for expression syntax errors

[willibrandon]

Fixes #6

Implements eager validation for expression syntax errors, making them non-throwing in the TryCompile path.

Changes

• Added ExpressionValidationException to handle validation errors internally
• Invalid regular expressions in IsMatch()/IndexOfMatch() are detected during compilation
• Unknown function names are reported gracefully instead of throwing
• CI modifier usage logs warnings for unsupported functions (maintains backward compatibility)
• Single validation error per exception

Testing

• Added test suite in ExpressionValidationTests.cs
• All existing tests pass
• Backward compatibility maintained - Compile() still throws for invalid expressions

[nblumhardt]

Howdy! Thanks for checking this out.

Because NameResolver makes built-in functions pluggable, the set of ci capable operators isn't fixed, so I don't think listing them in the validator will work.

I ran through the current expression compiler implementation; since compilation failures really are on the error path, I'm coming around to thinking we should just lean on a custom ExpressionValidationException type, catch this where you have catch (ArgumentException ...) in TryCompileImpl(), and then work through the rest of the codebase to either throw this exception type where appropriate, or wrap exceptions from components we don't control.

From the outside, i.e. to the tests, I think the behavior would be almost identical.

I don't really like exceptions at the best of times :-) but skimming through the LinqExpressionCompiler type and a few others, the idea of maintaining a separate validator in parallel seems a bit daunting, so perhaps a blessed validation exception type is the lesser of the two evils?

[willibrandon]

Thanks for the feedback! I understand the concerns about the pluggable NameResolver and parallel validation.

A couple questions before I rework this:

1. For CI modifier validation - I can check at runtime if a resolved function supports StringComparison by inspecting the MethodInfo parameters, similar to how LinqExpressionCompiler already does. Should I implement this dynamic check, or skip CI validation entirely?

2. Should I convert all existing ArgumentExceptions in the compiler to ExpressionValidationException, or just focus on the three cases from issue Eager/non-throwing checks for more expression syntax errors #6 (regex, unknown functions, invalid CI)?

3. Do you want the ExpressionValidationException to support multiple errors like the current approach, or one error per exception?

Happy to rework this with the exception-based approach!

[nblumhardt]

Thanks for taking another look.

1. The NameResolver lookups only happen at compile time, so the cost is fairly trivial; by runtime (executing the expression) the whole thing's compiled via the LINQ expression tree and there won't be any further lookups. I think adding the check during name resolution seems worthwhile 👍

2. Perhaps to keep things moving forward, we stick to the cases in Eager/non-throwing checks for more expression syntax errors #6, and leave the others for further work? Could be a lot of back-and-forth otherwise.

3. I think since we can only get a single error out of TryCompile() (and out of ExpressionTemplate.TryParse()), carrying a single error on the exception type might be reasonable. The exception-driven approach makes collecting multiple errors more complicated and might not be worthwhile at this point. Since the exception type can remain internal (assuming we catch it internally in the APIs where it can be thrown) it would be possible to modify its interface to add multiple error support in the future, anyway.

[willibrandon]

Thanks @nblumhardt! I've updated the PR with the exception-based approach:

• ExpressionValidationException replaces the pre-validation approach
• Invalid regex and unknown functions throw validation exceptions caught by TryCompile()
• CI modifier validation uses dynamic StringComparison parameter detection
• Non-supporting functions log warnings via SelfLog for backward compatibility
• Single error per exception
• Focused on the three cases from Eager/non-throwing checks for more expression syntax errors #6

All existing tests pass.

Let me know if you need any adjustments.

[nblumhardt]

Awesome, thanks! Just looking for a chance to review, should be able to take a look shortly 😅

[nblumhardt]

Awesome, LGTM! 👍

[nblumhardt]

Thanks @willibrandon!