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

Incorrect Handling of Qualifiers in ConstantExpressionHelper #823

Closed

Conversation

RenanCarlosPereira
Copy link
Contributor

Description:
There is an issue in the ExpressionPromoter class where the qualifier in the numeric literal expressions is not correctly handled, leading to incorrect type promotions. This is due to the way qualifiers are stored and retrieved from the ConstantExpressionHelper.

Steps to Reproduce:

  1. Create an instance of ParsingConfig.
  2. Use NumberParser to parse a numeric literal with a qualifier.
  3. Attempt to promote the parsed expression to a different type using ExpressionPromoter.

Example 1:

[Fact]
public void promoter()
{
    // Assign
    var parsingConfig = new ParsingConfig();
    var numberParser = new NumberParser(parsingConfig);
    var promoter = new ExpressionPromoter(parsingConfig);

    var doubleExpression = numberParser.ParseRealLiteral("10.5d", 'd', true);

    // Act & Assert
    var promotedExpression = promoter.Promote(doubleExpression, typeof(decimal), true, true);

    Assert.NotNull(promotedExpression);
    Assert.Equal(typeof(decimal), promotedExpression.Type);
}

Example 2:

[Fact]
public void promoteDoubleToDecimal()
{
    // Assign
    var parsingConfig = new ParsingConfig();
    var numberParser = new NumberParser(parsingConfig);
    var promoter = new ExpressionPromoter(parsingConfig);

    var doubleExpression = numberParser.ParseRealLiteral("123.456d", 'd', true);

    // Act & Assert
    var promotedExpression = promoter.Promote(doubleExpression, typeof(decimal), true, true);

    Assert.NotNull(promotedExpression);
    Assert.Equal(typeof(decimal), promotedExpression.Type);
}

Expected Behavior:
The Promote method should correctly handle the qualifier and convert the numeric literal to the specified target type.

Actual Behavior:
The promotion fails, and the conversion does not occur as expected due to the incorrect handling of the qualifier in the ConstantExpressionHelper.

Additional Information:
This issue affects applications that rely on dynamic LINQ queries where numeric-type promotions with qualifiers are necessary.

Proposed Solution:
Ensure that the CreateLiteral in ConstantExpressionHelper correctly handles qualifiers without the prefix when creating and retrieving constant expressions.

@RenanCarlosPereira
Copy link
Contributor Author

this PR is related to this issue:
#821

@@ -152,5 +152,31 @@ public void NumberParser_ParseIntegerLiteral(string text, double expected)
// Assert
result.Value.Should().Be(expected);
}

[Theory]
Copy link
Collaborator

@StefH StefH Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to split into multiple tests to make sure that the value is really the correct type:

        [Theory]
        [InlineData("42", 'm', 42m)]
        [InlineData("-42", 'm', -42m)]
        [InlineData("42m", 'm', 42m)]
        [InlineData("-42m", 'm', -42m)]
        public void NumberParser_ParseDecimalLiteral(string text, char qualifier, decimal expected)
        {
            // Act
            var result = (ConstantExpression) new NumberParser(_parsingConfig).ParseRealLiteral(text, qualifier, true);

            // Assert
            result.Value.Should().Be(expected);
        }
        [Theory]
        [InlineData("42", 'd', 42d)]
        [InlineData("-42", 'd', -42d)]
        [InlineData("42m", 'd', 42d)]
        [InlineData("-42m", 'd', -42d)]
        public void NumberParser_ParseDoubleLiteral(string text, char qualifier, double expected)
        {
            // Act
            var result = (ConstantExpression) new NumberParser(_parsingConfig).ParseRealLiteral(text, qualifier, true);

            // Assert
            result.Value.Should().Be(expected);
        }
        [Theory]
        [InlineData("42", 'f', 42f)]
        [InlineData("-42", 'f', -42f)]
        [InlineData("42m", 'f', 42f)]
        [InlineData("-42m", 'f', -42f)]
        public void NumberParser_ParseFloatLiteral(string text, char qualifier, float expected)
        {
            // Act
            var result = (ConstantExpression) new NumberParser(_parsingConfig).ParseRealLiteral(text, qualifier, true);

            // Assert
            result.Value.Should().Be(expected);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @StefH if I do that we will have the sonar complain:
https://rules.sonarsource.com/csharp/RSPEC-4144/

Please let me know if you'd like me to do it anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please implement it

@StefH
Copy link
Collaborator

StefH commented Jun 25, 2024

@RenanCarlosPereira
BTW : can you make a PR from a new branch in your fork? (instead of master)

@RenanCarlosPereira
Copy link
Contributor Author

RenanCarlosPereira commented Jun 25, 2024

yeah, I forgot to open a branch to it, doing it now 😅😅

I splited the tests, all passing

@RenanCarlosPereira
Copy link
Contributor Author

@RenanCarlosPereira BTW : can you make a PR from a new branch in your fork? (instead of master)

That is the new PR, I will abandon this one:
#824

@StefH
Copy link
Collaborator

StefH commented Jun 25, 2024

@RenanCarlosPereira BTW : can you make a PR from a new branch in your fork? (instead of master)

That is the new PR, I will abandon this one: #824

OK, You can indeed abandon this

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

Successfully merging this pull request may close these issues.

None yet

2 participants