Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type hinting variadic arguments #4858

Closed
milesj opened this issue Feb 18, 2015 · 12 comments
Closed

Type hinting variadic arguments #4858

milesj opened this issue Feb 18, 2015 · 12 comments
Assignees

Comments

@milesj
Copy link
Contributor

milesj commented Feb 18, 2015

If we don't type hint variadic arguments we get this following type checker error.

Was expecting a type hint (what about: array)

If we do type hint them, the type checker error goes away, but then we get this runtime error.

Fatal error: Parameter $args is variadic and has a type constraint (array); variadic params with type constraints are not supported

What's the best way to go about this?

@paulbiss paulbiss added the hack label Feb 18, 2015
@paulbiss
Copy link
Contributor

The runtime doesn't have full support for variadics yet (they can't be typed). The main concern is that in PHP 5.6 type-checking a variadic requires looping over the variadic array to check each argument. We plan to add support for this, but in the meantime if you want to use strict mode with variadics you'll need to add an HH_FIXME,

function foo(
  int $arg1,
  bool $arg2,
  /* HH_FIXME[4033]: variadic + strict */ ...$args
): string {
  // ...
}

mpajunen added a commit to mpajunen/phamda that referenced this issue Feb 22, 2015
HHVM does not support type hints for variadic arguments yet:
facebook/hhvm#4858
BenMorel added a commit to brick/money that referenced this issue May 31, 2015
Variadic params with type constraints are not supported by HHVM yet.
See facebook/hhvm#4858
milesj pushed a commit to titon/context that referenced this issue Jun 1, 2015
milesj pushed a commit to titon/utility that referenced this issue Jun 1, 2015
BenMorel added a commit to brick/math that referenced this issue Jun 5, 2015
BenMorel added a commit to brick/geo that referenced this issue Sep 21, 2015
BenMorel added a commit to brick/date-time that referenced this issue Nov 30, 2015
emonkak added a commit to emonkak/date-time that referenced this issue Dec 4, 2015
* upstream/master:
  Add note about facebook/hhvm#4858 bug
  HHVM bug #3651 fixed
  TimeZoneRegion methods to get available time-zone identifiers
  Fix failing test
  Add (plus|minus)(Months|Years)() methods to YearMonth
@fredemmott fredemmott self-assigned this Dec 14, 2015
@fredemmott
Copy link
Contributor

Had a chat with @dlreeves and @sgolemon; we're likely to:

  • at runtime, ignore the type in Hack files and trust the typechecker (this is consistent with other array parameters, if we just consider variadics to be syntactic sugar over them)
  • keep on banning them in PHP files as (1) we've not came across any real-world examples of people typing variadics in PHP code (2) the runtime verification that PHP requires is very expensive (3) it's better to loudly complain than to silently allow bad parameters

@milesj
Copy link
Contributor Author

milesj commented Dec 14, 2015

I'm curious what's expensive about it? It seems like a basic O(N) problem, which shouldn't be too large, usually under 10 items.

@fredemmott
Copy link
Contributor

an O(n) problem at runtime is a large perf issue in the context of HHVM.

@milesj
Copy link
Contributor Author

milesj commented Dec 14, 2015

True, but it seems like variadic arguments would be so sparse, that the cost incursion would be relatively minor. Coming from an outside perspective at least.

@fredemmott
Copy link
Contributor

It's also almost unnecessary in Hack (same exceptions as generics), and it would be weird to have different validation behavior for Hack and PHP.

@jurchiks
Copy link
Contributor

If PHP 7 supports it, you have to support it eventually.

@mschwager
Copy link

HH_FIXME[4033] doesn't work for type hinted variadics:

$ cat test.php 
<?php

function test(/* HH_FIXME[4033] */ int ...$numbers) {
    echo array_sum($numbers);
}

test(1, 2, 3);

$ hhvm test.php 
Fatal error: Parameter $numbers is variadic and has a type constraint (int); variadic params with type constraints are not supported in /path/to/test.php on line 3

$ php test.php 
6

Any thoughts on how to remedy this?

@lstrojny
Copy link

Btw, real world usage of variadic callables: https://github.com/lstrojny/functional-php/blob/master/src/Functional/Compose.php

@fredemmott
Copy link
Contributor

Closing - supported in '<?hh' since 3.13. Not supported in '<?php', filing a separate issue.

@fredemmott
Copy link
Contributor

@mschwager, @lstrojny : #6954

@fredemmott
Copy link
Contributor

@mschwager : HH_FIXME has no runtime affects; it only affects the Hack typechecker, and your code isn't hack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants