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

Add extensive set of drop order tests #133605

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

traviscross
Copy link
Contributor

@traviscross traviscross commented Nov 29, 2024

On lang, we've recently been discussing the drop order with respect to let chains apropos of how we shortened temporary lifetimes in Rust 2024 and how we may shorten them further in the future.

Here we add an extensive set of tests that demonstrate the drop order in the cases that interest us.

Regarding the let chains stabilization prompting this analysis, see:

r? ghost

cc @ehuss @dingxiangfei2009 @nikomatsakis

@traviscross traviscross added F-let_chains `#![feature(let_chains)]` A-edition-2024 Area: The 2024 edition L-tail_expr_drop_order Lint: tail_expr_drop_order L-if_let_rescope Lint: if_let_rescope labels Nov 29, 2024
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 29, 2024
@traviscross
Copy link
Contributor Author

traviscross commented Nov 29, 2024

@dingxiangfei2009: This set of tests fails on run-rustfix due to the machine-applicable lints not successfully migrating all the code. Is this expected, and is there anything that might be done?

@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross force-pushed the TC/add-2024-drop-order-tests branch 8 times, most recently from 16cf80f to 9db5de5 Compare December 1, 2024 02:25
@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross force-pushed the TC/add-2024-drop-order-tests branch from 9db5de5 to 4a98806 Compare December 1, 2024 08:59
@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross force-pushed the TC/add-2024-drop-order-tests branch from 4a98806 to a967277 Compare December 1, 2024 11:23
@traviscross
Copy link
Contributor Author

Originally, this had...

#![cfg_attr(e2021, warn(rust_2024_compatibility))]

...but this was causing a different number of warnings to be generated in the CI llvm-18 test from what I could reproduce (or bless) in testing locally. It was a "nice to have", so I'm removed it for now to make this PR ready and to land these tests.

Similarly, this originally had...

//@ [e2021] run-rustfix

...but the automated lints don't migrate all the code, so it fails since there are still diagnostics emitted in the fixed version. It too was a "nice to have", so I've similarly removed it.

Both of these could be added back in separate PRs later.

@traviscross
Copy link
Contributor Author

r? @compiler-errors

@traviscross traviscross marked this pull request as ready for review December 2, 2024 19:39
@dingxiangfei2009
Copy link
Contributor

@traviscross They did not apply intentionally because migrating spans inside macro is considered "risky" and error-prone. The suggestion might apply but we won't know for sure if the macro would do something entirely different. I hope this still remains a reasonable compromise.

@traviscross traviscross marked this pull request as draft December 5, 2024 00:15
@traviscross traviscross force-pushed the TC/add-2024-drop-order-tests branch from a967277 to 5354ea4 Compare December 5, 2024 00:41
@rust-log-analyzer

This comment has been minimized.

@traviscross
Copy link
Contributor Author

traviscross commented Dec 5, 2024

@dingxiangfei2009: Thanks. I've updated the test to remove use of cfg_match!. Combined with rustfix-only-machine-applicable, that solves the run-rustfix issue.

One last issue:

My local build produces 15 warnings, and so that's what I get when blessing the test. But the CI sees 10 warnings. This is fully rebased, and using stage 2 to run the test. Any idea what might be causing this?

@traviscross
Copy link
Contributor Author

Specifically, the CI seems to not be emitting the tail_expr_drop_order warnings.

@dingxiangfei2009
Copy link
Contributor

dingxiangfei2009 commented Dec 6, 2024

CI tests enables a --pass=test flag to the compiletest. Without this flag, tail_expr_drop_order is properly linting. What confuses me at the moment is that essentially both if_let_rescope and tail_expr_drop_order use the same mechanism to decide the level.

I am investigating.

@dingxiangfei2009
Copy link
Contributor

dingxiangfei2009 commented Dec 8, 2024

The --pass flag tries to override the test mode, say from a //@ run-pass test to a check test for instance, when applies.

I have a theory.

The test mode only changes when --pass=check is passed to ./x.py and eventually to compiletest. What this flag means to compiletest is that --emit=metadata is passed to rustc invocations. This happens in TestCx::should_emit_metadata. For this test to effect, the compilation pipeline needs to progress past the borrow-checking phase. Could this option inadvertently suppresses the lints because it bails out the compilation early?

Follow-up: Yes, dropping --emit=metadata gets us back the missing warnings.

@traviscross
Copy link
Contributor Author

@jieyouxu: What do you think about the situation above with respect to compiletest?

@jieyouxu
Copy link
Member

I'll take a look in a bit

@jieyouxu jieyouxu self-assigned this Dec 12, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Dec 12, 2024

@dingxiangfei2009 your analysis is spot on.
@traviscross so usually build-pass and run-pass tests are also expected to check-pass, but in this scenario the lint has some specific behavior which is only available past --emit=metadata. This means that the emission will not be available if it's limited to check-pass a.k.a. --emit=metadata, if forcefully requested on a CI job via --pass=check.

You can use

// Lints are emitted past the borrow-checking phase, i.e. unavailable in
// `--pass=check`.
//@ ignore-pass

to instruct compiletest to not respect --pass=check for this test.

@jieyouxu jieyouxu removed their assignment Dec 12, 2024
@traviscross traviscross force-pushed the TC/add-2024-drop-order-tests branch 4 times, most recently from 3954148 to 4620f47 Compare December 18, 2024 00:58
@rust-log-analyzer

This comment was marked as resolved.

@traviscross traviscross force-pushed the TC/add-2024-drop-order-tests branch from 4620f47 to dabe224 Compare December 18, 2024 03:58
@traviscross traviscross marked this pull request as ready for review December 18, 2024 05:08
On lang, we've recently been discussing the drop order with respect to
`let` chains apropos of how we shortened temporary lifetimes in Rust
2024 and how we may shorten them further in the future.

Here we add an extensive set of tests that demonstrate the drop order
in the cases that interest us.
@traviscross traviscross force-pushed the TC/add-2024-drop-order-tests branch from dabe224 to 035889d Compare December 18, 2024 05:14
@traviscross
Copy link
Contributor Author

@rustbot ready

@esote
Copy link

esote commented Jan 5, 2025

Hi, it looks like this PR is only waiting on review at this point, or is it blocked on something? Just checking in since I'd love to see #132833 make it into the 2024 edition (if that's still the plan)

@est31
Copy link
Member

est31 commented Jan 5, 2025

@esote yes I think this is blocked on review only now. But there is other real blockers for #132833 as well. The 2024 edition will ship without let chains (it's less than 7 weeks until when it's released), but the breaking changes we need are already in place so we'll just stabilize whenever the blockers unblock (and then on edition 2024+ only).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition F-let_chains `#![feature(let_chains)]` L-if_let_rescope Lint: if_let_rescope L-tail_expr_drop_order Lint: tail_expr_drop_order S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants