GH-75: Document and cover the defensive guards, rather than delete them (#121)

* GH-75: Document the defensive guards the issue called dead, and cover four

The issue lists five "dead defensive code paths" to remove. Measured, none
is dead - each is a guard that is unreachable through the value converter's
CHAIN but live for another entry, and removing them would drop real
protection:

- The two null guards (builtin, object trait) throw for a null on a
  non-nullable target. Through the chain NullValueConversionStrategy claims
  every null first, so they never run there - but the strategy classes are
  public SPI, and a direct convert() call reaches them. Proven: a direct
  builtin convert(null, int) throws TypeMismatchException, as it should.
- The convertObjectValue non-object branch hands the value back for a type
  its own supports() would reject - reached only by a direct call skipping
  supports(). Proven the same way.
- The ValueConverter LogicException is unreachable while the passthrough
  strategy (supports() always true, last) is registered, and guards the
  invariant if that ever changes - otherwise the method would fall off the
  end returning null.
- The "collection class must be provided" throw is not a runtime guard at
  all but a load-bearing assertion: it narrows ?string to string for the
  calls below, and PHPStan max fails without it. Removing it, verified,
  breaks static analysis.

So the resolution is the acceptance criterion's "documented reason", not
removal: each branch now says why it cannot occur on the normal path and
why it is kept. The four SPI/invariant guards also gain tests that drive
them directly, turning "documented dead branch" into "live, covered
branch" - the criterion's preferred outcome for the null guards.

The architecture audit on the issue asked that the null guards not be
deleted before the two-entry-points fix (#87, still open), since a single
entry point may make them live through the chain too. Documenting and
covering rather than deleting respects that: nothing here has to be
revisited when #87 lands, and if #87 makes a guard live on the chain, its
test simply gains a second caller.

* GH-75: Cover the object null guard, drop the misuse-contract test, pin the message

Review found my commit message claimed to cover four guards but only three
tests existed - and the missing one was exactly the object-trait null
guard. Removing its throw made convertObjectValue return null for a
non-nullable object target with the whole suite still green, the precise
regression this was meant to prevent. Added theObjectGuardRefusesANull...,
verified by mutation: neutralising the throw now fails it.

Dropped theObjectGuardHandsBackAValueForANonObjectType. It asserted the
exact silent-passthrough the object trait does for a type it does not
support - but the SPI interface promises nothing about convert() on an
unsupported type, so the test froze a misuse-only implementation detail as
contract, and a future fail-fast refactor would break it for no real
reason. The branch keeps its documented reason; it is defensive against a
protocol violation, which the acceptance criterion covers by documentation
rather than a test.

And the builtin guard test asserted only the exception type, which several
sites in that strategy throw. It now matches the message, so it pins this
guard rather than any TypeMismatchException.

Author: magicsunday

src/JsonMapper.php

@@ -483,6 +483,11 @@ private function mapCollection(
         $isGenericCollectionMapping = $resolvedClassName === null && $collectionValueType instanceof Type;
 
         if ($isGenericCollectionMapping) {
+            // Not reachable at runtime: extractCollectionType() returns a Type only when the
+            // collection class is set, so $collectionValueType being one already implies a non-null
+            // collection class here. Kept because it also narrows ?string to string for the calls
+            // below - PHPStan max fails without it - so it is a load-bearing assertion, not merely
+            // a runtime guard.
             if ($resolvedCollectionClassName === null) {
                 throw new InvalidArgumentException(
                     'A collection class name must be provided when mapping without an element class.'

src/JsonMapper/Value/Strategy/BuiltinValueConversionStrategy.php

@@ -245,6 +245,11 @@ private function guardCompatibility(mixed $value, BuiltinType $type, MappingCont
                 return;
             }
 
+            // Reached only by a DIRECT call to convert() on this strategy. Through the value
+            // converter's chain a null never arrives here, because NullValueConversionStrategy is
+            // registered first and claims every null - but the strategy classes are public SPI, so
+            // this defends the case where one is invoked outside the chain.
+            //
             // Throw rather than record-and-continue: recording it here and returning anyway
             // leaves the caller to store a null the declared type forbids. On a property that
             // means a value contradicting its own docblock; in a collection it means the offending

src/JsonMapper/Value/Strategy/ObjectTypeConversionGuardTrait.php

@@ -60,6 +60,10 @@ private function guardNullableValue(mixed $value, ObjectType $type, MappingConte
             return;
         }
 
+        // Reached only by a DIRECT call to a strategy using this trait. Through the value
+        // converter's chain a null is claimed by NullValueConversionStrategy first, so it never
+        // arrives; the guard defends the public-SPI case where a strategy is invoked outside the
+        // chain, keeping a null off a non-nullable object target.
         throw new TypeMismatchException($context->getPath(), $type->getClassName(), 'null');
     }
 
@@ -77,6 +81,9 @@ private function convertObjectValue(Type $type, MappingContext $context, mixed $
     {
         $objectType = $this->extractObjectType($type);
 
+        // Unreachable through the chain: supports() returns false for a non-object type, so convert()
+        // is not called for one. Kept for a DIRECT call that skips supports() - it hands the value
+        // back untouched rather than dereferencing a null object type.
         if ($objectType === null) {
             return $value;
         }

src/JsonMapper/Value/ValueConverter.php

@@ -57,6 +57,10 @@ public function convert(Type $type, mixed $value, MappingContext $context): mixe
             }
         }
 
+        // Unreachable while PassthroughValueConversionStrategy is registered: its supports()
+        // always returns true and it is added last, so the loop always finds a strategy. Kept as an
+        // invariant guard - a future change that removed or misordered the passthrough would
+        // otherwise fall off the end of this method returning null implicitly.
         throw new LogicException(
             sprintf('No conversion strategy available for type %s.', $type::class)
         );

tests/JsonMapper/Value/StrategyDirectInvocationTest.php

@@ -0,0 +1,92 @@
+<?php
+
+/**
+ * This file is part of the package magicsunday/jsonmapper.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace MagicSunday\Test\JsonMapper\Value;
+
+use DateTime;
+use LogicException;
+use MagicSunday\JsonMapper\Context\MappingContext;
+use MagicSunday\JsonMapper\Exception\TypeMismatchException;
+use MagicSunday\JsonMapper\Value\Strategy\BuiltinValueConversionStrategy;
+use MagicSunday\JsonMapper\Value\Strategy\DateTimeValueConversionStrategy;
+use MagicSunday\JsonMapper\Value\Strategy\ValueConversionStrategyInterface;
+use MagicSunday\JsonMapper\Value\ValueConverter;
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\TypeInfo\Type;
+use Symfony\Component\TypeInfo\Type\BuiltinType;
+use Symfony\Component\TypeInfo\Type\ObjectType;
+use Symfony\Component\TypeInfo\TypeIdentifier;
+
+/**
+ * The strategy classes are public SPI: a caller may register its own and, in doing so, may invoke a
+ * strategy directly rather than through the value converter's chain. Several guards inside them are
+ * unreachable through that chain - NullValueConversionStrategy claims every null before they run -
+ * but not dead: they defend exactly the direct-invocation case. These pin that they still hold, so
+ * a future "this branch is never hit" cleanup meets a red test rather than a silent contract loss.
+ *
+ * @internal
+ */
+final class StrategyDirectInvocationTest extends TestCase
+{
+    #[Test]
+    public function theBuiltinStrategyRefusesANullForANonNullableType(): void
+    {
+        $strategy = new BuiltinValueConversionStrategy();
+
+        // The message pins THIS guard: TypeMismatchException is thrown from several sites in the
+        // strategy, and a future refactor routing null down a different one would otherwise keep
+        // this test green while covering the wrong branch. Matches() not Message() - the latter is
+        // deprecated across the PHPUnit majors the constraint spans.
+        $this->expectException(TypeMismatchException::class);
+        $this->expectExceptionMessageMatches('/expected int, got null/');
+
+        $strategy->convert(new BuiltinType(TypeIdentifier::INT), null, new MappingContext([]));
+    }
+
+    #[Test]
+    public function theObjectGuardRefusesANullForANonNullableType(): void
+    {
+        // The object-trait counterpart of the builtin guard above, and the one the first round of
+        // tests missed. Through the chain a null is claimed by NullValueConversionStrategy first;
+        // a direct call reaches guardNullableValue, which must keep a null off a non-nullable
+        // object target rather than let convertObjectValue return it.
+        $strategy = new DateTimeValueConversionStrategy();
+
+        $this->expectException(TypeMismatchException::class);
+
+        $strategy->convert(new ObjectType(DateTime::class), null, new MappingContext([]));
+    }
+
+    #[Test]
+    public function theConverterRaisesWhenNoStrategyMatches(): void
+    {
+        // The invariant guard: with no passthrough registered and a type nothing supports, the loop
+        // finds no strategy. In production PassthroughValueConversionStrategy (supports() always
+        // true, registered last) makes this unreachable - this drives it directly.
+        $converter = new ValueConverter();
+        $converter->addStrategy(new class implements ValueConversionStrategyInterface {
+            public function supports(Type $type, mixed $value, MappingContext $context): bool
+            {
+                return false;
+            }
+
+            public function convert(Type $type, mixed $value, MappingContext $context): mixed
+            {
+                return $value;
+            }
+        });
+
+        $this->expectException(LogicException::class);
+
+        $converter->convert(new BuiltinType(TypeIdentifier::INT), 'x', new MappingContext([]));
+    }
+}