scheduler: extract memo invalidation from the status field

[pmwhite]

The accumulation of the invalidation is orthogonal to what the build status is. Regardless of how the build progresses, we should always continually combine new build input change into the current invalidation, and we should only set the invalidation to empty when we also call Memo.reset with the current invalidation.

At the same time, it also makes sense to cleanup the handling of the build_input_change ivar. There is no need for it to be an option that is initialized on-demand, since it is easy and cheap to just create an initial ivar. In addition, we shouldn't care what the current build status is when waiting for a file to change. (I suspect that some of the reason for the complication is due to the --react-to-insignificant-file changes flag that no longer exists.) I think it should be handled in a similar way to the memo invalidation: it should be accumulated and reset at the same time. Here is an invariant: build_inputs_changed should be empty if and only if the memo invalidation is empty.

This patch doesn't claim to fix any bugs; it's just intended as a code cleanup.

[rgrinberg]

More precisely, fills past the first one are ignored. So I would call this idempotent instead.

[pmwhite]

Yes, good point. Note that since the data is unit, ignoring a fill is indistinguishable from not ignoring it. I've expanded the comment with more nuance. Feel free to edit it a bit if you prefer a different wording.

[rgrinberg]

The accumulation of the invalidation is orthogonal to what the build status is. Regardless of how the build progresses, we should always continually combine new build input change into the current invalidation, and we should only set the invalidation to empty when we also call Memo.reset with the current invalidation.

Previously, we had the invariant that when we're building something we have no pending invalidation. That seems kind of an important invariant to me.

[pmwhite]

Previously, we had the invariant that when we're building something we have no pending invalidation. That seems kind of an important invariant to me.

That invariant is still upheld, unless I'm mistaken. I think what you're saying that the types used to enforce the invariant, and with this change they don't; is that right? But I don't think the types were enforcing anything. Just because t.status is in the Building state doesn't mean there were no invalidations that we should respond to; we might have just ignored some. I'll admit that previously the types made it natural to update both the invalidation and the status at the same time, since it was in the same variable, but it also required a little bit more case analysis that pleasantly goes away with this change.

My ultimate plan is to get rid of the status field so that all these invariants are upheld by definition: rather than having to keep status in sync with invalidation, and cancel , we'll define "when we're building something" to mean "the cancel hasn't fired and the invalidation is empty." The refactor I'm trying to make is similar to going from 'a list option to 'a list because the None case always maps to [] (at least in this hypothetical scenario).

[rgrinberg]

I tried to think about how we could misuse an invalidation when status = Building but couldn't think of anything concrete.W ould it be helpful to add an assertion that the invalidations are empty when transitioning from Building to Standing_by?

[pmwhite]

That might be a good assertion to have, but if we merge the sequel to this PR or the alternative sequel, then it either wouldn't make sense because the status field would be gone, or it wouldn't be true anymore, because we'd actually leave the build in the Building state after an invalidation happens. How about we wait to see how the next PR ends up before adding the assertion to this PR?

[rgrinberg]

The assert isn't strictly necessary. It was just a suggestion.