Replace inline docblock declarations with assertions #47

Ocramius · 2018-03-28T16:35:10Z

This RFC proposes replacing the typical /** @var Something $something */ with assert($something instanceof Something) wherever possible.

This has a few nice side-effects that @DASPRiD exposed:

the type declaration is no longer a comment, but AST nodes that can be inspected and have well defined meaning in the language itself
assert() can report when these assumptions fail (once the CS rule is applied to a codebase)
assert() can be turned off, making this an informational addition

I didn't dig into PHPStan's new type declarations, which support foo[], (foo|bar)[] and union types (foo&bar) too much, so this is just a first sketch of the idea I stole from @DASPRiD.

Ocramius · 2018-03-28T16:36:10Z

tests/input/inline_type_hint_assertions.php

+        /** @var Type1|Type2 $unionType */
+        $unionType = expression();
+
+        /** @var Type1&Type2 $intersectionType */


Unsure if we should deal with this now. I can see it growing in usage since it was first introduced a couple months ago, so it may be worth exploring after poking the @phpstorm folks

Issue for intersection types is pending and being ignored: https://youtrack.jetbrains.com/issue/WI-39419 for three months already. :(
Unfortunately until then, it's a no-go, it completely breaks assumptions in PhpStorm.

cc @JanTvrdik

Is that one actually a problem?

assert($intersectionType instanceof Type1 && $intersectionType instacenof Type2);

The problem here is that contemporary tooling does not support this except for phpstan.

@morozov the CS tool simply needs to learn to parse them in order to replace them. These annotations are still valid in docblocks.

@muglug awesome! Now we just need to convince JetBrains 😂

These annotations are still valid in docblocks.

Do you also want to replicate complex argument types with assertions? Otherwise, unlike local variables, they remain unenforced. For instance:

/** * @param string|int $a */ function ($a) { assert(is_string($a) || is_int($a)); }

Might be interesting, but unlikely for this PR

I'm not too sure about how I feel about assertions for public API parameters. Personally of course I always avoid complex types in public APIs and only use them in private APIs when I want to keep things simple.

The thing with assertions is, that they are not ran by default (heck, PHP throws away the assertion op-codes when assertions are disabled). Thus most users wouldn't get those checks. IMHO a public API should do parameter checks the proper way (and throw exceptions itself) for such stuff.

Assertions come in handy when you have methods which are supposed to return something specific, but the API cannot specify the return type (like PSR containers). Thus you assert on the return type.

morozov · 2018-03-28T18:47:13Z

tests/input/inline_type_hint_assertions.php

+        /** @var int|float|bool|string|null|array $multipleScalarTypes */
+        $multipleScalarTypes = expression();
+
+        /** @var Potato $variableThatIsNowhereToBeFound */


Is it for the cases like extract() or eval()? If the standard doesn't allow them (and why would it?), then this case doesn't need to be handled.

Replacing an inline definition with an assertion still challenges the assumption, which is good

carusogabriel · 2018-03-28T19:38:08Z

tests/fixed/inline_type_hint_assertions.php

+        assert($nullableType instanceof Type || $nullableType === null);
+
+        $multipleScalarTypes = expression();
+        assert(is_int($multipleScalarTypes) || is_float($multipleScalarTypes) || is_bool($multipleScalarTypes) || is_string($multipleScalarTypes) || $multipleScalarTypes === null || is_array($multipleScalarTypes));


What about assert(in_array(gettype($multipleScalarTypes), ['int', 'float', 'bool', 'string', 'null', 'array'], true)), isn't a more readable/cheaper way?

I think it's harder to parse for the IDE to infer the type.

Indeed: let's keep it to very well known constructs

mikeSimonson · 2018-03-28T20:08:29Z

@Ocramius I like the idea but I don't what's better, holding off the part that are not supported by phpstorm or going all in and see how it goes.

Ocramius · 2018-03-28T20:16:50Z

holding off the part that are not supported by phpstorm or going all in and see how it goes.

Yeah, I think this will be the initial approach

Majkl578 · 2018-03-28T20:50:57Z

$ git grep -E '/\*\*\s+@var\s+[^\|]+\s+\$' lib tests | wc -l

There is currently 174 affected non-compound occurrences in ORM.

 git grep -E '/\*\*\s+@var\s+\S+\s+\$' lib tests | wc -l

There is currently 249 afftected compound or non-compound occurrences in ORM.

This may (or may not) have serious impact.

assert() can be turned off, making this an informational addition

This assumption is not valid, since you can't turn it off per-library or so. There are projects out there that build business asserts using assert().

DASPRiD · 2018-03-28T20:54:59Z

Having business logic depend on assertions is a seriously bad practice, and they should feel bad.

Ocramius · 2018-03-28T20:55:39Z

@Majkl578 we'd obviously have to turn assertions on in the test suite.

Majkl578 · 2018-03-28T20:56:07Z

@DASPRiD Not really. Please consult your opinion with beberlei/assert and webmozart/assert.

Ocramius · 2018-03-28T20:58:22Z

Those two libs are not at all matching assert() from PHP. I use them extensively in business logic precisely because you can't turn them off.

Ocramius · 2018-03-28T20:59:43Z

Strictly related: https://www.reddit.com/r/PHP/comments/7yiaib/comment/duhbahh

Majkl578 · 2018-03-28T21:06:26Z

I can easily use assert() for the same purpose (and avoid method call overheads) with simple pre-condition: asserts are always enabled.
This pre-condition is totally ok for a project, but absolute no-go for a library.

Ocramius · 2018-03-28T21:12:47Z

Indeed, but (assuming th docblocks are correct) whether the assertions are enabled or disabled makes no functional difference with the proposed patch approach

Majkl578 · 2018-03-28T21:19:57Z

Not functional, but could have performance impact, depending on the amount & # of uses of course.

Majkl578 · 2018-03-28T21:22:23Z

In other words, simply adding 250 assert()s just because you don't like phpDoc is not enough to justify that.
Argument 1. is functionally irrelevant.
Argument 2. is valid, but you can achieve the same with SA for @var.
Argument 3. is where I disagree most (see above).

@var

Putting `/** @var foo $foo */` after `$foo = bar();` is now a fixable violation as per slevomat/coding-standard@63a8186 Thanks @kukulich! Meanwhile, also pinning down `slevomat/coding-standard` to the specific commit that includes all required changes for this test to run. Ref: f797ff6#commitcomment-33331234

Ocramius · 2019-04-29T08:35:41Z

composer.json

