[WIP] Skip single use lifetime lint for generated opaque types #78004

sapessi · 2020-10-16T03:14:04Z

The opaque type generated by the desugaring process of an async function uses the lifetimes defined by the originating function. The DefId for the lifetimes in the opaque type are different from the ones in the originating async function - as they should be, as far as I understand, and could therefore be considered a single use lifetimes, this causes the single_use_lifetimes lint to fail compilation if explicitly denied. This fix skips the lint for lifetimes used only once in generated opaque types for an async function that are declared in the parent async function definition.

More info in the comments on the original issue: 1 and 2

rust-highfive · 2020-10-16T03:14:07Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

sapessi · 2020-10-16T16:13:12Z

I'm working on adding more tests for different cases

camelid · 2020-10-17T18:41:30Z

@sapessi since this is WIP, do you want to mark it as a draft PR?

sapessi · 2020-10-17T18:55:00Z

@sapessi since this is WIP, do you want to mark it as a draft PR?

Yes please. I thought it was final but then as I was working I realized that this opened a larger question (see my last comment in the issue). There are cases where the lifetime only appears once in the async method signature but the linter does not throw the error because the lifetime is reused in the desugared generator - the behavior could be counter-intuitive for developers who do not see the desugared hir. We could treat the desugared opaque type for async methods as a completely different scope as far as lifetime linting is concerned and prevent a single-use lifetime from halting compilation since this code is outside of developers' control

camelid · 2020-10-17T18:57:19Z

I'm not able to mark the PR as draft; you will have to do it. The button for it should be in the top-right, right below "Reviewers". For example:

camelid · 2020-10-17T18:58:12Z

Oops, sorry about that! Apparently I can.

Old error output: = note: this warning originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info) help: wrap this expression in parentheses | 4 | break '_l $f(;) | ^ ^ New error output: = note: this warning originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info) help: wrap this expression in parentheses | 4 | break '_l ($f); | ^ ^

Forbid inline const block referencing params from being used in patterns Fix #82518

Old error output: 3 | let _: usize = $f; | ----- ^ expected `usize`, found struct `Baz` | | | expected due to this New error output: 3 | let _: usize = $f; | ----- ^^ expected `usize`, found struct `Baz` | | | expected due to this

Make `-Z gcc-ld=lld` work for Apple targets `-Z gcc-ld=lld` was introduced in #85961. It does not work on Macos because lld needs be either named `ld64` or passed `-flavor darwin` as the first two arguments in order to select the Mach-O flavor. Rust invokes cc (=clang) on Macos for linking which calls `ld` as linker binary and not `ld64`, so just creating an `ld64` binary and modifying the search path with `-B` does not work. In order to solve this patch does: * Set the `lld_flavor` for all Apple-derived targets to `LldFlavor::Ld64`. As far as I can see this actually works towards fixing `-Xlinker=rust-lld` as all those targets use the Mach-O object format. * Copy/hardlink rust-lld to the gcc-ld subdirectory as ld64 next to ld. * If `-Z gcc-ld=lld` is used and the target lld flavor is Ld64 add `-fuse-ld=/path/to/ld64` to the linker invocation. Fixes #86945.

Update the stdarch submodule This notably brings in a number of codegen updates to ensure that wasm simd intrinsics generate the expected instruction with LLVM 13

When calling a method requiring a mutable self borrow on an inmutable to a mutable borrow of the type, suggest making the binding mutable. Fix #83241.

Treat types in unnormalized function signatures as well-formed Fixes #87748 r? `@nikomatsakis`

ast_lowering: Introduce `lower_span` for catching all spans entering HIR This PR cherry-picks the `fn lower_span` change from #84373. I also introduced `fn lower_ident` for lowering spans in identifiers, and audited places where HIR structures with spans or identifiers are constructed and added a few missing `lower_span`s/`lower_ident`s. Having a hook for spans entering HIR can be useful for things other than #84373, e.g. #35148. I also want to check whether this change causes perf regressions due to some accidental inlining issues. r? `@cjgillot`

Refactor unsized suggestions `@rustbot` label +A-diagnostics +A-traits +A-typesystem +C-cleanup +T-compiler

Add an example for deriving PartialOrd on enums For some reason, I always forget which variants are smaller and which are larger when you derive PartialOrd on an enum. And the wording in the current docs is not entirely clear to me. So, I often end up making a small enum, deriving PartialOrd on it, and then writing a `#[test]` with an assert that the top one is smaller than the bottom one (or the other way around) to figure out which way the deriving goes. So then I figured, it would be great if the standard library docs just had that example, so if I keep forgetting, at least I can figure it out quickly by looking at std's docs.

