PropertiesMustHavePrivateSetters convention passes for get-only properties

[gkinsman]

Perhaps a side-effect of the introduction of get-only properties in some version of C# released since this library was a thing. This convention is slightly ambiguous as it asserts that properties must have private setters - implying a setter should exist - but in fact passes when no setter exists.

Unsure of a good way forward:

1. Expected behaviour?
2. Change it to fail when no setter exists - likely a big impact
3. Add a new convention that ensures a private setter exists?

The reason for this is that PropertyInfo.CanWrite is false for get-only properties. I've created my own called PropertiesMustHaveSettersThatArePrivate that omits the CanWrite check :).

[andrewabest]

👋 howdy George!

This convention was created to enforce proper encapsulation in domain objects originally to ensure aggregates were appropriately enforcing their invariants.

As you've mentioned, CanWrite will be false for get-only properties, which also tells us that the logic in the line that identifies failures is also suspect, because assumedly subject.GetSetMethod(true) == null will never be true for the types we are inspecting.

I suspect the correct way to go about this would be to modify this to be more specific and ensure all properties on a given type have private setters, and then if you wanted to have a type that could have get only properties or private setters, you'd use

types
    .MustConformTo(
        Convention.PropertiesMustBeImmutable.Or(
        Convention.PropertiesMustHavePrivateSetters));

But as you've said, it would be a large breaking change.

I'll need to think this one over a little more to think about how to move forward with it - I might reserve this change for whatever the next 'major' release might be, as this is one of the more fundamental conventions that a lot of people will be utilizing.

[gkinsman]

Hey Andrew! Thanks for your reply 🙂.

I can easily appreciate that the naming of these conventions is not easy - the balance between usability and semantic correctness is a fine one.

I wonder if there's value in being specific with convention names - thinking about PropertiesMustBeImmutable compared with PropertiesMustNotHaveSetters - the former is correct, but the latter feels, dare I say it, more conventional. It seems possible that the meaning of "immutable" could be made ambiguous in future C# releases (with e.g. record types).

Maybe a non-breaking-change route could be to obsolete the existing convention, and introduce two new ones named as you suggest, though I couldn't say what a good rename for the PropertiesMustHavePrivateSetters would be.

[andrewabest]

I agree with tightening up the naming to remove ambiguity - MustNotHAveSetters is a lot better than MustBeImmutable, as that can imply any number of things for an object depending on language features, and could change over time with the language spec as you've mentioned.

I also wonder if there is value in a rule of 'conventions must only do one thing' - i.e. if you want multiple functions within a convention, you should be .And() or .Or() -ing together specific conventions to form an aggregate convention.

I don't think I'll go the Obsolete route, I think a breaking change with a major version bump is the way to go.

Step one is going to be establishing some more clear convention composition and naming rules for conventions, and then reviewing the existing conventions and modifying as appropriate for conformance (or at least documenting where non-conformance makes sense).

I'll use this issue to do that, but might not get to it for a few weeks, as I have a VS 2019 launch to prepare for among other things :)

[andrewabest]

@gkinsman I thought I'd do the first cut for this one - it is really just the discussed rename, but I've taken the time to abstract the commonality from all of the property conventions into a new base type.

One thing that worries me a little is the coverage of the tests around these conventions - I'm not convinced it was good enough given the hole discovered initially. I'm going to review those before I merge this - if you could eyeball them as well (if you have time), it would be much appreciated!

[andrewabest]

Alright I've reviewed the tests and made a few more adjustments in that PR. The conventions are now a lot more explicit, and I'm pretty happy with the changes. @gkinsman I'll merge at the end of this week, if you have any feedback before then I'm all ears :)

[andrewabest]

Resolved with #65