Static analysis on a codebase that has never had any

The bug that started this was a method renamed in one place and called by its old name in another, on a path the test suite did not reach. It shipped, it was obvious on reading, and nothing in the pipeline had any opinion about it. PHPStan finds that class of thing without running the code — and on a codebase that has never been analysed, its first report is 4,000 errors long.

The symptom

$ vendor/bin/phpstan analyse --level=max src

 [ERROR] Found 4118 errors

  ------ ---------------------------------------------------------------
   Line   src/Billing/Invoice.php
  ------ ---------------------------------------------------------------
   88     Call to an undefined method AppModelsOrder::totalWithTax()
   112    Parameter #1 $amount of method add() expects Money, float given
   140    Access to an undefined property AppModelsOrder::$vat_rate
  ------ ---------------------------------------------------------------

Somewhere in those 4,118 lines are perhaps twenty real bugs. The rest is the analyser being correct about a codebase written before anyone was checking, and the difference is not visible from the summary.

Why it happens

PHP resolves almost everything at the moment a line executes. A method that does not exist is not an error until it is called, a property that was never declared springs into existence on assignment, and a docblock is a comment. So the only mechanism that has ever checked any of this is the test suite, and a suite covers the paths somebody thought to cover.

The analyser reads every path regardless of coverage, which is why it finds things and why it finds so many: it is applying a standard to code written without one.

The fix

Start at level 0 and mean it

# phpstan.neon
parameters:
    level: 0
    paths:
        - src

# 0  unknown classes, unknown functions, wrong argument counts
# 1  undefined variables, unknown magic methods
# 2  unknown methods on all expressions
# 3  return types, property assignment types
# 4  dead code, always-false conditions
# 5  argument types passed to methods
# 6  missing type hints
# 7  partially wrong union types

$ vendor/bin/phpstan analyse
 [ERROR] Found 61 errors

Sixty-one is a number somebody can work through in an afternoon, and level 0 is the level where almost every finding is a genuine defect — a class that does not exist, a function removed two years ago, a call with the wrong number of arguments. Nothing at level 0 is a style opinion.

Ignore what will not be fixed, by pattern

Some of the sixty-one were not fixable. An ORM generating scope methods at runtime produces “undefined method” for every one of them, and the analyser is right that the method does not exist in any file.

parameters:
    level: 0
    paths: [src]
    ignoreErrors:
        # magic scopes on Eloquent models
        - '#Call to an undefined method [A-Za-z\\]+::(where|scope)[A-Za-z]+()#'
        # one directory, until it is rewritten
        - message: '#Access to an undefined property#'
          path: src/Legacy/*

The other lever is a stub file: telling the analyser what a dynamically-created method looks like, rather than ignoring the error. It is more work and it is the honest version, because the next person reading the code gets the same information the analyser does.

The errors worth fixing first

Of the sixty-one, three categories mattered and the rest were noise the ignore patterns absorbed.

// 1. A method that genuinely does not exist — the bug that started this.
$order->totalWithTax();          // renamed to totalIncludingTax() in November

// 2. A function removed by a dependency upgrade, on an error path.
$this->logger->addRecord(...);   // gone in monolog 1.23

// 3. An argument count that has been wrong since it was written.
format_money($cents);            // signature: (int $cents, string $currency)

The third had been passing a null currency for eighteen months, defaulting to the site currency, and had never been noticed because the site had one currency. It would have been a bug the week a second one was added.

A gate, not a wall

Making it a CI failure at level 0 was uncontroversial because the count was zero by then. Making it useful means raising the level, and raising the level on a schedule that the team sets rather than a build that suddenly fails.

analyse:
  stage: lint
  script:
    - composer install --prefer-dist --no-interaction
    - vendor/bin/phpstan analyse --no-progress --error-format=table
  # fails the build. the level in phpstan.neon is the contract.

# and for a branch, only what changed — fast enough to run on save
$ git diff --name-only --diff-filter=ACMR origin/master...HEAD -- '*.php' 
>   | xargs -r vendor/bin/phpstan analyse --level=5 --no-progress

Analysing changed files at a higher level than the project floor is the trick that makes progress without a big-bang commit: new code is held to level 5 while the codebase as a whole is at level 2. The floor rises when someone has time, in a commit containing nothing else.

Climbing

# level 0 → 61   (fixed in an afternoon)
# level 1 → 88   (mostly undefined variables in old templates)
# level 2 → 214
# level 3 → 502  (return types — the first level that needs annotations)
# level 4 → 611
# level 5 → 1,204  (argument types; where the real bugs are)
# level 6 → 3,880  (missing type hints; a project, not a task)

The jump at level 3 is where it stops being free, because it requires writing down types nobody had written down. Level 5 is where the second real crop of bugs appeared — six places passing a string where an object was expected, all on error paths. Level 6 was left for later and is still later.

Verifying it worked

The proof is not the error count going down; it is a defect caught before it shipped that the suite did not catch.

$ git push

lint      FAILED  00:34
  src/Shipping/ZoneResolver.php
   47  Parameter #2 $country of method rateFor() expects Country,
       string given.

test      skipped
build     skipped

That change had a test, and the test passed — it exercised the domestic path, where the parameter was never reached. The analyser did not need a test to see it.

The extension that pays for itself immediately

Analysing the test suite is usually skipped, and it is where a surprising share of the findings are — a mock configured against a method that was renamed, an assertion on a property that no longer exists. Without the PHPUnit extension it is unusable, because every mock is an opaque MockObject and every call on one is an error.

parameters:
    level: 5
    paths:
        - src
        - tests

includes:
    - vendor/phpstan/phpstan-phpunit/extension.neon

With it, the analyser understands that createMock(Gateway::class) returns something that is both a MockObject and a Gateway, and it checks that every mocked method exists on the real class. That single rule found eleven tests configuring mocks against methods that had been renamed — tests that passed, because a mock will happily stub anything you name.

What it does not find

Every error in this article is a shape mistake — a name, a type, an argument count. None of them is a wrong calculation, a missing authorisation check, an off-by-one, or a race. It is worth being explicit about that internally, because a team that has just adopted static analysis will reasonably assume the safety net is wider than it is.

// PHPStan is entirely happy with all three
public function total(): int
{
    return $this->net + $this->tax;      // should be net + tax + shipping
}

public function delete(Order $order): void
{
    $this->orders->remove($order);      // no capability check
}

foreach (range(0, count($rows)) as $i) { /* off by one */ }

