Add more typehints to arguments in PHP
ClosedPublic

Authored by mtijink on Jun 22 2017, 12:57 PM.

Details

Summary

In php 7.0 and php 7.1, new type hints were added. I would like to see those supported in KDevelop.

This patch adds support for most of the new type hints (only "self" is missing), but some types do not get a better resulting DUChain type, since I do not know how to add the types "iterable" and "callable" in the DUChain.

Since this is my first patch, I also have some questions, which I hope you can answer:

  • How should I add support for "self"? Adding it as a token gave all kinds of problems.
  • How should the "callable" type be represented? It should accept any kind of function, but I only saw support for types with known function signatures
  • This patch added some tokens, so they cannot be used as class names and constants anymore. This is a problem, I think, since PHP < 7 still allows e.g. "int" as class name. Another problem is that the newer keywords (e.g. "iterable", "int") can still be used as names for constants (but not for classes etc.), which is not allowed with this code either.

Feedback very welcome!

Diff Detail

Repository
R52 KDevelop: PHP Support
Lint
Lint Skipped
Unit
Unit Tests Skipped
mtijink created this revision.Jun 22 2017, 12:57 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJun 22 2017, 12:57 PM
apol added a subscriber: apol.Jun 22 2017, 1:11 PM

No tests? :)

mtijink updated this revision to Diff 15749.Jun 22 2017, 3:46 PM

I added a couple of tests, I think they cover everything I changed. Also, I added support for the "?" optional marking for typehints.

pprkut added inline comments.Jun 22 2017, 4:01 PM
parser/php.g
887

I think calling this "isNullable" would better indicate what it entails. "?string" doesn't mean the string type is optional, it means it can either be string or NULL.

mtijink updated this revision to Diff 15757.Jun 22 2017, 6:51 PM

Good point, changed.

mtijink updated this revision to Diff 15918.Jun 27 2017, 1:04 PM

I also added type support for iterable (previous, it only parsed correctly), which either is an array or an object which implements Traversable. Could someone look at this, give some comments and (if okay) merge it?

Also, I still do not know how the issues I mentioned earlier should be handled:

  • How should I add support for self? Adding it as a token gave all kinds of problems. Update: adding it by detecting self as a class before assigning the typehint works, but might not be the ideal solution.
  • How should the callable type be represented? It should accept any kind of function, but I only saw support for types with known function signatures
  • This patch added some tokens, so they cannot be used as class names and constants anymore. This is a problem, I think, since PHP < 7 still allows e.g. int as class name. Another problem is that the newer keywords (e.g. iterable, int) can still be used as names for constants (but not for classes etc.), which is not allowed with this code either.
pprkut accepted this revision.Jun 27 2017, 3:58 PM

The changes look good, although I think it would be preferable to have the basic types in a separate commit from the more complicated ones like iterable, self, object, etc.

Regarding the tokens, I'm afraid that's the way it is right now. There's currently no way to support more than one PHP version and traditionally we opted for the newest one.
My next goal was context sensitive lexer support which should take care of the problem with using the tokens as identifiers, so I wouldn't worry too much about that.

IMHO, self should still work as a token. We could take a look at the errors you're getting in a separate WIP diff. You could take some hints from the grammar definition in the PHP7 source and see how they did it for comparison.

Regarding callable, I'm not sure what you mean.

This revision is now accepted and ready to land.Jun 27 2017, 3:58 PM

I do not have commit rights, so could you submit it for me? I can supply the two diffs (in two new revisions? Or one here and a separate one?).

Additionally, I was thinking it is useful to move the typehint code to a helper function: it's already used in two places here. Return typehints, which I also plan to add, also need this code.

Actually, php does not have any of the type hints in its grammar. I guess they distinguish between classes and other typehints at a larger stage. We could do that there too, if preferred.

About callable: as far as I found, the DUChain only supports function types like int (float, string), whereas callable accepts any function, so no signature is known.

mtijink updated this revision to Diff 16131.Jul 3 2017, 2:18 PM

Here's a small refactoring: the code determining the parameter type was refactored into a helper function. The support for iterable was removed in the new diff, it will be added in a later diff.

mwolff accepted this revision.Jul 4 2017, 8:45 AM
mwolff added a subscriber: mwolff.

lgtm, I'll land this now

mwolff added a comment.Jul 4 2017, 8:47 AM

What email should I associate to you with this patch?

matthijstijink@gmail.com is good, thanks!

Could you merge this change? I have a couple of follow-up diffs adding more of the new PHP features.

This revision was automatically updated to reflect the committed changes.