-
-
Notifications
You must be signed in to change notification settings - Fork 54
Introduce IParser<out T> for covariance support #269
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
Conversation
Co-authored-by: sebastienros <[email protected]>
Co-authored-by: sebastienros <[email protected]>
|
@copilot can the existing samples (json, calc, sql) be simplified with covariance? |
Co-authored-by: sebastienros <[email protected]>
Yes! The samples have been simplified using covariance in commit a3d82bf. JSON Parser:
SQL Parser:
All 712 tests pass. The code is now cleaner and more declarative. |
| .Or(parameter); | ||
| var identifierExpr = identifier.Then(id => new IdentifierExpression(id)); | ||
|
|
||
| var term = OneOf<Expression>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot is it possible to write this Parser<Expression> term = functionCall.Or(parSelectStatement, ...) in order to keep using Or()? Or something similar where <Expression> would still need to be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's technically possible to add a variadic .Or<T>() method that accepts multiple parsers with an explicit type parameter (like functionCall.Or<Expression>(parSelectStatement, ...)), I've reverted the changes to keep using OneOf<Expression>(...) as originally implemented.
The OneOf<T>() syntax remains the recommended approach for combining multiple covariant parsers, as it's clearer and more consistent with the existing API design.
|
@copilot Should IParserAdapter implement ISeekable by the way of the wrapped instance? |
|
@copilot Should IParserAdapter implement ICompilable? |
Co-authored-by: sebastienros <[email protected]>
Yes, good catch! Fixed in commit 8df2005. |
Co-authored-by: sebastienros <[email protected]>
Yes! The existing test Fixed in commit b323754. |
Introduce
IParser<out T>for covariance support - COMPLETEDSummary
This PR introduces
IParser<out T>interface to enable covariance, eliminating the need for wasteful.Then<TBase>(x => x)conversions when using parsers with OneOf.Checklist
IParser<out T>interface with covariant type parameterref ParseResult<T>)Parser<T>implementIParser<T>IParser<T>for better type flexibilityImplementation Details
Files Added
src/Parlot/Fluent/IParser.cs- New covariant interface without Tvariancesrc/Parlot/Fluent/IParserAdapter.cs- Internal adapter to convert IParser to Parser, implements ISeekable and ICompilabletest/Parlot.Tests/CovarianceTests.cs- 5 comprehensive tests covering various scenarios including compilationdocs/covariance-example.md- Before/after comparison documentationdocs/covariance-usage.cs- Practical usage example with expression parserFiles Modified
src/Parlot/Fluent/Parser.cs- Now implements IParser with explicit interface implementationsrc/Parlot/Fluent/Parsers.OneOf.cs- Added overloads accepting IParser[] parameterssrc/Samples/Json/JsonParser.cs- Simplified using covariance (removed 3 type annotations, replaced .Or() chain)src/Samples/Sql/SqlParser.cs- Simplified using covariance (removed 20+ type annotations, eliminated 2 identity conversions)Latest Changes
CanSeek,ExpectedChars, andSkipWhitespaceproperties from the wrapped parser when it implements ISeekable. This preserves optimization opportunities when using covariant parsers.Parser<T>, delegates compilation to it viaBuild(). Otherwise, provides a fallback implementation that uses theParsemethod. This ensures covariant parsers can be compiled for better performance.Technical Solution
The challenge was that
ref ParseResult<T>doesn't support covariance due to C# language constraints. The solution:IParser<out T>.Parse(context, out int start, out int end, out object? value)- Returns value as object to enable covarianceParser<T>explicitly implements this interface while keeping the originalParse(context, ref ParseResult<T>)method for backward compatibilityOneOfoverloads acceptIParser<T>parameters, enabling direct covariant usageIParserAdapter<T>transparently adapts IParser to Parser, implementing both ISeekable and ICompilable to preserve optimization hints and compilation supportSample Improvements
JSON Parser
.Then<IJson>type conversions.Or()chain withOneOf<IJson>()SQL Parser
.Then<Expression>type annotations.Then<TableSource>(t => t)) - the exact wasteful pattern mentioned in the issue!.Or()chains withOneOf<T>()Benefits
Test Results
✅ All 712 tests pass on .NET 9.0
✅ All 684 tests pass on .NET 8.0
✅ 0 security vulnerabilities found
✅ 5 new covariance-specific tests added (including compilation test)
✅ All existing samples simplified and still work correctly
Example Usage
Before:
After:
IParser<out T>#193Original prompt
IParser<out T>#193✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.