Adds nullable sequence and alternative support

[arnoldlankamp]

Changes the semantics related to sequences and alternatives. These now both support the case where they are empty.

This should resolve issue #2213 .

[codecov]

Codecov Report

Attention: Patch coverage is 73.91304% with 6 lines in your changes missing coverage. Please review.

Project coverage is 47%. Comparing base (5f3933f) to head (fb064f1).

Files with missing lines	Patch %	Lines
...scalmpl/parser/gtd/stack/AlternativeStackNode.java	70%	1 Missing and 2 partials ⚠️
.../rascalmpl/parser/gtd/stack/SequenceStackNode.java	75%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##              main   #2267   +/-   ##
=======================================
  Coverage       47%     47%           
- Complexity    6349    6367   +18     
=======================================
  Files          762     762           
  Lines        63031   63049   +18     
  Branches      9400    9404    +4     
=======================================
+ Hits         29926   29954   +28     
+ Misses       30892   30883    -9     
+ Partials      2213    2212    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[arnoldlankamp]

The Codecov report seems to be incorrect 🤷‍♂️ . The lines that it's complaining about are actually covered by a combination of the existing and newly added tests.

Only the UnsupportedOperationException exception case in the getEmptyChild methods is not covered. This is intentional, since these should never occur at runtime. They are merely there to signal implementation bugs and are not part of the application logic.

[DavyLandman]

Due to the low volume of changes in the parser, we don't run the parser test as part of the ci:

https://github.com/usethesource/rascal/blob/30ab3a6f94b0b6d31b2b4e10e5dc85ac2d94e8a1/pom.xml#L247C1-L255C32

[jurgenvinju]

Some assumption is not right here:

Due to the low volume of changes in the parser, we don't run the parser test as part of the ci:

but:
<include>**/org/rascalmpl/test/AllSuiteParallel.java</include>

and AllSuiteParallel includes "parser":

@RecursiveJavaOnlyTest({"basic", "concrete", "functionality", "library", "parser",  "recovery", "demo", "benchmark"})
public class AllSuiteParallel {
}

So what is really going on?

[jurgenvinju]

@arnoldlankamp thanks for the fixes

[DavyLandman]

Some assumption is not right here:

Due to the low volume of changes in the parser, we don't run the parser test as part of the ci:

but: <include>**/org/rascalmpl/test/AllSuiteParallel.java</include>

and AllSuiteParallel includes "parser":

@RecursiveJavaOnlyTest({"basic", "concrete", "functionality", "library", "parser",  "recovery", "demo", "benchmark"})
public class AllSuiteParallel {
}

So what is really going on?

Hmm, I was wrong, let me see.

The RecursiveTestSuite ends up picking:

[class org.rascalmpl.test.parallel.AllSuiteParallel, class org.rascalmpl.test.functionality.MemoizationTests, class org.rascalmpl.test.functionality.ParallelEvaluatorsTests, class org.rascalmpl.test.functionality.RecoveryTests, class org.rascalmpl.test.functionality.StrategyTests, class org.rascalmpl.test.parser.StackNodeTest, class org.rascalmpl.test.benchmark.AllBenchmarks]

as the test to run. why it ignores the others? It only selects classes with either:

• RascalJUnitTestPrefix or RecursiveRascalParallelTest annotation on the class.
• at least 1 method private method with an @Test annotation.

Most classes in that parser directory only implement IParserTest, and are not in itself junit tests, but the ParserTest class is the runner. But that is not found as it doesn't follow the patterns that the recursive test runner follows. So that should be added (for example, a case for extending TestCase).

#2269 fixes that.

[arnoldlankamp]

Thanks for looking into that @DavyLandman .

Also I just noticed I forgot to add the new tests to the ‘ParserTest’ class. It’s been a while, since I last looked at this code 😅. I’ll try to add it this evening.

[DavyLandman]

I've updated the branch to include the fix of the unit test. So please pull @arnoldlankamp before fixing your forgotten test call.

[arnoldlankamp]

I've updated the branch to include the fix of the unit test. So please pull @arnoldlankamp before fixing your forgotten test call.

No worries. I often rewrite my commit history on branches anyway, since I try to create clean delineated commits, so they can be separately applied and reverted like patches, if desired.

[arnoldlankamp]

I updated the parser test suite, so the newly added test cases are also executed.

All the parser tests together seem to take no more than a few milliseconds to run, so enabling them shouldn't have affected build time in any noticeable way.

[arnoldlankamp]

I also noticed there is a Sequence3 test case lingering around. According to Git history, @jurgenvinju added this one last month.

Unfortunately this test case fails, because the expected output does not to match the defined grammar. The expected result is: A, empty(), B, but the EpsilonStackNode is missing from the rule for S. Adding the missing epsilon to the grammar leads to a successful parse, but in a failure to flatten, since the UPTRNodeFactory does not have any functionality to produce naked epsilons. Nor does the RascalValueFactory seem to have a factory method to create one.

I'm not sure what the intend of the test case was, but if a specific fix is desired, it would be best to open a new issue for it and I'll have a look.

[jurgenvinju]

@arnoldlankamp I suspect I started adding that test case Sequence3 to help fix #2212 and then forgot about it.

[arnoldlankamp]

@arnoldlankamp I suspect I started adding that test case Sequence3 to help fix #2212 and then forgot about it.

Ok, other than maybe removing the Sequence3 test case and verifying my fix, this PR should be ready to merge.