The correct framing is that it raises the floor. The class of bug it removes is the one that used to be found by a user, at a cost of an incident, and is now found in nine seconds by a machine. Everything above that floor is still the test suite’s job, and the suite has not become less important — it has stopped being the only thing checking anything.

Where the levels stop being free

Levels 0 to 2 cost nothing but time: every finding is a name that does not resolve, and fixing it improves the code with no debate. Level 3 is the first that asks for something the codebase does not contain, and the character of the work changes with it.

// level 3 wants to know what this returns, and the docblock lies
/**
 * @return array
 */
public function summarise(): array
{
    return ['sku' => $this->sku, 'price' => $this->price->cents()];
}

// so either annotate the shape
/**
 * @return array{sku: string, price: int}
 */

// or admit it wanted to be an object
public function summarise(): ProductSummary

Both are more work than the error, and the second is a design change smuggled in by a linter — which is a legitimate thing to resist on a Tuesday afternoon. The pattern worth noticing is that the annotations the analyser asks for are usually describing a structure that has outgrown being an array, so the errors are pointing at something real even when the fix is a docblock.

Level 6 demands a type hint on every parameter and return, and on a codebase of this age that is 3,880 errors representing months rather than days. It was left, and it is still left. Deciding out loud that a level is not worth reaching is a better outcome than a project that stalls at level 4 while everyone assumes it is heading for 8.

Keeping the ignore list from becoming permanent

Twenty-two patterns went in during the first week and nothing removes them. A year later nobody knows which are still needed, and a broad one is silencing errors that would now be worth seeing.

# phpstan reports patterns that no longer match anything
$ vendor/bin/phpstan analyse --no-progress 2>&1 | grep 'was not matched'

  Ignored error pattern #Access to an undefined property# in path
  src/Legacy/* was not matched in reported errors.
  Ignored error pattern #Call to an undefined method .*::findByCode# 
  was not matched in reported errors.

The unmatched-pattern report is on by default and easy to miss in the output. It is the whole maintenance mechanism: a pattern that matches nothing is a line to delete, and running through them quarterly takes twenty minutes. Without it the list only grows, and a list that only grows is a way of not adopting the tool while appearing to have done.

What this costs

Above level 3 the tool asks for annotations describing things the engine cannot know, and some of them are genuinely awkward — an array shape, a generic collection, a callable signature. Writing them is work, and arguing about whether a particular one is worth it is a recurring cost that does not appear in any estimate.

There is also a failure mode specific to a codebase that adopts this enthusiastically: the ignore list becomes a place to put anything inconvenient, and after a year it is 200 patterns long and nobody knows which are still needed. Reviewing it quarterly and deleting entries that no longer match takes twenty minutes and is the difference between a tool and a formality.

And it finds nothing about behaviour. Every error in this article is a shape mistake; none of them is a wrong calculation, a missing authorisation check or a race. Static analysis raises the floor and the tests are still the ceiling.