Allow optional parameters before non-optional ones in PHP
ClosedPublic

Authored by mtijink on Jul 31 2017, 1:20 PM.

Details

Summary

PHP allows default values for parameters, even if later parameters do not have default values (it does not using those default values while calling functions though). This diff adds support for that, by dropping the default values of arguments before the last non-optional argument.

Diff Detail

Repository
R52 KDevelop: PHP Support
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mtijink created this revision.Jul 31 2017, 1:20 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJul 31 2017, 1:20 PM
kfunk requested changes to this revision.Jul 31 2017, 1:55 PM
kfunk added a subscriber: kfunk.
kfunk added inline comments.
duchain/builders/declarationbuilder.cpp
754

This looks a bit like a hack.

And it also introduces a regression for e.g.:
function x($b, $a = 1) -> Won't show the default arg anymore.

Maybe this issue should be addresses at the DUChain-level in kdevplatform? You can probably add a addEmptyDefaultParameter() method in FunctionDefinition in kdevplatform.

Then here (pseudo-algo), do two passes over the param list:

  • first pass: check if it has default args at all
  • second pass: if it has default args:
    • if param has default value: addDefaultParameter(...)
    • else: addEmptyDefaultParameter()

(Or some similar approach..., I don't know the exact DUChain API of the top of my head)

This revision now requires changes to proceed.Jul 31 2017, 1:55 PM

The example you provide still works for me. Maybe you meant function foo($a = 1, $b)? That did not work previously either, since it would show the following signature: [returntype] foo($a, $b = 1). But that would be useful to show, indeed.

In a way, this code already does two passes, to record how many optional/non-optional arguments there are, but I see what you mean. The DUChain code now assumes that any default argument appears at the end of the argument list (and does not store anything for earlier arguments), which is why I chose this approach. I'll check if changing that behaviour is not too difficult.

mtijink updated this revision to Diff 17487.Aug 1 2017, 12:11 PM
mtijink edited edge metadata.

Change approach to default arguments before non-default arguments

This approach just adds default arguments for all arguments, but "empty" default arguments will not be displayed (when combined with D7031)

function foo($a, $b = false, $c) {}
Should work?
Now it says that $c should have a default...

Works for me. Did you use it with the commit on kdevelop.git? It did not work without that, for me. Also, you might need to clear the cache, otherwise the old (incorrect) behaviour is still stored in the cache.

kfunk accepted this revision.Aug 22 2017, 6:45 AM
This revision is now accepted and ready to land.Aug 22 2017, 6:45 AM
kfunk added a comment.Sep 4 2017, 10:03 PM

Not merged yet. Alexander?

This revision was automatically updated to reflect the committed changes.

Sorry, had no time lately