Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Number parsing rejecting doubles in scientific notation #531

Closed
alexweav opened this issue Jul 9, 2021 · 3 comments
Closed

Number parsing rejecting doubles in scientific notation #531

alexweav opened this issue Jul 9, 2021 · 3 comments
Assignees
Labels

Comments

@alexweav
Copy link
Contributor

alexweav commented Jul 9, 2021

1. Description

Hello, I ran into an issue with parsing doubles using scientific notation. I believe I have tracked the issue down, and I am hoping to provide a fix.

Given a class defining a double field:

public class Test
{
    public double Value { get; set; }
}

The following Dynamic Linq expression results in a ParseException when passed to DynamicExpressionParser.ParseLambda:

Value < 1.2345E-4

where this documentation states that 1.2345E-4 is a valid double literal.

This appears to happen because ExpressionParser.TryParseAsDouble always passes NumberStyles.Number to double.Parse here. This flag specifically disallows exponents in doubles: https://docs.microsoft.com/en-us/dotnet/api/system.globalization.numberstyles?view=net-5.0

I noticed that TryParseAsFloat uses NumberStyles.Float instead, which does allow exponents. However, parsing as float always fails in my example above, because the expression doesn't add a qualifier which forces it to be recognized as a float.

2. Exception

Here is the exception that is thrown by ParseLambda:

System.Linq.Dynamic.Core.Exceptions.ParseException
  HResult=0x80131500
  Message=Invalid real literal '2e2'
  Source=System.Linq.Dynamic.Core
  StackTrace:
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.TryParseAsDouble(String text, Char qualifier)
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.TryParseAsDecimal(String text, Char qualifier)
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.TryParseAsFloat(String text, Char qualifier)
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseRealLiteral()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParsePrimaryStart()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParsePrimary()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseUnary()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseMultiplicative()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseAdditive()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseShiftOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseComparisonOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseLogicalAndOrOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseIn()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseAndOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseOrOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseLambdaOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseNullCoalescingOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseConditionalOperator()
   at System.Linq.Dynamic.Core.Parser.ExpressionParser.Parse(Type resultType, Boolean createParameterCtor)
   at System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda(Type delegateType, ParsingConfig parsingConfig, Boolean createParameterCtor, ParameterExpression[] parameters, Type resultType, String expression, Object[] values)
   at System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda(ParsingConfig parsingConfig, Boolean createParameterCtor, ParameterExpression[] parameters, Type resultType, String expression, Object[] values)
   at System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda[T,TResult](ParsingConfig parsingConfig, Boolean createParameterCtor, String expression, Object[] values)
   at ReproduceDynamicLinq.Program.Main() in C:\Users\alweaver\source\repos\ReproduceDynamicLinq\ReproduceDynamicLinq\Program.cs:line 12

3. Fiddle or Project

Fiddle: https://dotnetfiddle.net/trfQjl

Note: There are issues when running this fiddle in-browser, due to incompatibilities between the security attributes on ParseException and the fact that dotnet fiddle executes code as a Medium Trust application. It ends up throwing a runtime error instead. Running the same code locally reproduces the expected exception.

4. Any further technical details

I am happy to provide a PR to improve the numeric parsing, given a bit of guidance:

  1. Should we always hardcode the NumberStyles enum, or should I go ahead and add the enum to ParsingConfig so that users may adjust it according to their needs? NumberParseCulture is already configurable in this way, so it seems like the supported number style might also be a good candidate to be a configuration option. I'm happy to make this configurable if we want.
  2. Should the default NumberStyles value be NumberStyles.Number | NumberStyles.Exponent, or NumberStyles.Float? Technically, NumberStyles.Float disables AllowTrailingSign, which Number currently enables. I presume the first is preferable, to avoid a regression.
  3. Another approach might be to keep the default of NumberStyles.Number and only make the flag configurable through ParsingConfig. This would allow me to set this flag in my own ParsingConfig and use exponents freely, but it also avoids changing existing behavior of the parser. The drawback here is that the default parse behavior still doesn't match the documentation.

Thanks all :)

@StefH
Copy link
Collaborator

StefH commented Jul 10, 2021

@alexweav Thank you for your analysis.

I'm wondering, what if I change the code for doubles and floats to use NumberStyles.Any instead of NumberStyles.Number ?
Would that solve your issue? And/Or would that break current functionality?

@StefH
Copy link
Collaborator

StefH commented Jul 11, 2021

@alexweav
I've made a PR which should fix this.
#532

Can you please review and check if all the required unit-tests cover your scenario?

@StefH StefH self-assigned this Jul 11, 2021
@StefH StefH added the bug label Jul 11, 2021
@alexweav
Copy link
Contributor Author

@StefH This is fantastic! I really appreciate this change. I reviewed the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants