Skip to content

User documents and ADR statements about Old and EnsureAssignable that the code does not bear out #62

Open

Description

@cwouyang

While the documents for #57 were reviewed, one reviewer read README.md,
docs/examples/API_REFERENCE.md, docs/examples/USAGE_EXAMPLES.md and the [Unreleased] section
of CHANGELOG.md whole against the code. It found statements about Old<T>() and
EnsureAssignable<T>() that the code does not bear out. They are older than #57 and were left out
of it on purpose. Most were read from the code and not measured: measure each before acting on
it. Line numbers are those of the #57 branch and move.

A contract can check less than the documents say, or throw where they promise a comparison

  1. Private fields declared on a base class are not compared when the pair is written in a
    method of a derived class (T is the derived class; reflection does not return a base class's
    private fields for it). The XML documentation of EnsureAssignable<T>() says so; the four user
    documents say "a base class … compares the members it declares or inherits" and give no
    exception. Old<T>() does copy such a field, which makes the gap harder to guess. No test has
    the pair in a derived-class method.
  2. "Two different Streams are compared member by member." For T = Stream every public
    property is read on both values; ReadTimeout and WriteTimeout throw on a stream without
    timeouts, and Length / Position on one that cannot seek, so the call would end in
    TargetInvocationException, which is neither a pass nor a violation. The two-instance tests use
    a Component, not a stream. Not run.
  3. A class derived from a shared type that is "fixed" (Regex, Type, MemberInfo,
    Assembly, or a class of yours declared in System.Collections.Frozen) with state of its own:
    Old(() => this) returns this, nothing throws, and the pair always passes (a test shows it
    for a class derived from Regex). API_REFERENCE.md gives the advice "check each state member
    with Contract.Ensure" for the first group only; the CHANGELOG's migration text gives it for
    every shared type. The two disagree.

Too strong or too weak

  1. "A dictionary value whose Equals ignores the changed content is reported" (CHANGELOG) holds
    for a class; a value of a value type is settled by Equals first, as README and the API
    reference say for members and elements.
  2. A copied dictionary or set whose keys have no value GetHashCode cannot be looked up by key
    (enumeration works). Measured for array keys during Old shares an array of frozen collections instead of copying it #57 and stated once in the API reference;
    the existing test Old_WhenDictionaryKeyHasIdentityHash_EnumeratesButMissesLookups shows it for
    a key class. [Unreleased] has no entry, although the JSON copy of 2.0.0 rebuilt such a set
    (to be confirmed against 2.0.0).
  3. "The comparers of the base class library" are shared: the code shares CoreLib types that
    implement the non-generic or the string comparer interfaces, which is fewer.
  4. "every … timer" is shared (CHANGELOG): the list holds System.Threading.Timer
    (System.Timers.Timer is shared as a Component); a PeriodicTimer is copied. The API
    reference is exact.
  5. "otherwise its visible fields are compared" for a value type: an ArraySegment<T> and an
    ImmutableArray<T> are compared by what they enumerate. Neither is named in the four files,
    while the CHANGELOG says that for Memory<T> a change outside the window is reported.
  6. A collection class as T (List<T>) is compared by its members, private fields included
    (_items, _size, _version), so adding and removing an element reports a violation that
    names private fields. The only sentence about collections is "supported".
  7. Under Native AOT, T = an interface with no property of its own (IList<int>,
    IEnumerable<int>) holding two arrays: by the code the call reports "cannot compare … no
    properties or fields are visible". The documents name these interfaces as a T under which
    nothing is reported. Not measured.

Smaller

  1. USAGE_EXAMPLES.md lines 21–23 link to #aggregate-root-with-invariants, #value-object and
    #domain-service; no such headings exist.
  2. USAGE_EXAMPLES.md (near line 1206) sets DBC with Environment.SetEnvironmentVariable in a
    test-class constructor; the configuration is read once at process start (README says so), so
    the sample has no effect once any contract call has run.
  3. "shared" is used for "Old<T>() returns the original instance" and for "one object referenced
    from two places"; "pair" for the Old + EnsureAssignable pair and for a pair of arrays;
    "visible" for "through the declared type" and for "to reflection after trimming".
  4. A copy of a finalizable object never runs its finalizer (GC.SuppressFinalize); the
    [Unreleased] entry that lists what does not run leaves the finalizer out.
  5. ADR-0022, the Old + EnsureAssignable never fails in a class derived from a shared type #54 amendment: "every state-holding entry is a class, or a test that only a class
    satisfies" does not hold for a struct named System.Threading.Lock (the lock is matched by
    full name).
  6. The paragraph "An array as T" of API_REFERENCE.md (written for Old shares an array of frozen collections instead of copying it #57) is one very long line
    with six rules; a list would read better.

Suggested handling. Measure items 1–3 first (one test each); they decide whether a document
sentence or the code is to change. Items 1–3 and 9 touch what #40 and #46 discuss.

Activity

  1. cwouyang commented on Oct 11, 2026

    @cwouyang
    OwnerAuthor

    From the review of the last commit of #63 (the limit for an enumerator of a frozen collection that a copied object holds). Wording points in text that #63 adds, left for this card:

    • The advice "when EnsureAssignable<T>() compares the members of the type that holds it, list that member as assignable" (XML remarks of Old<T>(), API_REFERENCE.md, ADR-0022) holds when that type is the T of the call (measured: a pattern that names the member excuses it). For a holder reached through a member of T the patterns are matched against the members of T only, so the member of T has to be listed. Read from the code, not measured.
    • "An enumerator of a frozen collection is a value type declared in System.Collections.Frozen" holds for FrozenSet<T>.Enumerator and FrozenDictionary<TKey, TValue>.Enumerator; other enumerators that a frozen collection hands out through an interface are not all of that kind.
    • API_REFERENCE.md: "the enumerator member with no change gives no exception" stands under "Measured" for a member declared FrozenSet<T>.Enumerator and for one declared IEnumerator<T>; only the first was measured.
    • The XML remarks of Old<T>() do not say that the same change without an enumerator member is not reported (the API reference does).
    • ADR-0022, the limit on an enumerator member: "not measured for this case" is out of date for the advice above (it was measured afterwards for the T of the call), and the decision recorded there carries no date.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions