Skip to content

[CodeQuality] Add concat support on ReturnTypeFromStrictScalarReturnExprRector to be string#3435

Merged
TomasVotruba merged 16 commits intomainfrom
concat-support-return-strict
Mar 2, 2023
Merged

[CodeQuality] Add concat support on ReturnTypeFromStrictScalarReturnExprRector to be string#3435
TomasVotruba merged 16 commits intomainfrom
concat-support-return-strict

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented Mar 2, 2023

direct return concat will always be string, ref https://3v4l.org/mfs5l unless defined.

@samsonasik samsonasik requested a review from TomasVotruba as a code owner March 2, 2023 07:45
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Mar 2, 2023

For combine of integer type, I think that need to be skipped as possibly used as float.

@samsonasik samsonasik force-pushed the concat-support-return-strict branch from d621c90 to e76a1dd Compare March 2, 2023 08:06
@samsonasik samsonasik force-pushed the concat-support-return-strict branch from cf5b05e to 7596dca Compare March 2, 2023 08:15
@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Mar 2, 2023

After some check, concat actually always be string, even object concated:

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@TomasVotruba TomasVotruba merged commit 4e625d0 into main Mar 2, 2023
@TomasVotruba
Copy link
Copy Markdown
Member

Thank you 👍

@TomasVotruba TomasVotruba deleted the concat-support-return-strict branch March 2, 2023 12:12
{
public function run()
{
return 1 . 1;
Copy link
Copy Markdown
Contributor

@staabm staabm Mar 5, 2023

Choose a reason for hiding this comment

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

this could also be float 1.1 (meaning the . is not a concat op, but a decimal separator )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

1.1 is float, but 1 . 1 is concat;

ref https://3v4l.org/pFIUa

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh wow. didn‘t realize whitespace is significant in this case. Makes sense

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems suprising enough, that we should have a separate unit case for it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think not needed, 1.1 is exactly Dnumber :), 1 . 1 is Concat

samsonasik added a commit that referenced this pull request May 8, 2023
…xprRector to be string (#3435)

Co-authored-by: GitHub Action <actions@github.com>
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.

4 participants