[Proposal] Type-hint what can be type-hinted

X-Coder264 commented 5 years ago

There has been a bunch of docblock PRs lately (and they are coming all the time) as evidenced by Taylor's comment - https://github.com/laravel/framework/pull/26320#issuecomment-434686720. I think something should definitively be done about that. We shouldn't waste so much time on such "stupid" stuff as we can use what the language already gives us instead of docblocks.

As Laravel requires PHP 7.0 since 5.5 and 7.1 since 5.6 I think it's finally time to start using PHP 7.1 features. My proposal is that we start using scalar type-hints and return type-hints (both object and scalar) for all new features from 5.8 (or 5.9 onwards). Telescope already kinda started that trend (https://github.com/laravel/telescope/blob/1.0/src/Contracts/EntriesRepository.php#L17, https://github.com/laravel/telescope/blob/1.0/src/Telescope.php#L213 etc.), but that needs to become a standard thing in the framework itself as well.

In addition to that the proposal is to add the no_superfluous_phpdoc_tags PHP CS Fixer rule to the StyleCI config. Activating that rule would immediately cut down a bunch of unnecessary docblocks and as new stuff gets type-hinted almost all new code wouldn't even have any docblocks.

Of course I'm not proposing to force type-hinting stuff just for the sake of it - for example it doesn't make sense to type-hint something as a Closure if an invokable class can be passed as well. That would only limit the functionality without any good reason, but in most cases it makes sense to type-hint stuff.

It's 2018, IDEs are smart enough to understand this stuff without the need for docblocks. Less docblocks = less maintenance cost 😉

michaeldyrynda commented 5 years ago

What’s your argument in favour of typehints that isn’t “it’s easier for my IDE to understand”?

A lot of Laravel’s power comes from using reflection depending on passes arguments; statically typing methods would hamper some of this functionality - think methods that accept an array of arguments AND separate positional arguments.

sisve commented 5 years ago

Defensive programming. If you expect a string, declare it a string to avoid any weirdness. This example is can also argue for strict comparisons when you expect certain types. However, if you expect a type, why not declare that expectation? That way a strict_types user will be notified if these expectations aren't met.

<?php

function isPasswordReasonable($passwordHash) {
    return $passwordHash == md5('reasonable');
}

function isPasswordReasonableTyped(string $passwordHash) {
    return $passwordHash == md5('reasonable');
}

var_dump(isPasswordReasonable(md5('unreasonable'))); // false
var_dump(isPasswordReasonable(md5('reasonable')));   // true
var_dump(isPasswordReasonable(5));                   // true

var_dump(isPasswordReasonableTyped(md5('unreasonable'))); // false
var_dump(isPasswordReasonableTyped(md5('reasonable')));   // true
var_dump(isPasswordReasonableTyped(5));                   // false

A lot of Laravel’s power comes from using reflection depending on passes arguments; statically typing methods would hamper some of this functionality - think methods that accept an array of arguments AND separate positional arguments.

Method accepting several types are problematic. We've seen PR's adding callable support to methods accepting arrays; but some arrays will then be considered callable if they have certain content.

Method with optional parameters are problematic. There have been cases where we supported Model::where('field', $request->input('search_field')) to issue a WHERE field = $input, but what happened if someone entered "=" into the field? You didn't get WHERE field = '=', you got WHERE field IS null because Laravel interpreted this as Model::where('field', '=', null).

How many issues have we gotten so far because someone has tried $table->integer('age', 11) (to create mysql's INT(11)), but got issues with primary keys? The second parameter is really a boolean-ish variable telling the migration to create a primary key (weirdly named $autoIncrement). The problem here are two-fold, we would need to both type the integer function, and promote the use of strict_types to catch the error. But if we start typing the method, then the user can opt-in into getting these errors about wrong types.

michaeldyrynda commented 5 years ago

I think the first two make it a non-starter, otherwise you’ll end up with a mish mash of hints and no hints, which will make it harder to reason about.

The static typing only helps if you’re using an IDE that parses types for you, or some linter/static analysis. End of the day, types or no types, PHP will blow up at runtime, seeing as there’s no compilation now.

mfn commented 5 years ago

PHP will blow up at runtime, seeing as there’s no compilation now.

You can also argue about "where" it blows up.

If you use type declarations, and to that extend, strict_types, the chances are the wrong values will blow up as early as possible at runtime and not deep down somewhere in the framework which in my experience is very useful.

otherwise you’ll end up with a mish mash of hints and no hints, which will make it harder to reason about.

I think a mish mash is inevitable anyway. Unlikely a 10k lines PR will land, converting everything
I don't see what makes it harder, please elaborate. I'd say it makes everything easier.

The current way of phpdoc to hint type is a hack/workaround introduced at times where PHP had none of them. As @X-Coder264 but it eloquently :-)

We shouldn't waste so much time on such "stupid" stuff as we can use what the language already gives us instead of docblocks.

I've been using strict_types + type declarations on mid-sized projects for years and did nothing but improve things.

Enforcing a type as early as possible is always preferable than silently going ahead and let it blow up later.

Yes, there will always be cases that can't be declared (polymorphic function calls, types within arrays) but this doesn't prevent going ahead for all other cases where it's possible.

And who knows, one day we might get support for embedded types or Generics or …

michaeldyrynda commented 5 years ago

You can also argue about "where" it blows up.

Yes, you're just changing where it blows up. It blows up regardless.

I don't see what makes it harder, please elaborate. I'd say it makes everything easier.

For one, you have to figure out what can and what can't be hinted, and you have to figure out why one thing is hinted and another isn't. Granted this is only if you get into the belly of the framework, but cognitive overhead nonetheless.

Enforcing a type as early as possible is always preferable than silently going ahead and let it blow up later.

Enforcing types in a dynamic language makes sense than in a statically typed one. You're taking away a lot of the power that the dynamic nature of the language gives you. It's weird that PHP is taking this hybrid path, but other dynamic languages (Ruby, Python) are happy sticking to their dynamic roots.

mattstauffer commented 5 years ago

Just in case it looked like Michael was the only one with his opinion, I'm gonna chip in here and throw 100% support behind what he's saying here to try to even at least the number of people on each side out a bit.

OK, as you were ❤️

X-Coder264 commented 5 years ago

I think the first two make it a non-starter, otherwise you’ll end up with a mish mash of hints and no hints, which will make it harder to reason about.

What here is "harder to reason about"? Ideally we would type-hint everything in the framework at once, but seeing as a change in a method signature is a breaking change that can't be done. But all changes have to start somewhere, so yes, there would be a "transition period" where there would be methods which are type-hinted and methods which are not. I don't want to be "that" guy, but I guess I have to be - Symfony does this for a while and has progressed a lot if you compare to what the state of the codebase was just a year ago. And I don't see that they lost any functionality because of that nor I see any new "cognitive overhead" (whether we are talking about using those methods as a user or whether we are talking about working with that code as a framework contributor).

For one, you have to figure out what can and what can't be hinted, and you have to figure out why one thing is hinted and another isn't. Granted this is only if you get into the belly of the framework, but cognitive overhead nonetheless.

When you are developing a new feature for the framework you obviously know how you want your feature to be used and with what types your code works with so I don't see a problem there. It's not like we are gonna go to every method in the framework and be like "can we type-hint this" and then do it or not. When looking from a perspective of a developer using the framework, why should he concern himself with "oh why is this typed and this isn't" stuff? So TBH I don't understand all this arguments about cognitive overhead.

Like others previously mentioned using type-hints (especially in combination with strict_types) helps to avoid a bunch of bugs and there is less maintenance cost for the maintainers ("oh look here, the docblock is out-of-sync with the implementation, let's send a PR" kind of thing). So from my POV this would be a win-win situation.

Enforcing types in a dynamic language makes sense than in a statically typed one. You're taking away a lot of the power that the dynamic nature of the language gives you.

I said that we should type-hint only what makes sense to type-hint. Having said that automatically excludes the possibility of taking away even the smallest bit of "power" from the language/framework. But of course if I expect that my function receives a string, I don't want an integer passed (like the above example with hashing) so to be able to type-hint that would be great. I'd argue that docblocks are the ones presenting a "cognitive overhead" as there is stuff that should blow up when I pass a non expected value, instead you'll probably get some weird edge case behavior and then you'll have to go and check the docblock to see what's allowed first (and all of that could be avoided if stuff would be a bit more stricter as PHP would tell you straight away that what you are trying to do is wrong).

sisve commented 5 years ago

The static typing only helps if you’re using an IDE that parses types for you, or some linter/static analysis. End of the day, types or no types, PHP will blow up at runtime, seeing as there’s no compilation now.

Yes, you're just changing where it blows up. It blows up regardless.

These statements are wrong. My code examples didn't blow up at all, they had unexpected behavior. This wasn't undefined behavior, the php language is very clear that it will type-juggle if necessary. However, it was unexpected behavior in the mind of the developer, because the dev only thought the function would accept strings (but didn't tell the runtime about it).

Type hints can help us help php to stop automatically type-juggle values for us. We know what we want in our code, we know what expectations, limitations and behavior is expected from our code. Why shouldn't we declare them?

X-Coder264 commented 5 years ago

If this has any hope of being seriously considered we really need some feedback about this from @taylorotwell, @driesvints and @themsaid :)

zlikavac32 commented 5 years ago

@michaeldyrynda You can't trust comments, you can trust the code. I don't think of type hints as a bad thing. And it's fine to have them where they are applicable. Why?

If we were to define a new function/method, first thing we'd do is check for user input. If I expect integer, and it's not integer, I want to throw exception to notify that something went wrong.

If for one parameter I only have one type that's valid, why bother with custom validation code when PHP can do all that stuff for us? I don't see where is the cognitive load here. Checks should be performed one way or the other.

In languages like C and Rust, types have a different meaning, than they have in PHP. In PHP, I think of them as language level asserts, that reduce amount of my code I have to write, to be able to trust the code that I wrote.

It also goes the other way around, If I expect the return value to be an integer, I don't want to check whether it really is or not. I wanna trust it.

Trusting comments is like trusting my government (spoiler alert, it does not work).

Why not typehint everything that is know to be instance of a class/interface or some other type, and provide PHPDoc for the ambiguous ones?

fico7489 commented 5 years ago

@X-Coder264 I am a big fan(the biggest one) of removing docblocks everywhere where docblocks are not needed like in your example :

from :

/*
 *
 * @param  \Illuminate\Http\Request  $request
 * @param  \Exception  $e
 * @return \Symfony\Component\HttpFoundation\Response
 * @return \Illuminate\Http\Response
 */
protected function renderException($request, Exception $e)
{

to :

protected function renderException(Request  $request, Exception $e) : Response
{

because in second example we already have all pieces of information that are in first docblock,

now IDE can know everything, like from first docblock example
if you pass wrong first param it would be much cleaner to you what is wrong
we have more cleaner code
we don't have duplicated code
we have less lines of code (which is good)
we have less things to maintain

imanghafoori1 commented 5 years ago

I saw that now one has talked about the performance difference so it is good to take a look at this:

https://stackoverflow.com/questions/32940170/are-scalar-and-strict-types-in-php7-a-performance-enhancing-feature

It seems that even if we enable OPcache, still there would be performance benefits.

driesvints commented 5 years ago

Just want to chip in here and give my 2 cents. Overall I agree with the statements made by @X-Coder264, @sisve and @mfn. I've always made it clear that I'm pro-type hinting what can be type hinted on Twitter etc.

I believe if any discussion is to be had about this they should refrain from being held about IDE beneficial ones. Those arguments are clear that they're helpful but as not everyone uses an IDE they're not beneficial to all.

What does warrant discussion is all the other benefits type-hints can bring:

Clearer errors (early faults)
Clearer APIs (because they're now enforced)
Smaller codebases (bye bye DocBlocks)
etc.

I personally would love to see the framework and our libraries adopt (much) more type hints but this decision is not up to me and it's important that we respect the wishes of all the community.

I'm gonna abstain from commenting further as I think I've made my position clear many times before :)

PS. Thanks to all of you for making this a constructive discussion 👍

X-Coder264 commented 5 years ago

There is a bunch of benefits about using type-hints, most of which you've also listed @driesvints and the IDE ones are just a nice little extra :)

But the "issue" at hand is not only what we gain if we use more type-hints, it's also about what are we missing and what problems/bugs arise because they aren't used as much as they should be. Just a recent example of this is this PR: https://github.com/laravel/framework/pull/26726

If the interface in question had proper type-hints (which it didn't as it was created when PHP 7 was quite fresh) we would've never got this PR as the original author who implemented this feature would've noticed the error before even sending his PR.

Enforcing interfaces with just docblocks in a time when we have type-hints doesn't make any sense.

DannyvdSluijs commented 5 years ago

To chip in I think this feature would be very helpful (and welcomed) as it will reduce overhead (both in the framework, but also in applications build on top of the framework as no type checking is needed anymore.)

The IDE benefits are only pre and should not way in the decision. The other benefits are already mentioned some time. I order to build an application I tend to program the stuff as strict as possible to avoid any scenario's or input I didn't consider. Currently the framework would allow any variant of inputs which in the context of PHP7 seems a bit old school. Personally I would thing keeping up with the latest language features is one of the bests ways to try and maintain your position as a framework. Standstill is only decay.

Don't get me wrong, Im not bashing or any. I believe the framework is very helpful in so many ways. I would only like to assist to make is even better. What would be the next logical step to get this item progressing?

Ref: https://github.com/laravel/framework/pull/28373#issuecomment-487932358

taylorotwell commented 5 years ago

This is a pretty broad proposal. The framework already uses type hints extensively. We can't type-hint every method because PHP's type-hinting system is immature compared to other languages and does support powerful things like method overloading, which means we wouldn't be able to offer methods that accept multiple types easily on our end - even though there are definitely valid use cases for such methods that we all know and use every day.

In general, I'm fine with type-hinting where it makes sense. But, it's also important all readers keep in mind type-hinting in a framework is pretty different than type-hinting in your own application. In your own application, you can change type-hints as needed. In a framework, we are locked in for at least 6 months per change and even then we risk breaking thousands of applications by changing type-hints if people are overriding methods, etc. So, type-hints for some things should be chosen with care. But, in general, the framework the already uses a ton of type-hints (all of the ones that were available when it was first written).

imanghafoori1 commented 5 years ago

Although I was a fan of type-hinting before... but my mind changed after contributing to laravel core. I agree that php type-system is immature and is risky to add type-hints. Better not to rush for it for now.

X-Coder264 commented 5 years ago

@taylorotwell

I don't think that this is a very "broad" proposal. The proposal consists of two things:

type-hint what can be type-hinted without causing breaking changes
start using scalar and return type-hints for all newly written code for the framework

About the first thing, Symfony is doing this exact thing for Symfony 5 -> https://github.com/symfony/symfony/issues/32179

Since Symfony 5 (and Laravel 6) will require PHP 7.2 which brought parameter type widening there is actually a bunch of places now in the code where such a thing can be done without causing any breaking change (as evidenced by the bunch of Symfony PR doing exactly that for each SF component).

About the second thing, I don't agree that the

framework already uses type hints extensively

at least not in the context of this proposal. This proposal is about the new features in PHP 7 which are scalar and return type hints. The framework at this moment does not use these features at all. When a PR for the framework comes in with (new) code that uses these features there are always comments to remove those as the framework "officially doesn't use scalar and return type-hints". Even if such code manages to get merged a followup commit is immediately made to remove those type-hints. I'd like to change that "policy" with this proposal.

imanghafoori1 commented 5 years ago

I did a very simple benchmark to compare the effect of type-hints on function call performance I thought it is going to be faster... But it became much Slower after adding scalar type-hints !!!

it is not scientific but it shows there is performance hit at least in some cases to be worried about.

https://gist.github.com/imanghafoori1/a9f672a9cc0a693737a75612822c3b82

results on my windows machine: 0.0167 (with type-hints) 0.0103 (with no type-hints)

X-Coder264 commented 5 years ago

PHP is a dynamic language and type checking is done at runtime so by adding type-hints the application surely won't be faster (further optimizations with type hints could probably be made after JIT lands in PHP though). The performance hit that you get from type hinting is so negligible that I haven't seen performance being brought up in any PHP typing discussion to be honest. I tried your benchmark on my machine and the difference is 1.5 ms on one hundred thousand method calls. Saying that that is "much slower", especially considering that in a real HTTP request that number probably isn't nowhere near that seems weird.

imanghafoori1 commented 5 years ago

For comparing performance I think it is more fair to say how many percents rather than milliseconds. (Like they say php7 is 300% faster then php5.6)

I do not want to calculate a value here since it is not a scientific benchmark.

But at least we can say : it would not be faster for php 7.2 or 7.3

Because some people (including myself) have mentioned performance benefits, in the above comments.

X-Coder264 commented 5 years ago

I don't care if percentage or milliseconds are used for comparing performance, the point is that in a real HTTP request scenario (as a user browsing a site) you couldn't tell the difference between a type-hinted and non type-hinted codebase.

zlikavac32 commented 5 years ago

@imanghafoori1 If you were so concerned with performance, you wouldn't use in_array on so many places, instead of constructing a hash map or something, depending on the access pattern (for example).

Just saying :)

imanghafoori1 commented 5 years ago

Maybe in php7.4 or above the script start to run faster with JIT and pre-loading.

Comparing performance is like comparing the speed of light and sound.

If you are in a room looking to your mom and talking her. Can you tell the difference the of speed of light and sound? The both seem to be real-time and infinitely fast.

But if you see lightening up in the sky you can perfectly see the difference. Light is real-time but sound has a sensible delay.

Then if you want to talk to your friend on the Mars from earth... Then you even start to hope for light to be faster, since your messages travelling at the speed of light, get to Mars only after 20 minutes !!!

So send a message to aliens in the nearby galaxy? Your message gets there after hundreds of years !!! Too late!

All and all it depends on the scale.

Under heavy loads any framework start to show it's slowness at some point.

laravel / ideas

[Proposal] Type-hint what can be type-hinted #1409