No Out Ref Params

ID

csharp.no_out_ref_params

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

api_design, code_smell, parameters

Description

Reports an out or ref parameter declared on a method that code outside the assembly can call — a public or protected member of a type that is itself visible from outside.

Methods that cannot be reached from outside the assembly are not reported, nor is the in modifier, nor out on the bool TryXxx(…​, out T) idiom. The receiver of an extension method (this ref MyStruct self) is not reported either.

Declarations that only restate a signature decided elsewhere are also left alone: an override, and a member matching by name and parameter count a method of an interface the containing type implements. The report belongs on the base or interface declaration, which is reported in its own right when this file declares it. Interfaces are resolved within the compilation unit, so an implementation of an interface declared in another file is still reported.

Finally, a Deconstruct method whose parameters are all out is not reported: that signature is the language’s user-defined deconstruction contract.

Rationale

out and ref hand the callee a pointer to the caller’s own variable. For a value type that is one level of indirection; for a reference type it is a pointer to a pointer, so the callee can swap the object the caller is holding — rarely what the author of the signature intended, and invisible to anybody reading the call site.

The cost lands on callers. They have to declare a variable before calling, initialise it for a ref, repeat the modifier at the call site, and keep track of which arguments come back changed. None of that composes: an out argument cannot be a property, cannot be the result of an expression, and cannot flow through a lambda or a query.

A method that has more than one thing to say should say it in its return type, where the names are part of the contract and the value can be passed on unchanged.

public class Geometry
{
    public void Split(string path, out string dir, out string file)   // FLAW
    {
        dir = "";
        file = path;
    }

    public (string Dir, string File) SplitPath(string path)           // OK, both values in the return type
        => ("", path);

    public static bool TryParsePoint(string text, out int x)          // OK, the bool TryXxx idiom
    {
        x = 0;
        return false;
    }

    internal void Scale(ref double factor) { }                        // OK, not visible outside the assembly

    public double Distance(in double from, in double to)              // OK, 'in' is read-only
        => to - from;

    public void Deconstruct(out int x, out int y)                     // OK, the deconstruction idiom
    {
        x = 0;
        y = 0;
    }
}

public interface IReader
{
    void Read(out string text);                                       // FLAW, this is where the signature is decided
}

public class FileReader : IReader
{
    public void Read(out string text) => text = "";                   // OK, the interface fixes the signature
}

Remediation

Return the values instead of writing them into the caller’s variables: a tuple with named elements for two or three results, a small purpose-built type when the group has meaning of its own, or a single object the method fills in. When the intent was only to avoid copying a large value type, in expresses that without letting data flow back.

Keep an out parameter when the method is a bool TryXxx that reports failure without throwing, or when it is the all-out Deconstruct the compiler binds deconstructing assignments to — those are the shapes callers already expect. When the parameter comes from a base class or an interface, fix it there: changing only the override or the implementation would not compile.

References