Bysetpos fix - #1040
Open
mkmelin wants to merge 3 commits into
Open
Bysetpos fix#1040mkmelin wants to merge 3 commits into
mkmelin wants to merge 3 commits into
Conversation
…ixes kewisch#1038 RecurIterator#next loops until check_contracting_rules() accepts the instance it has just stepped to. For a rule whose contracting parts exclude everything it would ever produce, FREQ=DAILY;BYMONTH=2;BYMONTHDAY=30 for example, that never happens and the call does not return. MONTHLY and YEARLY have invalid_count bail-outs, but those only count periods the expansion itself rejected, so they do not fire here either, and the other frequencies have no bail-out at all. Give up after 50000 periods. Every combination of the BY* parts comes round again within a leap year cycle, which is 480 monthly periods and 14610 daily ones, so a rule that can be satisfied at daily granularity or coarser is unaffected: BYMONTH=2;BYMONTHDAY=29;BYDAY=MO still resolves eighteen years out. Hourly and finer rules whose contracting parts only match years apart will now stop early, which seemed the better trade against a call that never returns.
BYSETPOS was honoured on two paths only, MONTHLY with BYDAY and YEARLY with BYDAY and BYMONTH, and silently ignored everywhere else: FREQ=DAILY;BYHOUR=9,12,17;BYSETPOS=-1 handed out all three hours of every day rather than the last one. Where it was honoured it numbered the instances of each month, so a yearly rule spanning several months picked one out of each of them instead of one out of the year. Replace both with a single implementation that works for any rule. A second iterator expands the rule without its BYSETPOS part, and the outer one takes a period of that expansion at a time and hands out the instances the positions name. A period is gathered from its start rather than from DTSTART, because the positions count the instances the period holds and not only those that come after the start date: the example in RFC 5545 section 3.8.5.3, the third TU, WE or TH of the month from a DTSTART on that third one, needs the two before it to be counted. Instances before DTSTART are dropped once they have been counted. While a period is gathered only the instances a position can name are kept, the first max(+n) and the last max(-n) of it. A period of a rule combining BYHOUR, BYMINUTE and BYSECOND holds tens of millions of instances, which is more memory than there is to give. COUNT and UNTIL bound what BYSETPOS hands out rather than the set it picks from, so the expansion runs without them. Cutting a period short at UNTIL would change which instance BYSETPOS=-1 names. Two expectations change. A rule whose start date is not one of the instances its positions name no longer hands the start date out first, so FREQ=MONTHLY;BYSETPOS=-1;BYDAY=SU,MO,TU,WE,TH,FR,SA from 2016-01-01 begins on the 31st rather than the 1st. And a rule naming a position its periods never reach now has no occurrences, a secondly period holding one instance and BYSETPOS=2 asking for a second.
An imported event repeating on the 15th of June at 9 AM and 5 PM, FREQ=YEARLY;BYMONTH=6;BYMONTHDAY=15;BYHOUR=9,17, only showed the 9 AM one. BYHOUR, BYMINUTE and BYSECOND expand a yearly rule rather than limiting it (RFC 5545 section 3.3.10, table 1): FREQ gives the size of the period the BY* parts apply to, not the number of occurrences. RecurIterator#next reads a zero from next_year as "no instance found this year" and counts it against the run of empty years it gives up after. But next_year also returned zero when next_hour had merely stepped to the next instance within the day, which is where those parts put their instances, so each one was generated and then discarded. Only the first combination of each day survived. next_month returns valid in the same situation; the yearly path was alone in not doing so. Return valid there too, unless the day is one the year does not have, where _nextByYearDay already refuses to produce an instance and no hour of it is one either. That check moves into a helper the two share. A rule with more combinations in a day than the 28 empty years next allows used to exhaust them and stop after one occurrence, so it no longer stops early.
This branch has not been deployed
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
1 participant
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.
Bug 2058960 related fix.