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

support ConstExprNode node to avoid crashing with 500 #47

Closed
wants to merge 1 commit into from
Closed

support ConstExprNode node to avoid crashing with 500 #47

wants to merge 1 commit into from

Conversation

ildyria
Copy link

@ildyria ildyria commented Oct 5, 2022

This allows the AST parser to not break on a ConstExprNode when going through documentation.

2022-10-05_1920x1080_12:05:04

@romalytvynenko
Copy link
Member

@ildyria Thanks for the fix!

Can you please share an example of the PhpDoc that causes this fail?

Thanks!

@ildyria
Copy link
Author

ildyria commented Oct 5, 2022

@ildyria Thanks for the fix!

Can you please share an example of the PhpDoc that causes this fail?

Thanks!

That is going to be difficult to find, I am using scramble as direct plug on https://github.com/LycheeOrg/Lychee and the project is quite big. I will try to find one.

@ildyria
Copy link
Author

ildyria commented Oct 5, 2022

Found it. This will crash.

	/**
	 * Reset the token of the currently authenticated user.
	 *
	 * @return array{'token': string}
	 *
	 * @throws UnauthenticatedException
	 * @throws ModelDBException
	 * @throws \Exception
	 */
	public function resetToken(): array
{...}

This will pass.

	/**
	 * Reset the token of the currently authenticated user.
	 *
	 * @return array
	 *
	 * @throws UnauthenticatedException
	 * @throws ModelDBException
	 * @throws \Exception
	 */
	public function resetToken(): array
{...}

Note that this is valid and follows https://phpstan.org/writing-php-code/phpdoc-types#array-shapes

@romalytvynenko
Copy link
Member

I see.

But as far as I can tell, suggested fix will result in loosing type information.

I believe that it should be a more comprehensive support of the const expressions in PhpDoc (which will result in a correct type).

So the PhpDoc comment:

/**
 * @var $foo array{'token': string}
 */

Should become type:

array{token: string}

With the suggested fix it will become an unknown type key:

array{unknown: string}

@ildyria
Copy link
Author

ildyria commented Oct 5, 2022

Said function is:

	/**
	 * Reset the token of the currently authenticated user.
	 *
	 * @return array{'token': string}
	 *
	 * @throws UnauthenticatedException
	 * @throws ModelDBException
	 * @throws \Exception
	 */
	public function resetToken(): array
	{
		/** @var User $user */
		$user = Auth::user() ?? throw new UnauthenticatedException();
		$token = strtr(base64_encode(random_bytes(16)), '+/', '-_');
		$user->token = hash('SHA512', $token);
		$user->save();

		return ['token' => $token];
	}

With this patch, the doc becomes:
2022-10-05_1920x1080_13:43:06

I believe that it should be a more comprehensive support of the const expressions in PhpDoc (which will result in a correct type).

I would agree, but I am not that familiar with the code base and php node parsing. :|

@romalytvynenko
Copy link
Member

With this patch, the doc becomes:...

Yeah, but the information is inferred from the code in this case, not from the comment:

return ['token' => $token];

I would agree, but I am not familiar with the code base. :|

That's fine, I will do it in the next release.

For now you can change PhpDoc comment to this (notice no ', which is an equivalent of the comment you have):

/**
 * @return array{token: string}
 */

I will add const expressions support in the next release.

Thanks.

@romalytvynenko
Copy link
Member

Fixed in #59

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