There are two lost results for go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql on go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go. They have the exact same cause: in the following function

func closeFileDeferredIndirect(f *os.File) {
	var cont = func() {
		f.Close() // NOT OK, if `f` is writable
	}

	defer cont()
}

we don't have flow from capture variable f in the function literal to the use of f in f.Close(). This flow used to be caught by this disjunct in basicLocalFlowStep:

  // SSA -> Instruction
  exists(SsaDefinition pred, IR::Instruction succ |
    succ = pred.getVariable().getAUse() and
    nodeFrom = ssaNode(pred) and
    nodeTo = instructionNode(succ)
  )

but now that has changed to:

  // SSA defn -> first SSA use
  exists(SsaExplicitDefinition pred, IR::Instruction succ | succ = pred.getAFirstUse() |
    nodeFrom = ssaNode(pred) and
    nodeTo = instructionNode(succ)
  )
  or
  // SSA use -> successive SSA use
  // Note this case includes Phi node traversal
  exists(IR::Instruction pred, IR::Instruction succ | succ = getAnAdjacentUse(pred) |
    nodeFrom = instructionNode(pred) and
    nodeTo = instructionNode(succ)
  )

capture variable f is an SsaImplicitDefinition rather than an SsaExplicitDefinition, so the first of these disjuncts doesn't catch it. I am not sure which of these disjuncts should be amended to capture this flow.

Presumably this is because of switching from SsaDefinition to SsaExplicitDefintion, I suppose with intent to exclude phi nodes, but also excluding SsaVariableCapture by mistake.

DCA showed a few extra alerts, which I've pushed a commit to fix, and perhaps a very slight increase in analysis time.

I have just rebased this and fixed new test failures. DCA shows no significant performance change. There are three lost cleartext-logging results on one repo, which have the same source and sinks right next to each other, so there is probably one cause. I looked into it for a while and didn't see the problem.

Rebased again and ran DCA. The same alerts were lost as last time. This run might show a slight performance regression, about 3%, most notably on the two largest projects. I will do a QA run to test performance more thoroughly and see if there are more alert regressions (which might be easier to debug than the ones in DCA).

@smowton Do you think you'll have a chance to review this this week? I'm happy to talk you through it, or answer any questions. Obviously the changes to QL code are more important to review than the test changes, so I'd focus your attention there. Note that the commit history starts by doing everything needed to get tests passing and DCA and QA good for use-use flow and then making proper post-update nodes and doing the same again. So it is clear whether a test change is due to use-use flow or proper post-update nodes.

The most recent DCA run:

• I don't understand the two lost go/clear-text-logging results in cockroachdb. There were three of them without proper post-update nodes so I guess it's good we got one back. They are very long paths so not the most valuable alerts.
• The other 3 alert losses (ignoring duplicates) are all removing FPs caused by jumping backwards from the use to the def.
• The 2 alerts gains (ignoring duplicates) are TPs for go/unvalidated-url-redirection. While refactoring the field read logic I fixed a bug in the additional steps for this query. If x has type net/url.URL then we want to propagate taint from writes to its Host field to the struct itself. Previously we were accidentally missing cases where the base of the write was a struct literal rather than a variable, i.e. the struct was being tainted during initialisation.
• On average analysis time has increased by 3%. The two large ones have a 5% increase in analysis time. Maybe this is unavoidable because we are adding a lot of extra nodes? I seem to recall there's a trick where you only create post-update nodes when they will be used. I haven't done that yet because (a) it seems like an optimisation that should be applied after you've got everything working, so you don't accidentally remove too many PUNs without noticing and (b) the way it was done before involved basicLocalFlowStep, which works with nodes, and I don't think you can use that while defining what nodes exist, so I think you'd have to create an equivalent predicate on the level of Instructions.

The most recent QA run (I haven't finished going through the alert changes):

• Performance seems slightly worse but no large outliers (if anything there are some outliers where analysis time is significantly lower, which I don't understand as they aren't repos where we've removed a lot of results).
• 230 repos had alert changes, with an average alert diff of -5. Looking through the (repo,query) pairs with the highest alert diffs, they are all ones where we have removed a lot of FPs, or at least dubious alerts. For go/cleartext-logging, one effect of proper PUNs is to fix something where we could get multiple alerts for what should just be one alert. I haven't seen any obviously incorrect changes yet. I've only looked at lost alerts so far - next I'll look at gained alerts to check they make sense.

@smowton The latest QA run looks good. When I looked into two lost results for go/impossible-interface-nil-check and I realised that it needs a small fix, which I've added. It's only a quality query, so I don't think we need to redo QA or anything. I've also update the change note to include a bug I incidentally fixed during the refactor. I think this is ready to merge.