Semantics of index-state in cabal.project

[andreabedini]

At IOG we have started using a custom hackage repositories, which we informally call CHaP.

To add the custom repository to a project we have been doing

repository cardano-haskell-packages
  url: https://input-output-hk.github.io/cardano-haskell-packages
  secure: True
  root-keys:
    -- snip

index-state: 2022-07-01T00:00:00Z
index-state: cardano-haskell-packages 2022-10-20T00:00:00Z

We thought the first index-state would apply to all active repositories, and the second one only to the specified repository. (For context: we did this because we were working around a limination with another tool which we use to parse cabal.project).

But we were wrong! After a while it became clear that the second index-state completely overrides the first (as the first was never there).

Indeed, the correct way to express two index-states would be

index-state:
  , hackage.haskell.org 2022-07-01T00:00:00Z
  , cardano-haskell-packages 2022-10-20T00:00:00Z

1. Is this the intended behaviour?
2. Should cabal accept multiple index-state stanza (for different repos)
3. Should cabal give a warning when it sees stanzas which override previos ones?

Thanks!

Ping @michaelpj @angerman

[michaelpj]

Just checking @andreabedini, I think we found that this also doesn't work?

index-state: hackage.haskell.org 2022-07-01T00:00:00Z
index-state: cardano-haskell-packages 2022-10-20T00:00:00Z

In terms of expected semantics, my naive expectation is that cabal does something like "overriding union" when it sees a subsequent stanza of the same kind, i.e. it unions as much as possible, and where there's a conflict it takes the later one. e.g. if you have two constraints stanzas it will take the union of them unless they conflict.

So I think the surprise is that we would expect cabal to be able to do some unioning here but in fact it just overrides the whole value. Especially in the case where there are two index-states for different repositories (the one with the "unqualified" index-state and the "qualified" one is a bit trickier).

[michaelpj]

Rough diagnosis: the TotalIndexState field in SharedProjectConfig is a Flag TotalIndexState, which has a "first" semigroup instance. Solution:

• Change the field to Maybe TotalIndexState
• Give RepoIndexState a "first" semigroup instance
• Give TotalIndexState a semigroup instance that unions the map of states with <>

If I understand correctly, that will make cabal do the right thing when parsing. I'm giving it a shot.

[andreabedini]

Yes,

index-state: hackage.haskell.org 2022-07-01T00:00:00Z
index-state: cardano-haskell-packages 2022-10-20T00:00:00Z

would use HEAD as index-state for hackage.

[michaelpj]

WIP here: https://github.com/michaelpj/cabal/tree/mpj/merge-index-states

Need to figure out how to write a test.

[ulysses4ever]

It's not exactly clear to me that the proposed semantics is any better than the current one, and the switch will probably break someone's code. It'd be interesting to try to solicit opinions on this on Discourse/Reddit/Cafe.

[mouse07410]

FWIW, the proposed semantics makes more sense to me than the currently-implemented one. I don't expect actual cider to be broken by this proposal.

I support this change, which actually is a fix.

[michaelpj]

It has been this way since it was introduced in #6597, I wonder if @phadej has a drive-by opinion.

[phadej]

The current behavior is so that if there is index-state: in the cabal.project, then the index-state is always completely specified. The type of the field in project config is TotalIndexState.

If you add a PartialIndexState type and change the project config to read that, and add conversion functions where GHC asks for them, then you get what you want.

TL;DR IIRC I did what was simplest at the time.

I doubt it will break much code as multiple repositories and index-states using them are not that common combination.
Maybe someone had written
index-state: hackage.haskell.org 2022-07-01T00:00:00Z today so that any extra repositories are at the latest, but it's not a big thing to fix (and that would work with older cabal-installs).
Also cabal freeze is explicit, writing all repositories in index-state (as well as active-repositories), so freeze files won't be broken, and freeze files are probably the files where those are used the most.

---

FWIW: flags is also a complete overwrite field in cabal.project, but that case is different as there wouldn't be a way to clear the flags if it was additive, where index-state: HEAD is the default setting for index-state.

[michaelpj]

The current behavior is so that if there is index-state: in the cabal.project, then the index-state is always completely specified. The type of the field in project config is TotalIndexState.

If you add a PartialIndexState type and change the project config to read that, and add conversion functions where GHC asks for them, then you get what you want.

It took me a while to see the problem, so let me see if I can say it right. The change I propose is basically just:

- Take the first value when combining
instance Semigroup RepoIndexState where
    s1 <> _ = s1

instance Semigroup TotalIndexState where
    (TIS s1 rs1) <> (TIS s2 rs2) = TIS (s1 <> s2) (Map.unionWith (<>) rs1 rs2)

data ProjectConfigShared =
...
  projectConfigIndexState :: Maybe TotalIndexState,
...

So the first index-state stanza will completely specify the index state, and subsequent ones will just override the bits that they specify. However the issue is with parsing: when we parse an index-state stanza we parse a TotalIndexState so we have to have a value for the first field, which will be HEAD if not supplied. So my change will still do the wrong thing here:

index-state: T1
index-state foo T2 -- will be parsed with HEAD as the first field, so will override T1

So indeed fixing this properly would need the first field to be Maybe RepoIndexState and then indeed it's not total. Probably still doable, but not the very simple fix I thought it was, alas.

[phadej]

No, you understood me wrong.

Nothing have to change about TotalIndexState. As it names implies, it's totally specified the index state.

The ProjectConfigShared have to be changed to

data ProjectConfigShared =
  ...
  projectConfigIndexState :: PartialIndexState,
  ...

where the naive implementation could be

newtype PartialIndexState = PartialIndexState (TotalIndexState -> TotalIndexState)

toTotalIndexState :: PartialIndexState -> TotalIndexState
toTotalIndexState (PartialIndexState f) = f headTotalIndexState

fromTotalIndexState :: TotalIndexState -> PartialIndexState
fromTotalIndexState tis = PartialIndexState (\_ -> tis)

however that representation won't work as you cannot pretty print it.

EDIT: I don't remember how TotalIndexState is represented now, maybe PartialIndexState can be represented very similarly, but please use different types for different things when they are different. That's important for cabal freeze, there the written PartailIndexState should specify the index-state fully.

[Mikolaj]

@andreabedini: do you have a good enough workaround?

@michaelpj: is there a way to cheaply salvage your work and improve the situation, before you completely context switch?

[andreabedini]

@Mikolaj yes, we have a workaround. Thanks for asking.

[michaelpj]

I got stuck thinking about what @phadej said. I think the fundamental change is not that hard, it's just representing it in a way that matches the way the cabal devs want to think about it.

• There are two types
  • One representing a potentially not-fully-specified index-state configuration, which is the sort of thing you get from parsing a project file (PartialIndexState? IndexStateConfig?). This has an obvious Semigroup instance so the cabal parsing setup will combine them in the right way.
  • One represents a fully-specified index-state configuration (TotalIndexState).
• I need to at least try having both.
• I'm not 100% sure if we definitely need TotalIndexState, that was partly what I wanted to investigate.
• I need to figure out how to write a test before I go any further anyway, and I was a bit stuck on that.

I think my previously linked tree provides some value if someone else wants to pick this up, I am planning to come back to this when I get some free cycles but I'm not sure when that will be, especially since we have a workaround.