Critique of immutable classes with circular references design, and better options

Question

I have a factory class that creates objects with circular references. I'd like them to be immutable (in some sense of the word) too. So I use the following technique, using a closure of sorts:

[<AbstractClass>]
type Parent() =
  abstract Children : seq<Child>
and Child(parent) =
  member __.Parent = parent

module Factory =

  let makeParent() =
    let children = ResizeArray()
    let parent = 
      { new Parent() with
        member __.Children = Seq.readonly children }
    [Child(parent); Child(parent); Child(parent)] |> children.AddRange
    parent

I like this better than an internal AddChild method because there's a stronger guarantee of immutability. Perhaps it's neurotic, but I prefer closures for access control.

Are there any pitfalls to this design? Are there better, perhaps less cumbersome, ways to do this?

Quondam

Not an answer, but [Child(parent); Child(parent); Child(parent)] |> List.iter children.Add could be reduced to [Child(parent); Child(parent); Child(parent)] |> children.AddRange. Personally, I use the approach you're demonstrating a lot when using ReadOnlyCollection<>. – Hokku

I don't know why, ReadOnlyCollection feels hacky to me. Throwing an exception when calling Add isn't quite the behavior I'd like. I'd rather the Add method not be there. – Quondam

I don't recall ever needing it in F#, but in C# there's no good built-in alternative. In any case, the same could be said of F#'s dict. :-] – Hokku

Good point. Although dict returning an IDictionary<'K, 'V> likely has more to do with compatibility than the lack of good alternatives. – Quondam

I think the same can be said of ReadOnlyCollection<'T>, given that it implements IList<'T>. In any case, we digress... ;-] – Hokku

Is the abstractness of Parent necessary? If true immutability is important, you can use records to create cycles in immutable object graphs (this can get a bit ugly, though). – Gramme

@kvb: No. It's abstract merely to facilitate the closure. I'm curious about how to do this with records, although, in my case it won't work because Parent implements an interface. (Correction: just discovered records can implement interfaces--that's news to me). – Quondam

@ildjarn,@daniel: doesn't Map<'k,'v> count as a built-in alternative to dict? – Lustral

@Lustral : They have different semantics. Map<> is immutable because it returns a new Map<> when you call Add, Remove, etc., leaving the original unchanged; dict returns an IDictionary<> which is immutable because it throws when you attempt to alter it by calling Add, Remove, etc. – Hokku

@Quondam - note that you don't actually need the AbstractClass with object expression for this solution to work: you can make Parent a normal class with a constructor taking Child seq and pass in Seq.readonly children since children is mutable and Seq.readonly will use its Enumerator under-the-hood. – Crony

@Stephen: Good point. I think that's an artifact of a prior design. – Quondam

@ildjarn: I see your point; they are conceptually different. – Lustral

Answer 1

You can use F#'s support for recursive initialization even when creating an instance of abstract class:

let makeParent() =
  let rec children = seq [ Child(parent); Child(parent); Child(parent) ]
  and parent = 
    { new Parent() with
      member __.Children = children }
  parent

When compiling the code, F# uses lazy values, so the value children becomes a lazy value and the property Children accesses the value of this lazy computation. This is fine, because it can first create instance of Parent (referencing the lazy value) and then actually construct the sequence.

Doing the same thing with records wouldn't work as nicely, because none of the computations would be delayed, but it works quite nicely here, because the sequence is not actually accessed when creating the Parent (if it was a record, this would be a field that would have to be evaluated).

The F# compiler cannot tell (in general) whether this is correct, so it emits a warning that can be disabled using #nowarn "40".

In general, I think that using let rec .. and .. to initialize recursive values is a good thing - it is a bit limited (one of the references must be delayed), but it forces you to keep the recursive references isolated and, I think, it keeps your code simpler.

EDIT To add an example when this may go wrong - if the constructor of Child tries to access the Children collection of its parent, then it forces evaluation of the lazy value before it can be created and you get a runtime error (which is what the warning says). Try adding this to the constructor of Child:

do printfn "%d" (Seq.length parent.Children)

Sitting

That's impressive. Should I be concerned about the resulting warning: This and other recursive references to the object(s) being defined will be checked for initialization-soundness at runtime through the use of a delayed reference. This is because you are defining one or more recursive objects, rather than recursive functions.? – Quondam

Very thorough answer. Thanks. – Quondam

@Quondam - added a few words about it. I wouldn't worry about this - usually, the warning means that you used it for the purpose for which it was designed :-). – Sitting

Seeing design considerations like this increases my appreciation for the F# team. – Quondam

Answer 2

I think that Tomas's answer is the way to go. However, for completeness I'll show how you could use recursive records to create cyclic immutable objects. This can potentially get quite ugly, so I've hidden the immutable record implementation behind some nicer properties:

type Parent = internal { children : Children option }
and internal Children = { first : Child; rest : Children option }
and Child = internal { parent : Parent }

let rec internal listToChildren = function
| [] -> None
| c::cs -> Some { first = c; rest = listToChildren cs }

let rec internal childrenToList = function
| None -> []
| Some { first = c; rest = cs } -> c::(childrenToList cs)

module Factory =
    let makeParent() = 
        let rec parent = { children = children }
        and child1 = { parent = parent }
        and child2 = { parent = parent }
        and child3 = { parent = parent }
        and children = [child1; child2; child3] |> listToChildren
        parent

type Parent with
    member p.Children = childrenToList p.children
type Child with
    member c.Parent = c.parent

Gramme

Wow. That's weird. Thanks for spelling it out, although, I'm not sure I totally understand it. – Quondam

@Gramme - won't childrenToList p.children be calculated on every call to Parent member Children? – Crony

Answer 3

I guess something like this can also be done:

type ParentFactory private (n) as X = 
    inherit Parent()
    let childs = [for i=1 to n do yield Child(X :> Parent)]
    override X.Children = childs |> List.toSeq;               
    static member Create n = (new ParentFactory(n)) :> Parent

Warren