A writable instance field is exposed to derived types

ID

vbnet.maintainability.avoid_protected_instance_fields

Severity

low

Remediation Complexity

medium

Remediation Risk

medium

Remediation Effort

medium

Resource

Api Design

Language

VB.NET

Description

Reports an instance field declared Protected or Protected Friend that is neither ReadOnly nor Const, with or without an initialiser. Shared fields are not reported here, nor are Private and Friend fields, nor properties. Protected WithEvents is not reported either: that is how the ASP.NET page parser and the WinForms designer bind a declarative control to a field, so the field is required to stay a Protected field and the remediation below does not apply to it.

Rationale

A protected field is part of the contract a base class offers its subclasses, and a writable one offers the weakest contract there is: direct access to storage, with no invariant attached. Every derived type - including types in assemblies you do not control - may assign any value the type system permits, at any point in its lifetime, so the base class cannot state when the field is valid, cannot validate what goes in, and cannot react when it changes. Fields that are meant to move together drift apart, because nothing forces a subclass that sets one to set the other. And the field is written from code the base class never sees, so when its value is wrong there is no setter to breakpoint and the search covers every subclass rather than one accessor. The cost is also paid on the way out. Because the field is inherited surface, replacing it with a property - to add validation, lazy initialisation, change notification, or simply to compute it - is a breaking change for every subclass that reads or writes it, and MyBase.field in an override cannot be intercepted the way a MyBase.Property call can. The longer the base class lives, the more subclasses hold the field directly and the more expensive the eventual fix becomes.

The following code illustrates the pattern detected by this rule:

Public MustInherit Class ReportWriterBase

    ' FLAGGED: A writable instance field is exposed to derived types
    Protected outputPath As String

    ' FLAGGED: A writable instance field is exposed to derived types
    Protected rowsWritten As Integer = 0

    ' FLAGGED: A writable instance field is exposed to derived types
    Protected Friend columns As List(Of String)

Remediation

Expose a protected property instead of the field, keeping the storage Private. The subclass code does not change shape - outputPath becomes OutputPath - but the base class regains control over what may be stored and when. Where subclasses only need to read the value, make the property Protected ReadOnly and take the value in the base constructor; where they need to influence it, prefer a Protected method or a MustOverride/Overridable member that expresses the decision the subclass is really making, rather than a field it is expected to poke. A field that never changes after construction can simply be marked ReadOnly, which is the cheapest fix when it applies.

' Before: any subclass may assign anything, at any time
Public MustInherit Class ReportWriterBase
    Protected outputPath As String
End Class

' After: the base class owns the invariant
Public MustInherit Class ReportWriterBase
    Private _outputPath As String

    Protected Property OutputPath As String
        Get
            Return _outputPath
        End Get
        Set(value As String)
            If String.IsNullOrWhiteSpace(value) Then Throw New ArgumentException(NameOf(value))
            _outputPath = value
        End Set
    End Property
End Class

Configuration

This detector does not need any configuration.

References