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

Add support for maths with constant numeric string #2209

Merged
merged 7 commits into from
Jun 22, 2023

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jan 30, 2023

When doing math, constantString can be casted to ConstantInt by Phpstan to compute the result.
Closes phpstan/phpstan#8803

In the same way boolean can be casted to 0|1, I added a commit
Closes phpstan/phpstan#8827

@VincentLanglet VincentLanglet requested a review from staabm January 31, 2023 20:04
@VincentLanglet VincentLanglet force-pushed the stringAdditions branch 2 times, most recently from 9a0575c to ebab444 Compare February 2, 2023 20:49
@VincentLanglet VincentLanglet changed the base branch from 1.10.x to 1.9.x February 2, 2023 20:50
@VincentLanglet VincentLanglet changed the title Add support for maths with constant numeric string Add support for maths with constant numeric string and boolean Feb 2, 2023
@phpstan-bot phpstan-bot changed the title Add support for maths with constant numeric string and boolean Add support for maths with constant numeric string Feb 2, 2023
@VincentLanglet VincentLanglet changed the base branch from 1.9.x to 1.10.x February 21, 2023 17:41
@VincentLanglet VincentLanglet force-pushed the stringAdditions branch 2 times, most recently from e709ec9 to 95c0003 Compare February 25, 2023 15:53
@VincentLanglet
Copy link
Contributor Author

Friendly ping @ondrejmirtes, I'm not sure if it's just a time-issue or if this PR got away from your notifications

@VincentLanglet VincentLanglet force-pushed the stringAdditions branch 3 times, most recently from 10ef075 to d77dcbe Compare March 14, 2023 10:07
@ondrejmirtes
Copy link
Member

On one real world project, it shows 101 new errors.

One example:

	/**
	 * @return array<int, array<string, int>>
	 */
	public function getIdsByStatus(BankTransactionStatus $status): array
	{
		return $this->entityManager->createQueryBuilder()
			->select('bt.id')
			->from(BankTransaction::class, 'bt')
			->andWhere('bt.status = :status')->setParameter('status', $status)
			->addOrderBy('bt.id', 'ASC')
			->getQuery()
			->getArrayResult();
	}

PHPStan reports:

 ------ -------------------------------------------------------------------------------------------
  Line   app/services/Bank/BankTransactionRepository.php
 ------ -------------------------------------------------------------------------------------------
  473    Method Slevomat\Bank\BankTransactionRepository::getIdsByStatus() should return array<int,
         array<string, int>> but returns array<int, array<string, int|null>>.
 ------ -------------------------------------------------------------------------------------------

The primary key really can't be null. The definition:

	#[ORM\Id]
	#[ORM\GeneratedValue]
	#[ORM\Column(type: 'big_integer', options: ['unsigned' => true])]
	#[Get]
	private ?int $id = null;

Another similar example:

			$id = $this->entityManager->createQueryBuilder()
				->select('c.id')
				->from(Campaign::class, 'c')
				->andWhere('c.hash = ?', $hash)
				->setMaxResults(1)
				->getQuery()
				->getSingleScalarResult();

PHPStan thinks the $id can be int|null but in fact can only be int.

@VincentLanglet
Copy link
Contributor Author

On one real world project, it shows 101 new errors.

I think you meant to post this on phpstan/phpstan-doctrine#412
I'll take a look.

@ondrejmirtes
Copy link
Member

Oh yeah, sorry :)

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.

Hi, type inference tests are great but the linked bug reports are about false positives in specific rules. Please also add regression tests for those rules. Thank you.

@VincentLanglet
Copy link
Contributor Author

Hi, type inference tests are great but the linked bug reports are about false positives in specific rules. Please also add regression tests for those rules. Thank you.

I added non-regression tests for those rules :)

@VincentLanglet
Copy link
Contributor Author

I rebased the PR, and the three failing build seems unrelated @ondrejmirtes :)

@VincentLanglet VincentLanglet force-pushed the stringAdditions branch 3 times, most recently from b12516f to bf1ebef Compare May 11, 2023 06:51
@VincentLanglet
Copy link
Contributor Author

Friendly ping @ondrejmirtes, I rebased the PR. This should be ready to be reviewed :)

@ondrejmirtes ondrejmirtes merged commit be7e86c into phpstan:1.10.x Jun 22, 2023
@ondrejmirtes
Copy link
Member

Thank you!

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