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 removal of useless @param tag when string|null insteadof ?string is used. #5684

Merged

Conversation

arjenschol
Copy link
Contributor

@arjenschol arjenschol commented Mar 4, 2024

Fixed by removing the "skip union types" in ParamTagRemover. This was added to fix a different bug, caused by a bug in TypeHasher, where the normalized UnionType was actually overwritten by the last element in the union,

@arjenschol
Copy link
Contributor Author

I'm actually not sure about the change in TypeHasher. Without skipping UnionType, the skip_union_of_interface_with_comment.php.inc testcase fails.

SomeClass|SomeInterface is normalized to SomeInterface.

@@ -94,7 +94,7 @@ private function createUnionTypeHash(UnionType $unionType): string
$normalizedUnionType = clone $unionType;

// change alias to non-alias
$normalizedUnionType = TypeTraverser::map(
TypeTraverser::map(
Copy link
Member

Choose a reason for hiding this comment

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

this TypeTraverser::map from line 96 to 106 seems no longer needed, then we can see if that broke something, so we can revert early :)

Copy link
Member

Choose a reason for hiding this comment

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

You can just remove exactly line 96 to 106, the clone seems still needed.

@samsonasik
Copy link
Member

Please use git rebase instead so only your commits shown in this PR

@@ -91,21 +91,7 @@ private function createUnionTypeHash(UnionType $unionType): string
return $booleanType->describe(VerbosityLevel::precise());
}

$normalizedUnionType = clone $unionType;
Copy link
Member

Choose a reason for hiding this comment

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

the clone is somehow still needed, see error in test https://github.com/rectorphp/rector-src/actions/runs/8169922103/job/22334963648#step:5:93

Suggested change
$normalizedUnionType = clone $unionType;
$clonedUnionType = clone $unionType;
return $clonedUnionType->describe(VerbosityLevel::precise());

@arjenschol arjenschol force-pushed the fix-nullable-union-param-tag-remover branch 2 times, most recently from b276474 to 098532a Compare March 7, 2024 08:46
…is used.

Fixed by removing the "skip union types" in ParamTagRemover.
This was added to fix a different bug, cause a bug in TypeHasher, where the normalized UnionType was actually overwritten by the last element in the union,
@arjenschol arjenschol force-pushed the fix-nullable-union-param-tag-remover branch from 098532a to d54f5ae Compare March 7, 2024 08:48
Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

Thank you @arjenschol , I will clean up in separate PR, or revert if it cause regression

@TomasVotruba I am merging it ;)

@samsonasik samsonasik merged commit 9b4ad93 into rectorphp:main Apr 3, 2024
39 checks passed
@samsonasik
Copy link
Member

#5792

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