Skip to content

Conversation

ondrejmirtes
Copy link
Member

@ondrejmirtes ondrejmirtes commented Mar 31, 2022

Closes #4308
Closes #6965
Closes #6961

@@ -62,7 +62,7 @@ public function getTypeFromFunctionCall(
$limit = new NullType();
}

$constantArrays = TypeUtils::getConstantArrays($valueType);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you give a small explanation what the semantic difference of these 2 get*Array methods is?
If seen you used it in some places while fixing stuff in past PRs, but by reading the code itself I cannot make sense of it.

@@ -62,7 +62,7 @@ public function getTypeFromFunctionCall(
$limit = new NullType();
}

$constantArrays = TypeUtils::getConstantArrays($valueType);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you give a small explanation what the semantic difference of these 2 get*Array methods is?
If seen you used it in some places while fixing stuff in past PRs, but by reading the code itself I cannot make sense of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well obviously I thought that TypeUtils::getConstantArrays would be the way to go forward and the old version would go away, but the old version is more efficient.

The difference is that if you have array{0: 'foo', 1?: 'bar'}, TypeUtils::getConstantArrays calls ConstantArrayType::getArrays() and that returns a union of two arrays:

  • array{'foo'}
  • array{'foo', 'bar'}

So code that uses TypeUtils::getConstantArrays doesn't need to care about optional keys. The problem is that with multiple optional keys, this explodes in many combinations and can become slow. So what I'm doing in this PR is to use TypeUtils::getOldConstantArrays, have less ConstantArrayTypes in a union and handle optional keys at each place explicitly.

@ondrejmirtes ondrejmirtes marked this pull request as ready for review April 2, 2022 12:32
@ondrejmirtes ondrejmirtes force-pushed the opti-const-array branch 2 times, most recently from a48030a to 269c86d Compare April 2, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants