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

.Compile() causes "The parser needs to implement ISkippableSequenceParser" exception. #108

Closed
gumbarros opened this issue May 18, 2024 · 7 comments

Comments

@gumbarros
Copy link
Contributor

Good night!

I changed the following parser from this:

        var decimalPointParser = ZeroOrOne(Terms.Integer(NumberOptions.AllowSign))
            .AndSkip(Terms.Char('.'))
            .And(ZeroOrOne(Terms.Integer()))
            .AndSkip(ZeroOrOne(Terms.Text("e", true)))
            .And(ZeroOrOne(Terms.Integer(NumberOptions.AllowSign)))
            .Then<LogicalExpression>((context, x) =>
            {
                var useDecimalAsDefault = ((LogicalExpressionParserContext)context).UseDecimalsAsDefault;

                if (useDecimalAsDefault)
                {
                    decimal decimalValue;
                    if (x.Item3 != 0)
                        decimalValue = Convert.ToDecimal(x.Item1 + "." + x.Item2 + "e" + x.Item3,
                            CultureInfo.InvariantCulture);
                    else
                        decimalValue = Convert.ToDecimal(x.Item1 + "." + x.Item2, CultureInfo.InvariantCulture);

                    return new ValueExpression(decimalValue);
                }

                double doubleValue;
                if (x.Item3 != 0)
                    doubleValue = Convert.ToDouble(x.Item1 + "." + x.Item2 + "e" + x.Item3,
                        CultureInfo.InvariantCulture);
                else
                    doubleValue = Convert.ToDouble(x.Item1 + "." + x.Item2, CultureInfo.InvariantCulture);

                return new ValueExpression(doubleValue);
            });

To this:

        var mandatoryIntWithOptionalDecimal  = 
            Terms.Integer(NumberOptions.AllowSign)
            .AndSkip(Terms.Char('.'))
            .And(ZeroOrOne(Terms.Integer()));
        
        var optionalIntWithMandatoryDecimal = 
            ZeroOrOne(Terms.Integer(NumberOptions.AllowSign))
                .AndSkip(Terms.Char('.'))
                .And(Terms.Integer());

        var decimalPointParser = 
            OneOf(mandatoryIntWithOptionalDecimal, optionalIntWithMandatoryDecimal)
            .AndSkip(ZeroOrOne(Terms.Text("e", true)))
            .And(ZeroOrOne(Terms.Integer(NumberOptions.AllowSign)))
            .Then<LogicalExpression>((context, x) =>
            {
                var useDecimalAsDefault = ((LogicalExpressionParserContext)context).UseDecimalsAsDefault;

                if (useDecimalAsDefault)
                {
                    decimal decimalValue;
                    if (x.Item3 != 0)
                        decimalValue = Convert.ToDecimal(x.Item1 + "." + x.Item2 + "e" + x.Item3,
                            CultureInfo.InvariantCulture);
                    else
                        decimalValue = Convert.ToDecimal(x.Item1 + "." + x.Item2, CultureInfo.InvariantCulture);

                    return new ValueExpression(decimalValue);
                }

                double doubleValue;
                if (x.Item3 != 0)
                    doubleValue = Convert.ToDouble(x.Item1 + "." + x.Item2 + "e" + x.Item3,
                        CultureInfo.InvariantCulture);
                else
                    doubleValue = Convert.ToDouble(x.Item1 + "." + x.Item2, CultureInfo.InvariantCulture);

                return new ValueExpression(doubleValue);
            });

And this caused the following exception:
The parser needs to implement ISkippableSequenceParser exception.

But when I don't compile the parser with .Compile() everything works. Is this expected behavior?

@gumbarros gumbarros changed the title .Compile() causes The parser needs to implement ISkippableSequenceParser exception. .Compile() causes "The parser needs to implement ISkippableSequenceParser" exception. May 18, 2024
@sebastienros
Copy link
Owner

