[compiler] fix a bug in NormalizeNames

[patrick-schultz]

Change Description

Fixes a bug that was reported in zulip.

Background: we use a function freshName to generate names to use in the IR which are guaranteed to not already be in use in the IR. We do that by maintaining a global variable counting the number of fresh names ever generated, and freshName returns the string s"__iruid_$i". NormalizeNames changes all the names used in the IR such that any name is defined at most once in the IR. It is also currently required to be deterministic, so that two IRs which are equal up to the choice of variable names (they are alpha equivalent) will be strictly equal after normalizing names. It does this by maintaining a local counter of how many new names have be generated, and each new variable is again the string s"__iruid_$i".

Here's the bug: it's possible (though difficult) to run NormalizeNames before freshName has been called many times. Say the IR has 10 variables which are renamed, so the last variable generated by NormalizeNames is "__iruid_10", and suppose that freshName hasn't been used yet. Now say Simplify is run, and a simplification rule needs to generate a fresh name. That name will be "__iruid_1" (since this is the first call to freshName). But that name is already used in the IR (generated by NormalizeNames), breaking the fundamental requirement of freshName. This is a problem because a reference to the existing use of "__iruid_1" might end up in the scope of the newly introduced binding of "__iruid_1", changing its meaning (and probably its type, if we're lucky).

The fix here is simple: use a different prefix in the names generated by NormalizeNames and freshName, so they can never collide.

Security Assessment

Delete all except the correct answer:

• This change has no security impact

Impact Description

Non-functional change in the query compiler.

(Reviewers: please confirm the security impact before approving)

[ehigham]

For such a simple fix, this is deceptively hard to test well as it requires a number preconditions. This fix will be true by construction when we use object hash instead of strings. Therefore I don't think it's worth adding a complicated or insufficient test.

[chrisvittal]

Thanks for the quick fix!