[Test] [Bug] Trigger the overpruned dependency bug

[effectfully]

The added test fails with

test
  Plugin
    Data
      9.6
        families
          stakingCredential: FAIL
            Exception: Error: Error from the PIR compiler:
                   Error from the PLC compiler:
                   Free type variable at  Ann {annInline = MayInline, annSrcSpans = { no-src-span }} :  Credential

The bug is in the dead code eliminator. To be precise, it's this part that it doesn't handle correctly despite promising to in PlutusIR.Analysis.Dependencies:

{- Note [Dependencies for datatype bindings, and pruning them]
At face value, all the names introduced by datatype bindings should depend on each other.
Given our meaning of "A depends on B", since we cannot remove any part of the datatype
binding without removing the whole thing, they all depend on each other

However, there are some circumstances in which we *can* prune datatype bindings.

In particular, if the datatype is only used at the type-level (i.e. all the term-level parts
(constructors and destructor) are dead), then we are free to completely replace the binding
with one for a trivial type with the same kind.

This is because there are *no* term-level effects, and types are erased in the end, so
in this case rest of the datatype binding really is superfluous.

But how do we represent this in the dependency graph? We still need to have proper dependencies
so that we don't make the wrong decisions wrt transitively used values, e.g.

let U :: * = ...
let datatype T = T1 | T2 U
in T1

Here we need to not delete U, even though T2 is "dead"!

The solution is to focus on the meaning of "dependency": with the pruning that we can do, we *can*
remove all the term level bits en masse, but only en-mass. So we need to make *them* into a clique,
so that this is visible to the dependency analysis.
-}

I don't know what went wrong there.

[effectfully]

@michaelpj any idea?

[michaelpj]

What happens if you get rid of the bit where it tries to prune dead constructors?

[effectfully]

What happens if you get rid of the bit where it tries to prune dead constructors?

The test becomes green, see 229b690.

[michaelpj]

Since you have a small test case I would dump the output from just after the bad dead code pass and see what's wrong. It looks like a simple case we should handle properly...

[michaelpj]

From the error it seems like we get rid of Credential entirely, which seems odd.

[effectfully]

Since you have a small test case I would dump the output from just after the bad dead code pass and see what's wrong. It looks like a simple case we should handle properly...

Yes, I've done exactly that when investigating and you're right that this is what's happening:

From the error it seems like we get rid of Credential entirely, which seems odd.

[michaelpj]

I don't see why from just looking at it, some debugging is going to be required I think!

[effectfully]

I don't see why from just looking at it, some debugging is going to be required I think!

Wanna do some debugging on a weekend? 🙂

[effectfully]

@zliu41 is this large enough difference to keep the bug in the dead code eliminator that causes necessary constructors to get eliminated?

[zliu41]

10% is bad but only a small number of golden files are affected - I'm fine either way. What's your preference?

[effectfully]

I guess let's keep the bug in then, I think it's quite low priority compared to other things and I don't feel like studying the DCE algo at the moment (or ever haha). I'll reflect this problem in an issue.

[zliu41]

Sounds good.

[zliu41]

I'm not quite sure what this does; worth some comment. There should also be some explanation or reference to the bug here.

[effectfully]

I have no idea either, that's Michael code and as you can see above Michael isn't particularly keen on dealing with it :)

[zliu41]

10% is bad but only a small number of golden files are affected - I'm fine either way. What's your preference?

[effectfully]

I've added an entry to #5967, so that we don't forget about this bug, and let's close this PR for now.