Skip to content

Fix removal of useless @param tag when string|null insteadof ?string is used.#5684

Merged
samsonasik merged 1 commit intorectorphp:mainfrom
arjenschol:fix-nullable-union-param-tag-remover
Apr 3, 2024
Merged

Fix removal of useless @param tag when string|null insteadof ?string is used.#5684
samsonasik merged 1 commit intorectorphp:mainfrom
arjenschol:fix-nullable-union-param-tag-remover

Conversation

@arjenschol
Copy link
Copy Markdown
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
Copy Markdown
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.


// change alias to non-alias
$normalizedUnionType = TypeTraverser::map(
TypeTraverser::map(
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

return $booleanType->describe(VerbosityLevel::precise());
}

$normalizedUnionType = clone $unionType;
Copy link
Copy Markdown
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
Copy Markdown
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
@samsonasik
Copy link
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

2 participants