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 the implied_bounds_entailment future compatibility lint. #5

Merged

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jan 12, 2023

see rust-lang/rust#105572. Your crate unintentionally relied on a bug in the compiler of Rust itself. This bug is now getting fixed and will be a hard error in a future release.

The issue is that the signature of the impl method differs from the signature of the trait method in a way where the impl would actually have stronger requirements for the caller. This could be use to trigger undefined behavior. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=71fd0f91713510e75174a5cda4da1edc

The impl requires T: 'static because 'a has to outlive 'static for Box<dyn Any 'a> as Any requires Self: 'static.

This PR fixes this by adding the Self: 'static bound to the impl itself. This bound is required to call downcast in coalesce_any.

Please ask if there are any questions.

@lcnr lcnr changed the title fix the implied_bounds_entailment lint. fix the implied_bounds_entailment future compatibility lint. Jan 12, 2023
`Any` already requires `'static`
@QuartzLibrary
Copy link
Owner

Hi @lcnr,

Thank you for the fix and the clear example of how this could lead to UB!

It seems that by not understanding at the time that Any requires 'static anyway I stumbled through adding a spurious lifetime and then the bug. In any case, the 'a lifetime can be removed entirely.

Really fantastic to see things like this being systematically addressed. Thanks again.

@QuartzLibrary QuartzLibrary merged commit fffa802 into QuartzLibrary:master Jan 13, 2023
@lcnr lcnr deleted the fix-implied_bounds_entailment-lint branch January 14, 2023 00:22
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