Fix code generation for a list of lists inside a conditional or switch - #1340
Open
SimonCockx wants to merge 4 commits into
Open
Fix code generation for a list of lists inside a conditional or switch#1340SimonCockx wants to merge 4 commits into
SimonCockx wants to merge 4 commits into
Conversation
`CardinalityProvider#isOutputListOfLists` did not look inside the branches
of a conditional or switch expression. As a result, a branch such as
if test
then foos extract item -> xs // a list of lists
passed validation, after which the generator emitted a `MapperListOfLists`
for that branch and a `List` for the other one. Since `TypeCoercionService`
has no conversion out of `MapperListOfLists`, it silently left the branch
as is and both branches were joined into `java.lang.Object`, producing Java
code that does not compile.
A conditional or switch is now considered a list of lists if any of its
branches is, which means such models are reported with the existing
"use flatten to create a list" error instead. On top of that:
- All branches of a conditional or switch must agree on whether they are a
list of lists (empty branches excepted), since there is no representation
for a mix of the two.
- A list of lists is now also rejected as an operand of a binary operation
and as an argument of a function or rule call. Both cases used to generate
Java code that does not compile as well.
- `then` and the empty representation of `TypeCoercionService` now support
`MapperListOfLists`, so that a conditional or switch that consistently
produces a list of lists can be flattened afterwards.
✅ Deploy Preview for finos-rune canceled.
|
A list of lists as an element of a list literal or as the value of a constructor attribute generated Java code that does not compile as well, without any validation error. Extract the check into a shared `isNotListOfListsCheck` and apply it consistently to operands of a binary operation, arguments of a function or rule call, elements of a list literal and values of a constructor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A list of lists that appears in a branch of a conditional or switch expression is not detected as such, which results in generated Java code that does not compile. Reported on a CDM extension model, where
generated
.addDates(ifThenElseResult)withifThenElseResulttyped asjava.lang.Object:Cause
CardinalityProvider#isOutputListOfListsonly walked throughextract,filter,thenandflatten, never into the branches of a conditional or switch. SinceGenerateWeeklyResetScheduleoutputs a list andcalculationPeriodsDatais a list, thatextractproduces a list of lists, but it stayed invisible tocheckFunctionOperation, so the model validated without issues.Code generation then emitted a
MapperListOfListsfor that branch and aListfor the other one.TypeCoercionServicehas no conversion out ofMapperListOfListsand silently leaves the expression unchanged when it cannot convert, soJavaIfThenElseBuilderjoined the two branch types intojava.lang.Object.Minimal reproduction:
Changes
CardinalityProvider: a conditional or switch is a list of lists if any of its branches is. Models like the one above are now reported with the existingAssign expression contains a list of lists, use flatten to create a listerror.ExpressionValidator: all branches of a conditional or switch must agree on whether they are a list of lists (empty branches excepted) - there is no representation for a mix of the two.ExpressionValidator: a list of lists is now also rejected as an operand of a binary operation (e.g.default) and as an argument of a function or rule call. Both cases used to generate Java code that does not compile as well, without any validation error.ExpressionGeneratorandTypeCoercionService:thenand the empty representation of aMapperListOfListsare supported, so a conditional or switch that consistently produces a list of lists can be flattened afterwards, e.g.(if test then foos extract item -> xs) flatten.The workaround for existing models is to flatten inside the branch:
then (calculationPeriodsData extract GenerateWeeklyResetSchedule(...) then flatten).Type of change
Note that models which relied on the unreported cases above will now get a validation error, but they could not have compiled before.
Tests
ListOfListsTest: new, covers generation and evaluation of a list of lists inside a conditional and a switch, followed byflattenorthen flatten.ExpressionValidatorTest: six new cases for the validations above.