Skip to content

Improve-callable-typing #60762

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

Open
wants to merge 2 commits into
base: 7.4
Choose a base branch
from

Conversation

jack-worman
Copy link
Contributor

Q A
Branch? 7.4
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

Defining a callable's shape is important to have valid static analysis.
It also improves documentation because developers no longer need to search the source code for what callable shape to use.

@jack-worman jack-worman requested a review from chalasr as a code owner June 11, 2025 12:26
@carsonbot carsonbot added this to the 7.4 milestone Jun 11, 2025
@jack-worman jack-worman force-pushed the Improve-callable-typing branch from 609c5fd to eeb6152 Compare June 11, 2025 12:29
@@ -237,7 +237,10 @@ private function addCsrfSection(ArrayNodeDefinition $rootNode): void
;
}

private function addFormSection(ArrayNodeDefinition $rootNode, callable $enableIfStandalone): void
/**
* @param \Closure(string, class-string):("canBeDisabled"|"canBeEnabled") $enableIfStandalone
Copy link
Contributor Author

@jack-worman jack-worman Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types like this could be more manageable if we took advantage of @psalm-type annotations, but it hasn't been done in this codebase before, so I didn't know if it was allowed.

@@ -160,6 +169,8 @@ public function setAutocompleterValues(?iterable $values): static

/**
* Gets the callback function used for the autocompleter.
*
* @return (callable(mixed):iterable)|null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return (callable(mixed):iterable)|null
* @return (callable(string):string[])|null

The autocomplete callback is expected to return an array of strings. We don't support returning an iterator (we use array functions with that value for instance) and values must be strings.
the current value from which to autocomplete is also guaranteed to be a string.

btw, the same type is useful for \Symfony\Component\Console\Helper\QuestionHelper::autocomplete (that is consuming the value returned by this getter), so you could update it as well.

private ?\Closure $autocompleterCallback = null;
/**
* @var (\Closure(mixed):bool)|null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the validator does not actually return bool. It returns the validated value (potentially transformed) or throws an exception.

@@ -70,11 +70,15 @@ public function table(array $headers, array $rows): void;

/**
* Asks a question.
*
* @param (callable(mixed):bool)|null $validator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those types are wrong (they must match the type in Question)

@jack-worman
Copy link
Contributor Author

Thanks for the review.
All comments should be addressed.

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

Successfully merging this pull request may close these issues.

3 participants