New recurrence evaluator #901

maknapp · 2025-12-31T16:12:22Z

This evaluator follows the same basic logic as the current one, but with a few key differences:

Moves the expand/limit grid into the flow control of the evaluator. No need for IsCandidateSetFullyExpanded.
BY rule normalization is moved to a separate class and cached when possible. Caching may not be very useful in real world usage though.
BYDAY values without offsets are generated. BYDAY with offsets still need to calculate entire range then sort, which is likely a small number of values anyway.
Enumeration is only restarted when BYSETPOS is set.
Reduced usage of Linq - reduces allocation.
BYSETPOS with only positive values stops evaluating the set early. Added benchmark to show this.
RecurrencePatternEvaluator2 is used inside RecurrencePatternEvaluator just to easily comment out and compare between the two evaluators. It is intended to replace.
Some new tests fail with current evaluator.
Starts evaluation at periodStart by skipping directly to it instead of looping. There is a new test in RecurrenceTestCases.txt commented out because it takes forever on the current evaluator.
BugByWeekNoNotWorking() test was changed to YEARLY because the WEEKLY+BYWEEKNO is invalid.
The seed value is incremented nearest to reference date always, instead of the start of the week/month/year that IncrementDate() does. This makes the seed value require minimal changes and no extra BY rules.
ProcessRecurrencePattern() is NOT applied. No extra BY rules are added to evaluation.

Before (`be1d758`):

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
GetOccurrences	1,352.93 us	9.169 us	8.577 us	224.6094	42.9688	1837.3 KB
MultipleEventsWithUntilOccurrencesSearchingByWholeCalendar	15.63 us	0.175 us	0.163 us	3.9673	0.1526	32.63 KB
MultipleEventsWithUntilOccurrences	74.32 us	0.589 us	0.551 us	13.5498	0.3662	111.19 KB
MultipleEventsWithUntilOccurrencesEventsAsParallel	45.78 us	0.641 us	0.535 us	16.1133	1.2207	130.64 KB
EventWithBySetPosRecurrence	1,149.22 us	9.324 us	8.722 us	175.7813	11.7188	1448.73 KB
EventWithPositiveBySetPosRecurrence	1,118.22 us	4.922 us	4.604 us	146.4844	7.8125	1209.14 KB
MultipleEventsWithCountOccurrencesSearchingByWholeCalendar	32.70 us	0.134 us	0.119 us	7.9956	0.7324	65.8 KB
MultipleEventsWithCountOccurrences	517.66 us	3.159 us	2.638 us	97.6563	15.6250	804.97 KB
MultipleEventsWithCountOccurrencesEventsAsParallel	257.24 us	4.402 us	3.902 us	102.5391	21.4844	831.75 KB

After:

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
GetOccurrences	927.16 us	17.564 us	18.793 us	113.2813	22.4609	928.75 KB
MultipleEventsWithUntilOccurrencesSearchingByWholeCalendar	11.03 us	0.039 us	0.034 us	2.8687	0.0763	23.53 KB
MultipleEventsWithUntilOccurrences	45.64 us	0.332 us	0.294 us	7.4463	0.1221	61.03 KB
MultipleEventsWithUntilOccurrencesEventsAsParallel	31.93 us	0.462 us	0.410 us	9.9487	0.8545	80.48 KB
EventWithBySetPosRecurrence	327.28 us	1.035 us	0.968 us	65.9180	3.4180	540.45 KB
EventWithPositiveBySetPosRecurrence	108.77 us	2.161 us	2.312 us	13.5498	0.4883	111.47 KB
MultipleEventsWithCountOccurrencesSearchingByWholeCalendar	27.50 us	0.201 us	0.178 us	7.0190	0.6714	57.36 KB
MultipleEventsWithCountOccurrences	343.27 us	1.078 us	0.955 us	52.7344	8.3008	433.59 KB
MultipleEventsWithCountOccurrencesEventsAsParallel	186.75 us	3.360 us	2.979 us	56.6406	11.4746	460.37 KB

codecov · 2025-12-31T16:14:35Z

Codecov Report

❌ Patch coverage is 91.18644% with 52 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs	93.6%	4 Missing and 22 partials ⚠️
Ical.Net/Evaluation/TodoEvaluator.cs	16.7%	8 Missing and 2 partials ⚠️
Ical.Net/DataTypes/RecurrencePattern.cs	0.0%	0 Missing and 9 partials ⚠️
Ical.Net/Evaluation/EventEvaluator.cs	81.5%	2 Missing and 3 partials ⚠️
Ical.Net/Evaluation/RecurringEvaluator.cs	90.0%	0 Missing and 1 partial ⚠️
Ical.Net/Evaluation/TimeZoneInfoEvaluator.cs	0.0%	1 Missing ⚠️

@@              Coverage Diff              @@
##           version/6.0    #901     +/-   ##
=============================================
+ Coverage         68.6%   68.8%   +0.2%     
=============================================
  Files              116     118      +2     
  Lines             4465    4738    +273     
  Branches          1013    1113    +100     
=============================================
+ Hits              3062    3261    +199     
- Misses            1035    1090     +55     
- Partials           368     387     +19

Files with missing lines	Coverage Δ
Ical.Net/CollectionExtensions.cs	`66.7% <ø> (ø)`
Ical.Net/Evaluation/ByRuleValues.cs	`100.0% <100.0%> (ø)`
Ical.Net/Evaluation/WeekDayValue.cs	`100.0% <100.0%> (ø)`
Ical.Net/NodaTimeExtensions.cs	`100.0% <100.0%> (ø)`
Ical.Net/Evaluation/RecurringEvaluator.cs	`97.3% <90.0%> (+0.2%)`	⬆️
Ical.Net/Evaluation/TimeZoneInfoEvaluator.cs	`16.7% <0.0%> (-3.3%)`	⬇️
Ical.Net/Evaluation/EventEvaluator.cs	`78.9% <81.5%> (+1.4%)`	⬆️
Ical.Net/DataTypes/RecurrencePattern.cs	`76.5% <0.0%> (-10.2%)`	⬇️
Ical.Net/Evaluation/TodoEvaluator.cs	`34.2% <16.7%> (-3.5%)`	⬇️
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs	`90.8% <93.6%> (-2.1%)`	⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

axunonb · 2026-01-01T22:12:50Z

Thanks for this PR, @maknapp! This looks like a very promising step towards version 6.0 in terms of the recurrence evaluation.
As the PR is still marked as a draft, I have just a few high-level comments for now.

Currently RecurrencePatternEvaluator2 and RecurrencePatternEvaluator are part of the PR, but I understand RecurrencePatternEvaluator1 is considered as the replacement. For the review it would be helpful to use the final implementation.

Variable Shadowing: I noticed several places where local variables or parameters hide class-level fields with the same name. This is error-prone. Can we rename the local vars or prefix class members with _ to ensure we don't accidentally use the wrong one (like editorconfig suggests)?

System.Data.EvaluateException: Should this read as Ical.Net.Evaluation.EvaluationException?

Redundant Logic: There are some if...else if blocks that seem to re-check conditions or could be simplified. flattening these with guard clauses or using switch expressions would make the evaluation logic much easier to follow.

To make the review easier and ensure the long-term health of the repo, would you mind resolving the Blocker/High issues identified in the Sonar report? Regarding Coverage the 4.7% decrease in overall coverage may be caused by RecurrencePatternEvaluator2 and RecurrencePatternEvaluator being part of the code base.

Thanks for the massive effort on the new evaluator.

maknapp · 2026-01-02T23:09:18Z

Currently RecurrencePatternEvaluator2 and RecurrencePatternEvaluator are part of the PR...

Replaced.

Variable Shadowing: I noticed several places where local variables or parameters hide class-level fields with the same name. This is error-prone. Can we rename the local vars or prefix class members with _ to ensure we don't accidentally use the wrong one (like editorconfig suggests)?

Done.

System.Data.EvaluateException: Should this read as Ical.Net.Evaluation.EvaluationException?

Yes :D

Redundant Logic: There are some if...else if blocks that seem to re-check conditions or could be simplified. flattening these with guard clauses or using switch expressions would make the evaluation logic much easier to follow.

Redundant within the same method? I just split some of the more complex methods, which may have been the areas you were talking about.

resolving the Blocker/High issues identified in the Sonar report?

I fixed some of them. The "break out of recursion" issues do not make sense - they generators intended to run forever. The remaining ExpandYearDay() complexity issue would not benefit from changing in my opinion. The nested ifs are separated for clarity and I do not see a better way to do it.

For the minor Linq issues, some of those cannot use Linq without affecting allocations. The simple ones can use Linq, but it is easier to debug without it.

minichma

Wow, that's great work! Couldn't go through much of the code yet, so here are just some very first thoughts.

Some time ago I have also been working on some of the aspects you are addressing in this PR (e.g. caching). Unfortunately I didn't manage to complete it but just FYI, this is the branch, should you be interested. Just keep in mind that its work in progress:

https://github.com/minichma/ical.net/tree/work/minichma/feature/modernize_rrule2

minichma · 2026-01-02T22:23:14Z

Ical.Net/Evaluation/ByRuleValues.cs

+    private readonly int[] normalSeconds;
+
+    private readonly int[] byWeekNo;
+    private readonly NormalValues<int, int> weeks;


add comment: what is the cache key? In this case it is the number of weeks per year, right?

minichma · 2026-01-02T22:29:29Z

Ical.Net/Evaluation/ByRuleValues.cs

+    private readonly NormalValues<int, int> weeks;
+
+    private readonly int[] byYearDay;
+    private readonly NormalValues<int, int> yearDays;


In this case the year is used as cache key. Followed the pattern of the weeks property, we could also use the number of days in the year as cache key. This way the number of variants would be limited (i.e. 365 or 366) and we could cache all of them. We could simply maintain something like Dictionary<int, NormalValues<int>> weeksByYearDays where the number of days per year would be the key. The NormalValues type itself wouldn't need to hold the cache key (which would be nice as caching is quite a different concern than holding the values). This way the number of cache misses could be improved even further. Same applies to other NormalValues properties.

I changed it to start caching all values with Dictionary. Benchmarks are generally faster with similar allocation. Is this okay to do with BYSETPOS though? How many different set sizes can a single rrule produce?

Is this okay to do with BYSETPOS though?

Good question. I guess it should be ok for BYSETPOS as well. The date part of an occurrence can have no more than 366 values per interval (days in a yearly recurrence), so 366 is a hard upper bound for the number of different set sizes (the time part doesn't affect this number, because it won't vary across intervals). The real max number of different set sizes will certainly be significantly lower, not sure what it is, but even 366 won't cause any issues. So I guess we're safe in this respect.

axunonb · 2026-01-04T20:14:06Z

The "break out of recursion" issues do not make sense

For the code logic this is the case. The analyzer just insists on every code path should yield - similar to all code paths must return a value. A yield break makes the analyzer happy, and maybe it's also faster to understand. It has no other effect.

axunonb · 2026-01-04T20:24:02Z

For the minor Linq issues, some of those cannot use Linq without affecting allocations.

Yes, absolutely, and therefore we should avoid Linq here for now. (Microsoft says this will be improved with net10.0.)
The foreach loops have another effect: The exit point from the methods do not get hit with code coverage, because it only gets executed when the iterator finishes or is disposed explicitly. Long story short: We can live with that.

axunonb · 2026-01-04T20:40:37Z

Increase code coverage for `RecurrencePatternEvaluator`

WeekdayValue with zero Offset: Although sth. like BYDAY=0SU is invalid in RFC5545, the lenient behavior to ignore is maybe better to understand and also better than to throw.
These cases should also get code coverage, same as the additional EvaluationExceptions thrown. You may want to the include the attached tests cases in the PR. RecurrencePatternEvaluator code coverage should be as high as possible.
Additional_Unit_Tests.txt

axunonb · 2026-01-04T20:56:37Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

-        // Helper that checks whether the given candidate matches any BYMONTHDAY entry
-        // taking negative values into account (relative to the month's length).
-        static bool MatchesAnyMonthDay(ZonedDateTime candidate, IEnumerable<int> monthDays)
+        if (weekDays.Length == 0)


Can this ever be true at this stage? From my understanding the checks happen long before. So maybe remove it here?

This should never be true right now, but it is a helpful check. Future code changes can mistakenly make it reachable, and rest of the method does not support an empty array.

Then System.Diagnostics.Debug.Assert(weekDays.Length != 0, "Weekdays must not be empty."); ?

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

axunonb · 2026-01-04T21:03:25Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

-        // optimize the start time for selecting candidates
-        // (only applicable where a COUNT is not specified)
-        if (pattern.Count is null && periodStartDt is not null)
+        throw new EvaluationException("Invalid frequency type");


Add the invalid _frequency to the message?

Are all of "not supported" -> "invalid" okay now?

axunonb · 2026-01-04T21:05:46Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

-            }
+            return StartByRules();
+        }
+        else if (_rule.HasNegativeSetPos)


Remove (all) the redundant else then the if returns a value?
IMHO this would be easier to read.

axunonb · 2026-01-04T21:11:53Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

-            // Expand behavior
-            return dates.SelectMany(d => pattern.ByMonth
-                .Select(month => d.LocalDateTime.PlusMonths(month - d.Month).InZoneLeniently(d.Zone)));
+            throw new EvaluationException($"BYDAY offsets are not supported in {_frequency}");


Refer to invalidity in RFC5545 instead of "support".

axunonb · 2026-01-04T21:14:55Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

+        }
+        else if (_frequency == FrequencyType.Weekly)
+        {
+            throw new EvaluationException("BYMONTHDAY is not supported in WEEKLY");


Refer to invalidity in RFC5545 instead of "support"?

axunonb · 2026-01-04T21:15:45Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

+        }
+        else
+        {
+            throw new EvaluationException($"BYYEARDAY is not supported with {_frequency}");


Refer to invalidity in RFC5545 instead of "support"?

axunonb · 2026-01-04T21:16:28Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

-        {
-            date = date.Next(targetDayOfWeek);
-        }
+                // Ignore values outside of the calendar year


"of" is redundant?

I was confused by this and had to look it up. This appears to be a US vs British English difference. :D

axunonb · 2026-01-04T21:17:08Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

+        }
+        else
+        {
+            throw new EvaluationException($"BYWEEKNO is not supported with {_frequency}");


Refer to invalidity in RFC5545 instead of "support"?

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

axunonb · 2026-01-04T21:19:36Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

+                .At(_seed.TimeOfDay)
+                .InZoneLeniently(_seed.Zone),
+
+            _ => throw new EvaluationException("Invalid frequency")


Add the invalid _frequency value to the message?

axunonb · 2026-01-04T21:24:14Z

Ical.Net.Tests/RecurrenceTests.cs

        Assert.That(result, Is.EqualTo(expected));
    }
+
+    [Test]


Optional: Add the tests to RecurrenceTestCases.txt
(Avoids a lot of boilerplate code)

The tests in RecurrenceTestCases.txt are all UTC events evaluated to "US-Eastern" time zone. These tests require the CalendarEvent to be in a specific time zone, so they cannot be added to the txt file.

axunonb

This is great work and excellent code, thanks a lot! Good to have you working on ical.net.
See comments in files and conversation.

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

maknapp · 2026-01-05T02:26:46Z

A yield break makes the analyzer happy

There is no place to yield break though.

For the various EvaluationExceptions, would it make sense to validate rrules before evaluation? Evaluating a Calendar would throw if just one event was invalid. If it were validated on creation, it could be ignored before being added to the calendar? Can this be done without breaking serialize/deserialize backwards compatibility?

