Lint error with also files and digest+modtime checking

[ndmitchell]

Given the test suite fragment:

[obj "rewrite1",obj "rewrite2"] &%> \outs -> do
    alwaysRerun
    forM_ outs $ \out -> writeFile' out "rewrite"

The run:

build ["rewrite1","--digest-and"]
build ["rewrite1","--digest-and","--lint","--sleep"]

Will fail with a lint error. The sequence of events the second time is:

• I load rewrite1 and determine the file has not changed modtime or hash, but that I depend on rewrite1 rewrite2.
• I load rewrite1 rewrite2 and determine the files have not changed modtime or hash, but that I depend on alwaysRerun which has changed.
• I rerun rewrite1 rewrite2 which updates both files in their modtime but not hash, so I report that neither file has changed.
• Since nothing has changed, I record that rewrite1 has not changed and continue.

This bug is reduced down from snowleopard/hadrian#206

Two possible solutions:

• Also files should not be storing the timestamp twice, which is a (minor) performance issue as well. There are a few plans around to fix that.
• I could check the storedValue after building all the dependencies. That seems a reasonable thing to do and kills this bug regardless.

[Mathnerd314]

I think there's a third solution:

• Change ((==) EqualCheap . equal key result) to ((/=) NotEqual . equal key result)

[ndmitchell]

@Mathnerd314 - yes, that works too. In fact, I originally had that as the lint checking function, but there are a few bugs you can introduce that don't get caught by that lesser check - can't remember what off the top of my head but I'll try and remember.

[ndmitchell]

In fact checking the stored value after may be a performance win anyway.

[ndmitchell]

As per #453, if checking the stored value after is valid then the builtin rule should take a Bool. If not, it should take an Action Bool - so figuring out if that order is valid matters.

My mild concern is that if you change the rules of Main.exe to no longer require A.hs, then delete Main.exe, you are going to end up rebuilding A.hs anyway - whereas currently you wouldn't. But alternatively I think you've got into a bit of a weird state anyway, you'll probably recover anyway, and it's only giving marginally more safety at most.

[Mathnerd314]

change the rules of Main.exe

If the rules change, Shake needs to discard the old result and its stored dependencies regardless (see #446). Doing dependencies first also disallows "unstable" rules that use different files depending on the phase of the moon, but I don't see that as a loss (instead, you depend on a separate "phase of the moon" rule first).

Were the bugs with EqualCheap vs. EqualExpensive found around dad4f85 and d073276?

[ndmitchell]

OK, I'm convinced, stored value should be checked at the end of all the dependencies, not the beginning. Allowing rules on the phase of the moon should be explicit (although I do worry about Werewolf exceptions 😃).

Yep, those do look like the bugs I must have found.

[ndmitchell]

Solved by pushing the stored value check to after the dependencies have been checked.

[ndmitchell]

In fact, looking at the code, I don't see how this can't have broken AssumeSkip. It didn't break any tests, and I suspect that's mostly because no one knows how to use AssumeSkip - see #436. I'll leave it for now.