@@ -28,7 +28,7 @@
    "require": {
        "php": "^7.1",
        "dealerdirect/phpcodesniffer-composer-installer": "^0.5.0",
-        "slevomat/coding-standard": "^5.0",
+        "slevomat/coding-standard": "dev-master#63a8186b129ee96d1277e68c80cf87c8cdb356d1",


@malarzm @deeky666 I'm likely raising a separate PR for stable slevomat release pinning by @kukulich.

IIRC pinning a commit could lead to weird side effects where we can end up with vendor requirements from current dev-master while code in pinned commit could require something else.

Yes: this is not for a stable release yet. I'd need a SemVer selector for the release to be tagged.

Ocramius · 2019-04-29T08:43:03Z

@doctrine/coding-standard-approvers please shoot a 👍 or 👎 my way.

As mentioned in #47 (comment), I'll pin to a stable release after this (chore/release work).

alcaeus · 2019-04-29T08:46:01Z

👍 on the rule itself. Not sure if we want to merge with commit pinning in composer.json, I'll defer to others for that decision.

SenseException · 2019-04-29T16:35:01Z

lib/Doctrine/ruleset.xml

@@ -311,6 +311,8 @@
    <rule ref="SlevomatCodingStandard.PHP.UselessParentheses"/>
    <!-- Forbid useless semicolon `;` -->
    <rule ref="SlevomatCodingStandard.PHP.UselessSemicolon"/>
+    <!-- Require /* @var type $foo */ and similar simple inline annotations to be replaced by assert() -->


Does this sniff support /* @var ArrayIterator $iterator */ with one * too like in this comment? IDEs like PHPStorm support those too, even though it doesn't autocomplete on "/*". In case it supports the single-*, some tests for those would be nice.

It does not. However it is reported (and fixed) by

coding-standard/lib/Doctrine/ruleset.xml

Line 200 in e008cac

<rule ref="SlevomatCodingStandard.Commenting.InlineDocCommentDeclaration"/>

Is it possible that both can be used? The first one fixing a /* and the other one handles it for the assert()?

A CS rule enforcing /** style would be preferable: please note that current static analysis tools fail to parse /* AFAIK

InvalidDocCommentDeclaration fixes /* to /** in first round and in second round RequireExplictionAssertion changes doccomment to assertion :)

A CS rule enforcing /** style would be preferable: please note that current static analysis tools fail to parse /* AFAIK

Last time I used NetBeans (granted, a while ago), it only supported vardoc syntax (/*) but not /**.

That changed a long time ago: at the moment, the ecosystem is pushing for /**, and enabling that sniff could put a tombstone on this

@Ocramius Not sure I undrestand you well but the InvalidInlineDocCommentDeclaration is already enabled ;)

@kukulich yeah, this was mostly for @DASPRiD 👍

lcobucci

I don't mind the commit pinning on composer.json, we just have to remember that we shouldn't release anything before removing it.

Thanks @kukulich!

ostrolucky

Why was review re-requested from me when nothing changed?

SenseException

A usage of SlevomatCodingStandard.Commenting.InlineDocCommentDeclaration can be discussed another time.

Dismissing current feedback due to approvals quota reached

lcobucci · 2019-04-30T14:47:10Z

Why was review re-requested from me when nothing changed?

@ostrolucky my bad (or maybe GH UI issues), sorry. I was asking re-reviews from people that requested changes but didn't look at it again, interface moved and I miss clicked 😓

ostrolucky · 2019-04-30T14:48:42Z

No worries I just wanted to know. Explanation makes sense :)

Ocramius added RFC New sniff labels Mar 28, 2018

Ocramius requested review from morozov, jwage, Majkl578, lcobucci, guilhermeblanco, jmikola, alcaeus, kimhemsoe, mikeSimonson, deeky666, malarzm and carusogabriel March 28, 2018 16:35

Ocramius commented Mar 28, 2018

View reviewed changes

morozov reviewed Mar 28, 2018

View reviewed changes

carusogabriel reviewed Mar 28, 2018

View reviewed changes

morozov approved these changes Mar 28, 2018

View reviewed changes

Ocramius force-pushed the feature/assertions-over-inline-type-docs branch from 40cae4d to c8e32d3 Compare April 29, 2019 08:34

Ocramius commented Apr 29, 2019

View reviewed changes

ostrolucky approved these changes Apr 29, 2019

View reviewed changes

jwage approved these changes Apr 29, 2019

View reviewed changes

SenseException reviewed Apr 29, 2019

View reviewed changes

lcobucci requested review from morozov and ostrolucky April 30, 2019 08:11

lcobucci approved these changes Apr 30, 2019

View reviewed changes

ostrolucky approved these changes Apr 30, 2019

View reviewed changes

SenseException approved these changes Apr 30, 2019

View reviewed changes

malarzm approved these changes Apr 30, 2019

View reviewed changes

Ocramius merged commit 89a1294 into master Apr 30, 2019

Ocramius deleted the feature/assertions-over-inline-type-docs branch April 30, 2019 14:24

Ocramius self-assigned this Apr 30, 2019

Ocramius added this to the 7.0.0 milestone Apr 30, 2019

lcobucci removed the On hold label Aug 14, 2019

lcobucci mentioned this pull request Aug 14, 2019

Guard L2C regions against corrupted data doctrine/orm#7778

Merged

Ocramius mentioned this pull request Dec 9, 2019

mixed assignment: allow it when immediately followed by an assertion? vimeo/psalm#2442

Closed

ostrolucky mentioned this pull request Jan 31, 2020

[Contracts/Deprecation] Provide a generic function and convention to trigger deprecation notices symfony/symfony#35526

Merged

MP-okui mentioned this pull request Feb 5, 2021

@var inline docblockの代わりにassert()を使用する MP-okui/php-coding-standard#14

Open

jrmajor mentioned this pull request Feb 17, 2022

DX: Narrow docblock types in Tokenizer PHP-CS-Fixer/PHP-CS-Fixer#6293

Merged

simPod mentioned this pull request Sep 22, 2022

feat(explicit assertions): enable advanced string types and integer ranges #293

Merged

Slamdunk mentioned this pull request Oct 27, 2022

Updates description for laminas-mvc application integration to simplify the description and usage laminas/laminas-form#195

Merged

Replace inline docblock declarations with assertions #47

Replace inline docblock declarations with assertions #47

Conversation

Ocramius commented Mar 28, 2018 • edited by carusogabriel Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Mar 28, 2018 • edited by carusogabriel Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeSimonson commented Mar 28, 2018

Ocramius commented Mar 28, 2018

Majkl578 commented Mar 28, 2018

DASPRiD commented Mar 28, 2018

Ocramius commented Mar 28, 2018

Majkl578 commented Mar 28, 2018 • edited Loading

Ocramius commented Mar 28, 2018

Ocramius commented Mar 28, 2018

Majkl578 commented Mar 28, 2018

Ocramius commented Mar 28, 2018

Majkl578 commented Mar 28, 2018

Majkl578 commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Apr 29, 2019

alcaeus commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DASPRiD Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci left a comment

Choose a reason for hiding this comment

ostrolucky left a comment

Choose a reason for hiding this comment

SenseException left a comment

Choose a reason for hiding this comment

lcobucci commented Apr 30, 2019

ostrolucky commented Apr 30, 2019

Ocramius commented Mar 28, 2018 •

edited by carusogabriel

Loading

morozov Mar 28, 2018 •

edited by carusogabriel

Loading

morozov Mar 28, 2018 •

edited

Loading

Majkl578 commented Mar 28, 2018 •

edited

Loading

DASPRiD Apr 30, 2019 •

edited

Loading