when I don't compile the parser with .Compile() everything works

Then it's a bug in the compilation code.

I did some changes recently for some compiled parsers, could you try to reference the source code and use that instead of the nuget package to see if it still reproes?

If you can remove as much as possible and it still fails that would help isolate the problem. I will also try that on my side. I believe you are not blocked as long as you don't try to use the compiled version, it's only slightly faster with compilation.

@sebastienros
Copy link
Owner

It's related to a sequence, a list of And() calls. Should be easy to isolate on my side.

@gumbarros
Copy link
Contributor Author

gumbarros commented May 18, 2024

Referencing the source causes a breaking change, probably related to OptionalResult<T>

I'm thinking in how to solve, but probably if I fix it it will work.

image

Edit: I will probably think in something tomorrow, stuck, because one parser is Parser<long> and the other Parser<OptionalResult<long>>

@sebastienros
Copy link
Owner

Yes, it's a breaking change but it makes ZeroOrOne easier to use.

@gumbarros
Copy link
Contributor Author

gumbarros commented May 18, 2024

Good morning,

Solved the compilation error with

        var mandatoryIntWithOptionalDecimal =
            Terms.Integer(NumberOptions.AllowSign)
                .AndSkip(Terms.Char('.'))
                .And(ZeroOrOne(Terms.Integer()))
                .Then(l=>new ValueTuple<OptionalResult<long>, OptionalResult<long>>(new OptionalResult<long>(true, l.Item1), l.Item2));

        
        var optionalIntWithMandatoryDecimal = 
            ZeroOrOne(Terms.Integer(NumberOptions.AllowSign))
                .AndSkip(Terms.Char('.'))
                .And(Terms.Integer())
                .Then(l=>new ValueTuple<OptionalResult<long>, OptionalResult<long>>(l.Item1, new OptionalResult<long>(true, l.Item2)));
        
        var decimalPointParser = 
            OneOf(mandatoryIntWithOptionalDecimal, optionalIntWithMandatoryDecimal)
            .AndSkip(ZeroOrOne(Terms.Text("e", true)))
            .And(ZeroOrOne(Terms.Integer(NumberOptions.AllowSign)))
            .Then<LogicalExpression>((context, x) =>
            {
                var useDecimalAsDefault = ((LogicalExpressionParserContext)context).UseDecimalsAsDefault;

                if (useDecimalAsDefault)
                {
                    decimal decimalValue;
                    if (x.Item3.HasValue)
                        decimalValue = Convert.ToDecimal(x.Item1 + "." + x.Item2 + "e" + x.Item3,
                            CultureInfo.InvariantCulture);
                    else
                        decimalValue = Convert.ToDecimal(x.Item1 + "." + x.Item2, CultureInfo.InvariantCulture);

                    return new ValueExpression(decimalValue);
                }

                double doubleValue;
                if (x.Item3.HasValue)
                    doubleValue = Convert.ToDouble(x.Item1 + "." + x.Item2 + "e" + x.Item3,
                        CultureInfo.InvariantCulture);
                else
                    doubleValue = Convert.ToDouble(x.Item1 + "." + x.Item2, CultureInfo.InvariantCulture);

                return new ValueExpression(doubleValue);
            });

But after calling .Compile() the exception still happens

@sebastienros
Copy link
Owner

sebastienros commented May 18, 2024

Currently working on it, I have a minimal repro, but I notice you are using Terms to parse the decimal where Literals would be right actually. Spaces can be found around terms, not literals. So when you look for Integer then '.' then Integer you don't want spaces between each.

Bote that between Integer '+' Integer there you want to use terms. But since you are building a parser for an element in this case you use Literal and not Term.

@sebastienros
Copy link
Owner

Didn't create a branch and PR by mistake, but it's fixed, understood the problem and added some tests.
Will ship a new release once I have looked at your other issues.

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

No branches or pull requests

2 participants