-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[DO NOT MERGE] Tail expr drop order crater run #129607
[DO NOT MERGE] Tail expr drop order crater run #129607
Conversation
r? @Nadrieril rustbot has assigned @Nadrieril. Use |
cc @traviscross |
r? jieyouxu |
975059d
to
78ec48b
Compare
if visitor.tcx.features().shorter_tail_lifetimes | ||
&& blk.span.edition().at_least_rust_2024() | ||
{ | ||
if blk.span.edition().at_least_rust_2021() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: note that this will limit your analysis of breakage to crates that are edition=2021
and beyond. Does previous editions have tail expr drop changes? Or should this not be gated on editions at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to say. I think not and the drop order should have been consistent before and including Edition 2021.
In side discussion, I've suggested revising this PR such that it will both have the new lint fire at deny-by-default and will enable the new behavior. That way, we'll get a full list of all crates affected in any way. We can then do more targeted and faster crater runs against that list then, if needed. @dingxiangfei2009 is working on that. |
@jieyouxu for context, let's do a build-and-test with Does this |
TC can also run crater so yeah whatever makes sense |
This comment has been minimized.
This comment has been minimized.
78ec48b
to
7a1491c
Compare
@jieyouxu a few adjustments Given that it is possible to migration code straight from Edition 2015 to 2024, I will drop the the edition gate everywhere the feature touches, to maximise the coverage of this change on more crates. |
I suspect you'll also need to temporarily ignore a few tests so the try job can build |
This comment has been minimized.
This comment has been minimized.
the gnu-tools failure looks genuine, but the heavy macro usage makes it hard to tell the root cause |
Locally:
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
@jieyouxu Ah good ol' time. Yes, we found out this @traviscross Given that we can't really remove the edition gate on the feature, does it still make sense to do the crater run? Maybe it is just sufficient to just run the crater on the lint itself? |
If we haven't yet, it'd be definitely worth making a PR to Presumably what we'd be looking for, by enabling the behavior in all editions in this run, would be any crates that are broken but for which the lint doesn't fire. That seems worth knowing, if possible. If that's too tricky to do, though, and we're confident that the lint catches the important cases, then just doing a crater run on the lint is probably sufficient. |
@traviscross I am working on a I just got to know that I feel like that we should give the lint the go ahead first. |
@traviscross I managed to fix the crate, juhu! tokio-rs/tracing#3066 Whether this gets checked in will be at their mercy. 🤞 |
☔ The latest upstream changes (presumably #132027) made this pull request unmergeable. Please resolve the merge conflicts. |
We will run further crater run on the second version of the lint. It has been a great journey and see you all over there. Ciao! |
I would like to nominate a crater run for a build and test
build-and-test
.Through this experiment, we would like to find out the extend of breakage in the ecosystem due to the change in drop order. This will help us evaluate the migration strategy and lint level.