…=petrochenkov Fix LLVM libunwind build for non-musl targets Broken in #85600. AFAICT, [only musl, mingw, and wasm](https://github.com/rust-lang/rust/blob/673d0db5e393e9c64897005b470bfeb6d5aec61b/compiler/rustc_target/src/spec/crt_objects.rs#L128-L132) should use the “self-contained” logic in rustbuild.

…Jung Add test case for using `slice::fill` with MaybeUninit Adds test for #87891 Looks alright? `@RalfJung` Fixes #87891

small const generics cleanup

remove redundant / misplaced sentence from docs Removes sentence that seems to have drifted down into the examples section. Luckily, someone already added an explanation of what happens with packed structs back into the initial section of the doc entry and this wayward sentence can likely just be deleted.

Update outdated docs of array::IntoIter::new.

Update primitive docs for rust 2021. Fixes #87701

Rollup of 7 pull requests Successful merges: - #88202 (Add an example for deriving PartialOrd on enums) - #88483 (Fix LLVM libunwind build for non-musl targets) - #88507 (Add test case for using `slice::fill` with MaybeUninit) - #88557 (small const generics cleanup) - #88579 (remove redundant / misplaced sentence from docs) - #88610 (Update outdated docs of array::IntoIter::new.) - #88613 (Update primitive docs for rust 2021.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Move global analyses from lowering to resolution Split off #87234 r? `@petrochenkov`

sunos systems add sanitizer supported.

estebank

The changes look reasonable to me! Do you still want to tackle this?

Left some nitpicks to reduce righwards drift of the code.

estebank · 2021-09-03T18:03:58Z

compiler/rustc_resolve/src/late/lifetimes.rs

+                                // opaque types generated when desugaring an async function can have a single
+                                // use lifetime even if it is explicitly denied (Issue #77175)
+                                if let hir::Node::Item(item) = self.tcx.hir().get(parent_hir_id) {
+                                    if let hir::ItemKind::OpaqueTy(ref opaque) = item.kind {
+                                        if opaque.origin == hir::OpaqueTyOrigin::AsyncFn {
+                                            // We want to do this only if the liftime identifier is already defined
+                                            // in the async function that generated this. Otherwise it could be
+                                            // an opaque type defined by the developer and we still want this
+                                            // lint to fail compilation
+                                            for p in opaque.generics.params {
+                                                if defined_by.contains_key(&p.name) {
+                                                    continue 'lifetimes;
+                                                }
+                                            }
+                                        }
+                                    }
+                                }


Suggested change

// opaque types generated when desugaring an async function can have a single

// use lifetime even if it is explicitly denied (Issue #77175)

if let hir::Node::Item(item) = self.tcx.hir().get(parent_hir_id) {

if let hir::ItemKind::OpaqueTy(ref opaque) = item.kind {

if opaque.origin == hir::OpaqueTyOrigin::AsyncFn {

// We want to do this only if the liftime identifier is already defined

// in the async function that generated this. Otherwise it could be

// an opaque type defined by the developer and we still want this

// lint to fail compilation

for p in opaque.generics.params {

if defined_by.contains_key(&p.name) {

continue 'lifetimes;

}

}

}

}

}

// opaque types generated when desugaring an async function can have a single

// use lifetime even if it is explicitly denied (Issue #77175)

if let hir::Node::Item(hir::Item {

kind: hir::ItemKind::OpaqueTy(hir::OpaqueTy {

origin: hir::OpaqueTyOrigin::AsyncFn,

..

}),

..

}) = self.tcx.hir().get(parent_hir_id) {

// We want to do this only if the liftime identifier is already defined

// in the async function that generated this. Otherwise it could be

// an opaque type defined by the developer and we still want this

// lint to fail compilation

for p in opaque.generics.params {

if defined_by.contains_key(&p.name) {

continue 'lifetimes;

}

}

}

Taken your suggestion with some minor changes because I need the opaque type while trying to prevent rightwards drift

estebank · 2021-09-03T18:04:36Z

src/test/ui/lifetimes/issue-77175.rs

+// edition:2018
+// check-pass
+
+// error: lifetime parameter `'a` only used once


Change this to make it clear that this was the previously emitted error that shouldn't happen.

Added more comprehensive comment

estebank · 2021-09-03T18:07:47Z

compiler/rustc_resolve/src/late/lifetimes.rs

+                                            // in the async function that generated this. Otherwise it could be
+                                            // an opaque type defined by the developer and we still want this
+                                            // lint to fail compilation
+                                            for p in opaque.generics.params {


I wonder if it makes sense to build an FxHashSet from these to make the amortized cost linear, but that's likely gold-plating.

You mean storing the parameters for the Generic type in an FxHashSet instead of an array?

sapessi · 2021-09-03T19:20:54Z

The changes look reasonable to me! Do you still want to tackle this?

Left some nitpicks to reduce righwards drift of the code.

Apologies for the radio silence - life and work have been pretty crazy. I would love to pick this up again. I'll be back online in a couple of weeks and I'll address your feedback, @estebank. Thank you!

Fix drop handling for `if let` expressions MIR lowering for `if let` expressions is now more complicated now that `if let` exists in HIR. This PR adds a scope for the variables bound in an `if let` expression and then uses an approach similar to how we handle loops to ensure that we reliably drop the correct variables. Closes #88307 cc `@flip1995` `@richkadel` `@c410-f3r`

Add regression test for a spurious import This PR adds a test that verifies that the bug described in the linked issue does not creep back into the code. In essence it checks that compiling some specific code (that uses 128 bit multiplication) with a specific set of compiler options does not lead to a spurious import of a panic function. I noticed that other wasm tests use `# only-wasm32-bare` in their `Makefile`. This will skip the test for me. I did not find out how to run this test locally. Maybe someone can help. closes #78744 r? `@jyn514`

As reported in issue #77175, the opaque type generated by the desugaring process of an async function uses the lifetimes defined by the originating function. The definition ID for the lifetimes in the opaque method is different from the one in the originating async function and it could therefore be considered a single use of the lifetimne, this causes the single_use_lifetimes lint to fail compilation if explicitly denied. This fix skips the lint for lifetimes used only once in generated opaque types for an async function that are declared in the parent async function definition

To add historical context on the issue

to prevent rightwards drift of the code

camelid · 2021-09-04T18:01:19Z

@sapessi rust-lang/rust follows a "No-Merge Policy", so please rebase over master instead of merging it in. Thanks!

camelid · 2021-09-04T18:01:39Z

Also, it seems like Git got confused and thinks this PR has 10,000 commits. I think rebasing may fix that.

sapessi · 2021-09-04T18:03:17Z

Also, it seems like Git got confused and thinks this PR has 10,000 commits. I think rebasing may fix that.

Yup, just realized this happened, must have forgotten the --ff-only at some point. I'll just recreate my branch and PR. Easier than fixing it probably since the changes are so small

camelid · 2021-09-04T18:06:02Z

You could also git rebase upstream/master. It may fix the problem, though I'm not certain. Or you could just create a new branch and cherry-pick as you mention.

sapessi · 2021-09-04T18:13:43Z

Attempted to rebase from upstream. It's gotten really confused. I cannot figure out where it thinks those 1K commits came from. Easier to start from scratch. I'll reference the new PR from here

sapessi · 2021-09-04T18:53:02Z

New PR is #88650. Closing this one and apologies for the git mess

rust-highfive assigned matthewjasper Oct 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2020

sapessi changed the title ~~Skip single use lifetime lint for generated opaque types~~ [WIP] Skip single use lifetime lint for generated opaque types Oct 16, 2020

camelid added A-lifetimes Area: lifetime related A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. labels Oct 17, 2020

camelid marked this pull request as draft October 17, 2020 18:57

matthewjasper added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2020

roxelo and others added 15 commits August 28, 2021 19:54

Move match tests in match folder

33817d2

Auto merge of #88088 - nbdd0121:const2, r=nagisa

3a21a5b

Forbid inline const block referencing params from being used in patterns Fix #82518

Fix: don't document private macros by default

3ee6d0b

Auto merge of #88295 - alexcrichton:update-stdarch, r=kennytm

66acdee

Update the stdarch submodule This notably brings in a number of codegen updates to ensure that wasm simd intrinsics generate the expected instruction with LLVM 13

ast_lowering: Introduce lower_span for catching all spans entering HIR

59013cd

Add regression test

6aacbd8

Suggestion for call on immutable binding of mutable type

e4368de

When calling a method requiring a mutable self borrow on an inmutable to a mutable borrow of the type, suggest making the binding mutable. Fix #83241.

Provide structured suggestion for removal of &mut

a6af6d6

rebase: fix test output

5b6f4b9

Auto merge of #88312 - jackh726:issue-87748, r=nikomatsakis

59ce765

Treat types in unnormalized function signatures as well-formed Fixes #87748 r? `@nikomatsakis`

Use the correct type for Enum variant tuples

ed30993

bors and others added 13 commits September 3, 2021 08:51

Auto merge of #86454 - tlyu:refactor-unsized-suggestions, r=davidtwco

e4e4179

Refactor unsized suggestions `@rustbot` label +A-diagnostics +A-traits +A-typesystem +C-cleanup +T-compiler

Update outdated docs of array::IntoIter::new.

5ea45f3

Update primitive docs for rust 2021.

00c8da1

Rollup merge of #88507 - atsuzaki:slice-fill-maybeuninit-test, r=Ralf…

80b572b

…Jung Add test case for using `slice::fill` with MaybeUninit Adds test for #87891 Looks alright? `@RalfJung` Fixes #87891

Rollup merge of #88557 - lcnr:const-generics-cleanup, r=BoxyUwU

4b5da4a

small const generics cleanup

Rollup merge of #88610 - m-ou-se:array-into-iter-docs, r=Amanieu

e13b9c9

Update outdated docs of array::IntoIter::new.

Rollup merge of #88613 - m-ou-se:array-docs-2021, r=Amanieu

2ce74b0

Update primitive docs for rust 2021. Fixes #87701

Auto merge of #88597 - cjgillot:lower-global, r=petrochenkov

577a76f

Move global analyses from lowering to resolution Split off #87234 r? `@petrochenkov`

Auto merge of #88454 - devnexen:sunos_asan, r=wesleywiser

4878034

sunos systems add sanitizer supported.

estebank reviewed Sep 3, 2021

View reviewed changes

Update coverage tests

4e2fd4f

bors and others added 6 commits September 3, 2021 20:31

Updated test comment

30f2695

To add historical context on the issue

Address CR comments

ce33e77

to prevent rightwards drift of the code

Merge local branch with master

fc24ad0

sapessi mentioned this pull request Sep 4, 2021

Skip single use lifetime lint for generated opaque types #88650

Merged

sapessi closed this Sep 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Skip single use lifetime lint for generated opaque types #78004

[WIP] Skip single use lifetime lint for generated opaque types #78004

sapessi commented Oct 16, 2020 •

edited

Loading

rust-highfive commented Oct 16, 2020

sapessi commented Oct 16, 2020

camelid commented Oct 17, 2020

sapessi commented Oct 17, 2020

camelid commented Oct 17, 2020

camelid commented Oct 17, 2020 •

edited

Loading

estebank left a comment

estebank Sep 3, 2021

sapessi Sep 4, 2021

estebank Sep 3, 2021

sapessi Sep 4, 2021

estebank Sep 3, 2021

sapessi Sep 4, 2021 •

edited

Loading

sapessi commented Sep 3, 2021

camelid commented Sep 4, 2021

camelid commented Sep 4, 2021 •

edited

Loading

sapessi commented Sep 4, 2021

camelid commented Sep 4, 2021

sapessi commented Sep 4, 2021

sapessi commented Sep 4, 2021

[WIP] Skip single use lifetime lint for generated opaque types #78004

[WIP] Skip single use lifetime lint for generated opaque types #78004

Conversation

sapessi commented Oct 16, 2020 • edited Loading

rust-highfive commented Oct 16, 2020

sapessi commented Oct 16, 2020

camelid commented Oct 17, 2020

sapessi commented Oct 17, 2020

camelid commented Oct 17, 2020

camelid commented Oct 17, 2020 • edited Loading

estebank left a comment

Choose a reason for hiding this comment

estebank Sep 3, 2021

Choose a reason for hiding this comment

sapessi Sep 4, 2021

Choose a reason for hiding this comment

estebank Sep 3, 2021

Choose a reason for hiding this comment

sapessi Sep 4, 2021

Choose a reason for hiding this comment

estebank Sep 3, 2021

Choose a reason for hiding this comment

sapessi Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

sapessi commented Sep 3, 2021

camelid commented Sep 4, 2021

camelid commented Sep 4, 2021 • edited Loading

sapessi commented Sep 4, 2021

camelid commented Sep 4, 2021

sapessi commented Sep 4, 2021

sapessi commented Sep 4, 2021

sapessi commented Oct 16, 2020 •

edited

Loading

camelid commented Oct 17, 2020 •

edited

Loading

sapessi Sep 4, 2021 •

edited

Loading

camelid commented Sep 4, 2021 •

edited

Loading