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

Fix lost list-type if substituted a element via loop #3908

Merged
merged 4 commits into from
Mar 30, 2025

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 28, 2025

@staabm
Copy link
Contributor Author

staabm commented Mar 28, 2025

I am not sure about the impl, as it e.g. does not work for the regular for loop as we don't have $items[$i] in the scope expressions (don't know whether that is intentional):

/**
 * @param non-empty-list<int> $items
 *
 * @return non-empty-list<int>
 */
function getItemsWithForLoop(array $items): array
{
	for($i = 0; $i < count($items); $i++) {
		$items[$i] = 1;
	}

	assertType('non-empty-list<int>', $items);
	return $items;
}

edit: logged as a new issue so its not forgotten

@staabm staabm marked this pull request as ready for review March 28, 2025 14:31
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This is nice, I have two ideas for improvements:

  1. Try to do this logic in produceArrayDimFetchAssignValueToWrite instead, I suspect it might work for nested assigns ($a[$i][$j]) as well.
  2. Try calling setExistingOffsetValueType instead of setOffsetValueType in this scenario. Right now I think it's used for some weird thing around unset but I suspect it might be a bit more elegant. We need to fix how AccessoryArrayListType::setExistingOffsetValueType is implemented too.

These are just ideas - if they don't work, I'm happy to revert to the current state and merge it as it is. Thanks!

@staabm
Copy link
Contributor Author

staabm commented Mar 30, 2025

I have implemented 1) - it works like you suspected.

regarding 2) - what are the expectations with setExistingOffsetValueType? I don't see why it would be more elegant or whether it will cover a situation setOffsetValueType won't. what are the differences between this 2 methods?

@ondrejmirtes
Copy link
Member

setExistingOffsetValueType

Nevermind, probably not a good idea 😊

@ondrejmirtes ondrejmirtes merged commit 9efcdf5 into phpstan:2.1.x Mar 30, 2025
416 of 417 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the bug12274 branch March 30, 2025 11:35
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.

Type list is lost if you substitute a element via loop
3 participants