axunonb · 2026-01-05T08:48:08Z

validate rrules before evaluation

A topic for another isssue/pr?

axunonb · 2026-01-05T08:55:06Z

There is no place to yield break though.

Marked as accepted in SonarCloud

Ical.Net/DataTypes/RecurrencePattern.cs

axunonb · 2026-01-05T09:04:53Z

Ical.Net.Tests/Calendars/Recurrence/RecurrenceTestCases.txt

 INSTANCES:20260601,20260908,20261208

 ############################## END ERRATA 1913 TESTS ##############################
+


axunonb · 2026-01-05T17:03:42Z

Now that RecurrencePatternEvaluator does not inherit from Evaluator, question is whether we still need Evaluator and IEvaluator at all.
The abstract Evaluator and IncrementDate(ref ZonedDateTime, RecurrencePattern, int) are the only places that reference IEvaluator.
RecurringEvaluator currently inheriting from Evaluator could implement IEvaluator directly.
This change could go into this PR or a new one. What do you think?

sonarqubecloud · 2026-01-08T02:07:15Z

Quality Gate passed

Issues
9 New issues
1 Accepted issue

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

maknapp requested review from axunonb and minichma December 31, 2025 16:12

maknapp force-pushed the rrule-eval-next branch 2 times, most recently from 5c2e556 to a9392b9 Compare January 2, 2026 21:15

New recurrence evaluator

be27fd6

maknapp force-pushed the rrule-eval-next branch from a9392b9 to be27fd6 Compare January 2, 2026 22:55

minichma reviewed Jan 2, 2026

View reviewed changes

axunonb reviewed Jan 4, 2026

View reviewed changes

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs Outdated Show resolved Hide resolved

axunonb reviewed Jan 4, 2026

View reviewed changes

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs Outdated Show resolved Hide resolved

axunonb reviewed Jan 4, 2026

View reviewed changes

axunonb requested changes Jan 4, 2026

View reviewed changes

axunonb marked this pull request as ready for review January 4, 2026 21:41

axunonb reviewed Jan 4, 2026

View reviewed changes

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs Outdated Show resolved Hide resolved

maknapp added 2 commits January 4, 2026 21:06

Cache all BY rules

5575b10

Prefix fields with underscore

6fa3f15

Add more recurrence tests

b4abc88

axunonb reviewed Jan 5, 2026

View reviewed changes

Ical.Net/DataTypes/RecurrencePattern.cs Outdated Show resolved Hide resolved

axunonb reviewed Jan 5, 2026

View reviewed changes

axunonb added the Version 6 label Jan 5, 2026

maknapp added 2 commits January 6, 2026 20:43

Cleanup

378dca4

Fix bymonth range for expanding day

743e7e9

maknapp force-pushed the rrule-eval-next branch from d817ff7 to 743e7e9 Compare January 7, 2026 02:09

Fix start-at with overlapping event

c494756

		INSTANCES:20260601,20260908,20261208

		############################## END ERRATA 1913 TESTS ##############################

New recurrence evaluator #901

Are you sure you want to change the base?

New recurrence evaluator #901

Uh oh!

Conversation

maknapp commented Dec 31, 2025

Before (be1d758):

After:

Uh oh!

codecov bot commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

axunonb commented Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maknapp commented Jan 2, 2026

Uh oh!

minichma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb commented Jan 4, 2026

Uh oh!

axunonb commented Jan 4, 2026

Uh oh!

axunonb commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Increase code coverage for RecurrencePatternEvaluator

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maknapp commented Jan 5, 2026

Uh oh!

axunonb commented Jan 5, 2026

Before (`be1d758`):

codecov bot commented Dec 31, 2025 •

edited

Loading

axunonb commented Jan 1, 2026 •

edited

Loading

axunonb commented Jan 4, 2026 •

edited

Loading

Increase code coverage for `RecurrencePatternEvaluator`

axunonb Jan 5, 2026 •

edited

Loading

axunonb Jan 4, 2026 •

edited

Loading