Skip to content

Fixes for RRULE:FREQ=YEARLY combined with BYMONTH #884

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.

Sign up for GitHub

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

Merged
axunonb merged 3 commits into from
Dec 1, 2025
Merged

Fixes for RRULE:FREQ=YEARLY combined with BYMONTH #884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf2d188
Refactored `RecurrencePatternEvaluator` and `Evaluator`
axunonb Nov 30, 2025
7062dcc
chore: Remove code duplications in `RecurrencePatternEvaluator.GetMon…
axunonb Nov 30, 2025
84eb777
Apply some GitHub Pilot review suggestions
axunonb Nov 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions Ical.Net.Tests/RecurrenceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ public void ByMonth2()
.TakeWhileBefore(new CalDateTime(2000, 12, 31)).ToList();
var evt2Occurrences = evt2.GetOccurrences(new CalDateTime(1997, 9, 1))
.TakeWhileBefore(new CalDateTime(2000, 12, 31)).ToList();
Assert.That(evt1Occurrences.Count == evt2Occurrences.Count, Is.True,
Assert.That(evt1Occurrences.Count, Is.EqualTo(evt2Occurrences.Count),
"ByMonth1 does not match ByMonth2 as it should");
for (var i = 0; i < evt1Occurrences.Count; i++)
Assert.That(evt2Occurrences[i].Period, Is.EqualTo(evt1Occurrences[i].Period),
Expand Down Expand Up @@ -1654,7 +1654,7 @@ public void HourlyUntil1()
new Period(new CalDateTime(1997, 9, 2, 9, 0, 0, _tzid), Duration.FromHours(1)),
new Period(new CalDateTime(1997, 9, 2, 12, 0, 0, _tzid), Duration.FromHours(1)),
new Period(new CalDateTime(1997, 9, 2, 15, 0, 0, _tzid), Duration.FromHours(1)),
new Period(new CalDateTime(1997, 9, 2, 18, 0, 0, _tzid), Duration.FromHours(1)),
new Period(new CalDateTime(1997, 9, 2, 18, 0, 0, _tzid), Duration.FromHours(1))
},
timeZones: null
);
Expand Down Expand Up @@ -2009,13 +2009,16 @@ public void YearlyInterval1()
var iCal = Calendar.Load(IcsFiles.YearlyInterval1)!;
EventOccurrenceTest(
iCal,
new CalDateTime(2006, 1, 1, 7, 0, 0, _tzid),
new CalDateTime(2007, 1, 31, 7, 0, 0, _tzid),
new[]
{
new Period(new CalDateTime(2007, 1, 8, 7, 0, 0, _tzid), Duration.FromHours(24)),
new Period(new CalDateTime(2007, 1, 9, 7, 0, 0, _tzid), Duration.FromHours(24))
},
new CalDateTime(2005, 1, 11, 7, 0, 0, _tzid),
new CalDateTime(2010, 1, 31, 7, 0, 0, _tzid),
[
new Period(new CalDateTime(2005, 4, 11, 7, 0, 0, _tzid), Duration.FromHours(24)),
new Period(new CalDateTime(2005, 4, 12, 7, 0, 0, _tzid), Duration.FromHours(24)),
new Period(new CalDateTime(2007, 4, 9, 7, 0, 0, _tzid), Duration.FromHours(24)),
new Period(new CalDateTime(2007, 4, 10, 7, 0, 0, _tzid), Duration.FromHours(24)),
new Period(new CalDateTime(2009, 4, 13, 7, 0, 0, _tzid), Duration.FromHours(24)),
new Period(new CalDateTime(2009, 4, 14, 7, 0, 0, _tzid), Duration.FromHours(24))
],
null
);
}
Expand Down
7 changes: 6 additions & 1 deletion Ical.Net/Evaluation/Evaluator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ protected void IncrementDate(ref CalDateTime dt, RecurrencePattern pattern, int
dt = old.AddDays(-old.Day + 1).AddMonths(interval);
break;
case FrequencyType.Yearly:
dt = old.AddDays(-old.DayOfYear + 1).AddYears(interval);
// When a rule uses BYWEEKNO, recurrence enumeration
// is based on week numbers relative to the year.
// So we preserve January 1st when adding years; otherwise, preserve month/day.
Copy link
Collaborator

@minichma minichma Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe more precisely

So we preserve the weekday when using BYWEEKNO and preserve month/day otherwise?

Copy link
Collaborator Author

@axunonb axunonb Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is more precise.

dt = (pattern.ByWeekNo.Count != 0)
? old.AddDays(-old.DayOfYear + 1).AddYears(interval)
: old.AddYears(interval);
break;
default:
// Frequency should always be valid at this stage.
Expand Down
182 changes: 147 additions & 35 deletions Ical.Net/Evaluation/RecurrencePatternEvaluator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,20 @@ private IEnumerable<CalDateTime> EnumerateDates(CalDateTime originalDate, CalDat
var dateCount = 0;
while (true)
{
if (searchEndDate < GetIntervalLowerLimit(intervalRefTime, pattern))
var lowerLimit = GetIntervalLowerLimit(intervalRefTime, pattern, originalDate);

if (searchEndDate < lowerLimit)
break;

var candidates = GetCandidates(intervalRefTime, pattern, expandBehavior);
// Use the original DTSTART month as the anchor for YEARLY rules when BYMONTH is not specified.
// This ensures BYMONTHDAY expansion/limiting is performed relative to the DTSTART month
// (the expected behavior for yearly rules without an explicit BYMONTH), instead of the
// current interval reference month.
var anchorMonth = pattern is { Frequency: FrequencyType.Yearly, ByMonth.Count: 0 }
? originalDate.Month
Copy link
Collaborator

@minichma minichma Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When changing this to return intervalRefTime.Month in both cases, all tests still run through. Would be great to have a test case where this distinction is essential.

Copy link
Collaborator Author

@axunonb axunonb Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The anchorMonth was one of the ideas introduced in a very first draft. It was only partly helpful. When investigating further, anchorMonth became fully redundant - which I oversaw. Excellent finding, Please keep up these reviews.

: intervalRefTime.Month;

var candidates = GetCandidates(intervalRefTime, pattern, expandBehavior, anchorMonth);

foreach (var t in candidates.Where(t => t >= originalDate))
{
Expand Down Expand Up @@ -210,16 +220,77 @@ private IEnumerable<CalDateTime> EnumerateDates(CalDateTime originalDate, CalDat
/// Find the lowest possible date/time for a recurrence in the given interval.
/// </summary>
/// <remarks>
/// Usually intervalRefTime is either at DTSTART or later at the start of the interval.
/// However, for YEARLY recurrences with BYWEEKNO=1 there could be recurrences before
/// Jan 1st, so we need to adjust the intervalRefTime to the start of the week.
/// For most frequencies the interval's lower limit is simply the provided
/// <paramref name="intervalRefTime"/>. YEARLY rules require special handling:
/// - If BYMONTH is present and BYWEEKNO is not, an occurrence for the interval
/// might fall earlier in the year than the intervalRefTime's month/day. In
/// that case we compute the earliest possible date/time that could be
/// generated for the interval (earliest month/day/hour/minute/second).
/// - If BYWEEKNO is present, the interval may contain days from the previous
/// or next year (ISO week boundaries). In that case we adjust the interval
/// start to the first day of the configured week so we don't miss candidates
/// that belong to the week containing Jan 1st.
/// </remarks>
private static CalDateTime GetIntervalLowerLimit(CalDateTime intervalRefTime, RecurrencePattern pattern)
private static CalDateTime GetIntervalLowerLimit(CalDateTime intervalRefTime, RecurrencePattern pattern, CalDateTime originalDate)
{
var intervalLowerLimit = intervalRefTime;
if ((pattern.Frequency == FrequencyType.Yearly) && (pattern.ByWeekNo.Count != 0))
intervalLowerLimit = GetFirstDayOfWeekDate(intervalRefTime, pattern.FirstDayOfWeek);
return intervalLowerLimit;
switch (pattern)
{
case { Frequency: FrequencyType.Yearly, ByMonth.Count: > 0, ByWeekNo.Count: 0 }:
Copy link
Collaborator

@minichma minichma Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this whole method is not perfectly intuitive, maybe we should improve the naming or the description. I.e. if I remember correctly, it is there for a minor performance improvement, i.e. to avoid iterating an extra year at the end of the sequence where this can be avoided, maybe also to avoid running into a EvaluationLimitExceededException in certain cases. However, for some reason this is only implemented for the case of BYWEEKNO. I assume this has historic reasons and might be related to the fact that in case of BYWEEKNO we might need to step back to the previous interval (at least when implemented the way it is today). For all other cases we don't do this extra calculation, which brings up the question whether we should do it for this new special case. I'd rather say no, i.e. I'd rather recommend implementing it for all cases (which I'd rather try to avoid) or only for the minimum set of cases necessary. But I could also easily miss something.

Copy link
Collaborator Author

@axunonb axunonb Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all other cases we don't do this extra calculation, which brings up the question whether we should do it for this new special case. I'd rather say no, i.e. I'd rather recommend implementing it for all cases (which I'd rather try to avoid)

Yes, agree, that's not clean. Covering special cases here is a risk. Not sure, which further special cases we can expect besides those just implemented. For the moment, they do their job.

{
// When evaluating a YEARLY rule that restricts months (BYMONTH) but not
// week numbers, it's possible that the earliest candidate inside the
// interval is in an earlier month/day than `intervalRefTime` (for example
// BYMONTH=1 while intervalRefTime is anchored on a later month). To avoid
// terminating enumeration prematurely (with a coarse UNTIL cutoff), compute
// the earliest plausible date/time for this interval and use that as the
// lower limit.
//
// We pick:
// - year = intervalRefTime.Year
// - month = smallest BYMONTH or the original DTSTART month if BYMONTH absent
// - day = smallest BYMONTHDAY (clamped to daysInMonth) or original DTSTART day
// - time components = smallest BYHOUR/BYMINUTE/BYSECOND or original DTSTART time
//
// This is a conservative earliest-possible candidate; it must not exclude
// any valid occurrence for the interval.
var year = intervalRefTime.Year;

// Determine the earliest month we could possibly generate for this interval.
var month = pattern.ByMonth.Min();

// Determine an appropriate day in the month.
var daysInMonth = Calendar.GetDaysInMonth(year, month);
int day;
if (pattern.ByMonthDay.Count > 0)
{
var md = pattern.ByMonthDay.Min();
day = md > 0 ? Math.Min(md, daysInMonth) : Math.Max(1, daysInMonth + md + 1);
Copy link
Collaborator

@minichma minichma Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not correct if positive/negative values are mixed, e.g. BYMONTHDAY=2,-2.

}
else
{
// default to original date's day, constrained to the target month length
day = Math.Min(originalDate.Day, daysInMonth);
}

// Determine earliest time components
var hour = pattern.ByHour.Count > 0 ? pattern.ByHour.Min() : originalDate.Hour;
var minute = pattern.ByMinute.Count > 0 ? pattern.ByMinute.Min() : originalDate.Minute;
var second = pattern.BySecond.Count > 0 ? pattern.BySecond.Min() : originalDate.Second;

return new CalDateTime(year, month, day, hour, minute, second, intervalRefTime.TzId);
}
case { Frequency: FrequencyType.Yearly, ByWeekNo.Count: not 0 }:
{
// YEARLY with BYWEEKNO: weeks may span year boundaries. Move the
// interval lower limit to the first day of the week so expansion over
// the week (including days before Jan 1st) is handled correctly.
return GetFirstDayOfWeekDate(intervalRefTime, pattern.FirstDayOfWeek);
}
default:
{
return intervalRefTime;
}
}
}

private struct ExpandContext
Expand All @@ -241,15 +312,17 @@ private struct ExpandContext
/// <param name="pattern"></param>
/// <param name="expandBehaviors"></param>
/// <returns>A list of possible dates.</returns>
private IEnumerable<CalDateTime> GetCandidates(CalDateTime date, RecurrencePattern pattern, bool?[] expandBehaviors)
private IEnumerable<CalDateTime> GetCandidates(CalDateTime date, RecurrencePattern pattern, bool?[] expandBehaviors, int anchorMonth)
{
var expandContext = new ExpandContext() { DatesFullyExpanded = false };

IEnumerable<CalDateTime> dates = [ date ];
IEnumerable<CalDateTime> dates = [date];
dates = GetMonthVariants(dates, pattern, expandBehaviors[0]);
dates = GetWeekNoVariants(dates, pattern, expandBehaviors[1], ref expandContext);
dates = GetYearDayVariants(dates, pattern, expandBehaviors[2], ref expandContext);
dates = GetMonthDayVariants(dates, pattern, expandBehaviors[3], ref expandContext);
// Use the provided anchorMonth (typically the original DTSTART month) so BYMONTHDAY expansion
// is performed relative to the intended month when BYMONTH is not specified.
dates = GetMonthDayVariants(dates, pattern, expandBehaviors[3], ref expandContext, anchorMonth: anchorMonth);
dates = GetDayVariants(dates, pattern, expandBehaviors[4], ref expandContext);
dates = GetHourVariants(dates, pattern, expandBehaviors[5]);
dates = GetMinuteVariants(dates, pattern, expandBehaviors[6]);
Expand Down Expand Up @@ -316,10 +389,8 @@ private static IEnumerable<CalDateTime> GetMonthVariants(IEnumerable<CalDateTime
return dates.Where(date => pattern.ByMonth.Contains(date.Month)
|| pattern.ByMonth.Contains(date.AddDays(6).Month));
}
else
{
return dates.Where(date => pattern.ByMonth.Contains(date.Month));
}

return dates.Where(date => pattern.ByMonth.Contains(date.Month));
}

/// <summary>
Expand Down Expand Up @@ -348,12 +419,12 @@ private static IEnumerable<CalDateTime> GetWeekNoVariants(IEnumerable<CalDateTim

private static IEnumerable<CalDateTime> GetWeekNoVariantsExpanded(IEnumerable<CalDateTime> dates, RecurrencePattern pattern)
{
foreach ((var t, var weekNo) in dates.SelectMany(t => GetByWeekNoForYearNormalized(pattern, t.Year), (t, weekNo) => (t, weekNo)))
foreach (var (t, weekNo) in dates.SelectMany(t => GetByWeekNoForYearNormalized(pattern, t.Year), (t, weekNo) => (t, weekNo)))
{
var date = t;

// Make sure we start from a reference date that is in a week that belongs to the current year.
// Its not important that the date lies in a certain week, but that the week belongs to the
// It's not important that the date lies in a certain week, but that the week belongs to the
// current year and that the week day is preserved.
if (date.Month == 1)
date = date.AddDays(7);
Expand Down Expand Up @@ -394,11 +465,30 @@ private static List<int> GetByWeekNoForYearNormalized(RecurrencePattern pattern,
}

/// <summary>
/// Applies BYYEARDAY rules specified in this Recur instance to the specified date list.
/// If no BYYEARDAY rules are specified, the date list is returned unmodified.
/// Applies <c>BYYEARDAY</c> rules from <paramref name="pattern"/> to a sequence of candidate dates.
/// </summary>
/// <param name="dates">The list of dates to which the BYYEARDAY rules will be applied.</param>
/// <returns>The modified list of dates after applying the BYYEARDAY rules.</returns>
/// <param name="dates">Sequence of candidate dates (typically interval reference dates or previously-expanded dates).</param>
/// <param name="pattern">Recurrence pattern containing the <c>BYYEARDAY</c> values.</param>
/// <param name="expand">
/// Controls the operation mode:
/// - <c>true</c> : perform an expand operation — each input date is expanded into the concrete dates
/// represented by the <c>BYYEARDAY</c> values for that date's year.
/// - <c>false</c> : perform a limit/filter operation — only return input dates that match any of the
/// <c>BYYEARDAY</c> values for the input date's year.
/// - <c>null</c> : no operation, return <paramref name="dates"/> unchanged.
/// </param>
/// <param name="expandContext">
/// Context that indicates whether earlier parts have already fully expanded the candidate set.
/// If <see cref="ExpandContext.DatesFullyExpanded"/> is <c>true</c> then expansion must not be
/// performed again and the method should behave in limit mode.
/// When this method performs an expansion it will set <see cref="ExpandContext.DatesFullyExpanded"/>
/// to <c>true</c> to prevent later parts from expanding again.
/// </param>
/// <returns>
/// A sequence of dates after applying the <c>BYYEARDAY</c> rules.
/// Expanded dates produced for a given input are constrained to the same calendar year as the input date;
/// out-of-range <c>BYYEARDAY</c> values (e.g. +-366 in non-leap years) are ignored.
/// </returns>
private static IEnumerable<CalDateTime> GetYearDayVariants(IEnumerable<CalDateTime> dates, RecurrencePattern pattern, bool? expand, ref ExpandContext expandContext)
{
if (expand is null || pattern.ByYearDay.Count == 0)
Expand Down Expand Up @@ -448,12 +538,11 @@ from yearDay in pattern.ByYearDay
}

/// <summary>
/// Applies BYMONTHDAY rules specified in this Recur instance to the specified date list.
/// Applies BYMONTHDAY rules specified in this RecurrencePattern instance to the specified date list.
/// If no BYMONTHDAY rules are specified, the date list is returned unmodified.
/// </summary>
/// <param name="dates">The list of dates to which the BYMONTHDAY rules will be applied.</param>
/// <returns>The modified list of dates after applying the BYMONTHDAY rules.</returns>
private static IEnumerable<CalDateTime> GetMonthDayVariants(IEnumerable<CalDateTime> dates, RecurrencePattern pattern, bool? expand, ref ExpandContext expandContext)
private static IEnumerable<CalDateTime> GetMonthDayVariants(IEnumerable<CalDateTime> dates, RecurrencePattern pattern, bool? expand, ref ExpandContext expandContext, int anchorMonth)
{
if (expand == null || pattern.ByMonthDay.Count == 0)
return dates;
Expand All @@ -465,22 +554,45 @@ private static IEnumerable<CalDateTime> GetMonthDayVariants(IEnumerable<CalDateT
}

// limit behavior
return GetMonthDayVariantsLimited(dates, pattern);
return GetMonthDayVariantsLimited(dates, pattern, anchorMonth);
}

private static IEnumerable<CalDateTime> GetMonthDayVariantsLimited(IEnumerable<CalDateTime> dates, RecurrencePattern pattern)
private static IEnumerable<CalDateTime> GetMonthDayVariantsLimited(IEnumerable<CalDateTime> dates, RecurrencePattern pattern, int anchorMonth)
{
// 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(CalDateTime candidate, IEnumerable<int> monthDays)
{
var daysInMonth = Calendar.GetDaysInMonth(candidate.Year, candidate.Month);
foreach (var monthDay in monthDays)
{
var byMonthDay = monthDay > 0 ? monthDay : (daysInMonth + monthDay + 1);
if (candidate.Day == byMonthDay)
return true;
}
return false;
}

foreach (var date in dates)
{
var daysInMonth = Calendar.GetDaysInMonth(date.Year, date.Month);
foreach (var monthDay in pattern.ByMonthDay)
if (pattern.ByMonth.Count > 0)
{
var byMonthDay = (monthDay > 0) ? monthDay : (daysInMonth + monthDay + 1);
if (date.Day == byMonthDay)
{
// If BYMONTH is specified, the date must be in one of those months
// and match a BYMONTHDAY value.
if (!pattern.ByMonth.Contains(date.Month))
continue;

if (MatchesAnyMonthDay(date, pattern.ByMonthDay))
yield return date;
}
else
{
// When BYMONTH is not specified, only consider dates in the anchor month.
if (date.Month != anchorMonth)
Copy link
Collaborator

@minichma minichma Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add some more context on why this was added. Not sure it is fully correct. Consider the following test case, which fails now (could be added to RecurrenceTestCases.txt if considered correct).

RRULE:FREQ=YEARLY;BYYEARDAY=-1,1;BYMONTHDAY=-1,1;COUNT=3
DTSTART:20250101
INSTANCES:20250101,20251231,20260101

Copy link
Collaborator Author

@axunonb axunonb Dec 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I consider the test correct. It worked before this commit. Sadly we didn't have the test before - so thanks a lot.
I wonder whether using an anchor month has even more side effects than this one.
Anyway, removing if (date.Month != anchorMonth) continue; makes sense, and all tests including the new one above pass.

continue;

if (MatchesAnyMonthDay(date, pattern.ByMonthDay))
yield return date;
break;
}
}
}
}
Expand Down
Loading