search

sklose / NCalc2

expression evaluator for .NET with built-in compiler
MIT License
166 stars 58 forks source link

NullReferenceException when expression contains null parameter #94

Closed Bykiev closed 5 months ago

Bykiev commented 5 months ago

I found a bug, which should be discussed: when expression contains null parameter and null parameters are not allowed a NullReferenceException is thrown. We don't need to get the most precise type if any of parameters is null

https://github.com/sklose/NCalc2/blob/4e0338cbf84236087b793430aa91287270a1df05/src/NCalc/Domain/EvaluationVisitor.cs#L68-L81

If the null parameters are not allowed we should throw ArgumentNullException instead, but it'll lead to breaking change. I think the best choice will be check first if any of parameters is null and don't throw any exceptions like EvaluateOptions.AllowNullParameter is specified. What do you think?

Upd: since if one or both null parameters in expression will cause NullReferenceException it will not be a breaking change and it's safe to throw ArgumentNullException, but a minor version increase should be done

Bykiev commented 5 months ago

I found the test ExpressionThrowsNullReferenceExceptionWithoutNullOption, which checks for NRE, so, it's not a bug. Should we throw ArgumentNullException then? It'll be a breaking change.

david-brink-talogy commented 5 months ago

The change you're suggesting sounds reasonable. I don't even think it needs a minor version change.

Bykiev commented 5 months ago

Thanks, I've reopened the PR and fixed the test case to handle ArgumentNullException. Please, review it