New rule: Records should be fully initialized in constructors

[Chadehoc]

Prerequisites

• This rule has not already been suggested.
• This should be a new rule, not an improvement to an existing rule.
• This rule would be generally useful, not specific to my code or setup.

Suggested rule title

RecordsShouldBeFullyInitialized

Rule description

There was discussion in #127 about initializing records through a constructor: should constructors be trusted to properly initialize their results? It was suggested that it should be a separate rule.

This rule would check that even within a constructor, a proper initialization is done by:

• Assigning the result (to Default(T), or a constant, or another initialized variable...)
• Calling another constructor

Calling Initialize isn't enough, because it will only properly initialize managed fields (unless the rule can test that every field in a record is either managed, or a sub-record having recursively the same property).

Rationale

Suppose a record has only two fields, X and Y. The following constructor could be thought harmless:

constructor TMyRecord.Create(AValX: Integer);
begin
  Self.X := AValX;
  Self.Y := 0;
end;

But if you add another field Z later, there is the risk of the constructor adaptation being forgotten.

The following is guaranteed to evolve better:

constructor TMyRecord.Create(AValX: Integer);
begin
  Self := Default(TMyRecord); // or calling another constructor
  Self.X := AValX;
end;

[fourls]

Thanks for the rule proposal!

I think a rule checking if records are initialised during their constructors would be a really solid addition to the default quality profile - it's easy to accidentally write a record constructor that misses a field or two and it would be a good idea to have a rule that catches these cases.

Suppose a record has only two fields, X and Y. The following constructor could be thought harmless:
...
But if you add another field Z later, there is the risk of the constructor adaptation being forgotten.

I disagree with this - I do not think a constructor that simply initialises all fields is bad practice. It works as expected, and because SonarDelphi will raise a new issue if the constructor is not updated, it's not at risk of being left behind.

I feel like assigning to Default(TMyRecord) is a bandaid solution to Delphi not generally having effective static analysis tools that can pick up these cases, rather than something that should be enforced in its own right. Because integrating SonarDelphi into your workflow means that you will be warned if you forget to do something like this, there's no need for us to require users to be overly defensive in their programming.

It will also create a lot of "noise" (issues that are not real problems) that undermines the core usefulness of the rule - that SonarDelphi can actually definitively tell you, without many false positives, whenever a field has not been initialised in a constructor.

To summarise:

• I think a rule requiring a constructor to initialize all record fields (whether by assigning to Default, calling another constructor, or assigning all fields) would be a very useful rule
• I think it would be a good rule for the default quality profile
• I don't think that a record constructor initializing each field separately is bad practice

Interested to hear others' thoughts?

[cirras]

I'm quite enthusiastic about this one, definitely a worthwhile addition.

In terms of feedback...

• I'm agreed on all the feedback offered by @fourls.

• Suggesting a simpler/shorter name: RecordInitialization

• Calling Initialize isn't enough

  You might have misinterpreted my earlier reference to Initialize as talking about the System.Initialize intrinsic.
  Actually, I was talking about the Initialize class operator on Managed Records.

Thoughts?

[Chadehoc]

Because integrating SonarDelphi into your workflow means that you will be warned if you forget to do something like this, there's no need for us to require users to be overly defensive in their programming.

Yes, indeed, if fields can be guaranteed exhaustive, there is no longer a need to initialize to Default. Good for me.

Suggesting a simpler/shorter name: RecordInitialization

RecordInitializationInConstructor? This is specific to constructors, else one does not see why a different rule while there is a VariableInitialization rule.

You might have misinterpreted my earlier reference to Initialize as talking about the System.Initialize intrinsic.

Yes, I was. sorry.

[cirras]

This is specific to constructors, else one does not see why a different rule while there is a VariableInitialization rule.

It's actually not specific to constructors though, as this rule should also target the Initialize operator. 🤔

[fourls]

Hmm, it could be a little confusing to be named so similarly to VariableInitialization.

It's actually not specific to constructors though, as this rule should also target the Initialize operator. 🤔

Although that's true, there's an argument to be made that the Initialize operator is an implicit default constructor.

How about RecordFieldInitialization? Or ExhaustiveRecordConstruction?

[cirras]

How about RecordFieldInitialization? Or ExhaustiveRecordConstruction?

These work. We could look at it from the "describing the problem" angle and call it PartialRecordConstruction.

[Chadehoc]

I would like to add a (possibly optional) subtlety here. I formerly agreed that "if fields can be guaranteed exhaustive, there is no longer a need to initialize to Default." But there is one exception.

The default hasher for non-packed records is wrong when there are "holes" due to field alignment, for example an Integer and an Int64 field. The default hasher uses also the "hole values", which can lead to nasty bugs if using such a record is used as a key dictionnary, for example, because this violates the expected invariant that two values that are Equal should have the same hash.

I would require using full assignment (for example to Default(T)) as the only proper initialization method if the record is not packed and field alignment results "in holes".

[fourls]

That's a very good point - I think this would be good as another separate rule. Conceptually, the motivations are a bit different: the reason for using Default(T) to initialize is about ensuring the entire record's memory is zeroed, while the reason for wanting record fields to be initialized is to ensure that all fields have predictable values.

Maybe we could have:

• A default rule "Records should be fully initialized in constructors" that enforces that all record fields are initialized (whether by assigning to Default, calling another constructor, or assigning all fields)
• A non-default rule "Record memory should be made deterministic in constructors" that enforces that the entire memory of the record has been consistently set (whether by assigning to Default or passing to ZeroMemory or FillChar, or by using a packed record)