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

False positive: single_use_lifetimes #77175

Closed
Nugine opened this issue Sep 25, 2020 · 8 comments · Fixed by #88650
Closed

False positive: single_use_lifetimes #77175

Nugine opened this issue Sep 25, 2020 · 8 comments · Fixed by #88650
Labels
A-async-await Area: Async & Await A-lifetimes Area: lifetime related A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Nugine
Copy link

Nugine commented Sep 25, 2020

I tried this code:

#[deny(single_use_lifetimes)]

// error: lifetime parameter `'a` only used once
async fn bar<'a>(s1: String, s2: &'_ str, s3: &'a str) -> &'a str {
    s3
}

fn foo<'a>(s1: String, s2: &'_ str, s3: &'a str) -> &'a str {
    s3
}

fn main() {}

I expected to see this happen: the code is correct.

Instead, this happened:

error: lifetime parameter `'a` only used once
 --> src/main.rs:4:14
  |
4 | async fn bar<'a>(s1: String, s2: &'_ str, s3: &'a str) -> &'a str {
  |              ^^ this lifetime...                           -- ...is used only here
  |
note: the lint level is defined here
 --> src/main.rs:1:8
  |
1 | #[deny(single_use_lifetimes)]
  |        ^^^^^^^^^^^^^^^^^^^^

Just swap the functions, then the code compiles:

#[deny(single_use_lifetimes)]

fn foo<'a>(s1: String, s2: &'_ str, s3: &'a str) -> &'a str {
    s3
}

async fn bar<'a>(s1: String, s2: &'_ str, s3: &'a str) -> &'a str {
    s3
}

fn main() {}

Update 20201014:

The examples may be misleading.
I forgot that #[deny(single_use_lifetimes)] takes effect on a single item.
It was a typo.

Here is a new example. It should be correct. But the compiler does not accept it.

#![deny(single_use_lifetimes)]

// error: lifetime parameter `'a` only used once
async fn bar<'a>(s1: String, s2: &'_ str, s3: &'a str) -> &'a str {
    s3
}

Meta

rustc --version --verbose:

rustc 1.46.0 (04488afe3 2020-08-24)
binary: rustc
commit-hash: 04488afe34512aa4c33566eb16d8c912a3ae04f9
commit-date: 2020-08-24
host: x86_64-unknown-linux-gnu
release: 1.46.0
LLVM version: 10.0

rustc +nightly --version --verbose:

rustc 1.48.0-nightly (8b4085359 2020-09-23)
binary: rustc
commit-hash: 8b4085359ae798dedb05c95ad42520557bd25320
commit-date: 2020-09-23
host: x86_64-unknown-linux-gnu
release: 1.48.0-nightly
LLVM version: 11.0
@Nugine Nugine added the C-bug Category: This is a bug. label Sep 25, 2020
@jonas-schievink jonas-schievink added A-async-await Area: Async & Await A-lifetimes Area: lifetime related A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Sep 25, 2020
@tmandry tmandry added this to On deck in wg-async work via automation Sep 29, 2020
@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Sep 29, 2020
@tmandry
Copy link
Contributor

tmandry commented Sep 29, 2020

Very strange. I'm thinking we have some stateful HIR lowering code that is hanging onto state it shouldn't.

@tmandry tmandry added the I-prioritize Indicates that prioritization has been requested for this issue label Oct 3, 2020
@camelid
Copy link
Member

camelid commented Oct 3, 2020

This bug existed as far back as November 2019 (when async/.await was stabilized), so it's not a recent regression.

@JohnTitor JohnTitor added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 3, 2020
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Oct 3, 2020

Marking this as P-medium based on the wg-prioritization discussion

@Dylan-DPC-zz Dylan-DPC-zz added P-medium Medium priority and removed I-prioritize Indicates that prioritization has been requested for this issue labels Oct 3, 2020
@sapessi
Copy link
Contributor

sapessi commented Oct 13, 2020

I did some digging into this. I'm brand new to the compiler so slowly learning the architecture along the way. Below are some thoughts on what I've seen so far.

The desugared HIR for both samples is the same:

async first

#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std;
// edition:2018

// error: lifetime parameter `'a` only used once
#[deny(single_use_lifetimes)]
async fn bar<'a, '_>(s1: String, s2: &'_ str, s3: &'a str)
 ->
     /*impl Trait*/ #[lang = "from_generator"](move |mut _task_context|
                                                   {
                                                       let s1 = s1;
                                                       let s2 = s2;
                                                       let s3 = s3;
                                                       { let _t = { s3 }; _t }
                                                   })

fn foo<'a>(s1: String, s2: &str, s3: &'a str) -> &'a str { s3 }

fn main() { }

async last

#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std;
// check-pass
// edition:2018

// error: lifetime parameter `'a` only used once
#[deny(single_use_lifetimes)]
fn foo<'a>(s1: String, s2: &str, s3: &'a str) -> &'a str { s3 }

async fn bar<'a, '_>(s1: String, s2: &'_ str, s3: &'a str)
 ->
     /*impl Trait*/ #[lang = "from_generator"](move |mut _task_context|
                                                   {
                                                       let s1 = s1;
                                                       let s2 = s2;
                                                       let s3 = s3;
                                                       { let _t = { s3 }; _t }
                                                   })

fn main() { }

As expected both resolves the same set of lifetimes (from rustc_resolve::late::lifetimes) and process them in the same order (since they are sorted). Dump from the lifetime_uses property at the first iteration:

async first

2:rustcDEBUG rustc_resolve::late::lifetimes lifetime_uses: {
	DefId(0:12 ~ issue_77175[317d]::bar::'_): Many, 
	DefId(0:4 ~ issue_77175[317d]::bar::'a): Many, 
	DefId(0:11 ~ issue_77175[317d]::bar::{opaque#0}::'_): Many, 
	DefId(0:10 ~ issue_77175[317d]::bar::{opaque#0}::'a): One(lifetime(HirId { owner: DefId(0:5 ~ issue_77175[317d]::bar::{opaque#0}), local_id: 1 }: 'a))
}

async last

2:rustcDEBUG rustc_resolve::late::lifetimes lifetime_uses: {
	DefId(0:12 ~ issue_77175_reverse[317d]::bar::'_): Many, 
	DefId(0:11 ~ issue_77175_reverse[317d]::bar::{opaque#0}::'_): Many, 
	DefId(0:6 ~ issue_77175_reverse[317d]::bar::'a): Many, 
	DefId(0:10 ~ issue_77175_reverse[317d]::bar::{opaque#0}::'a): One(lifetime(HirId { owner: DefId(0:7 ~ issue_77175_reverse[317d]::bar::{opaque#0}), local_id: 1 }: 'a))
}

Interestingly both included the 'a lifetime as used only once in the opaque/desugared async function and both trigger the lint. However, for the second case (async last) the compilation still succeeds - this is really confusing and I'm probably missing something big about the compiler architecture. Need to dig deeper.

@Nugine
Copy link
Author

Nugine commented Oct 14, 2020

The examples may be misleading.
I forgot that #[deny(single_use_lifetimes)] takes effect on a single item.
It was a typo.

Here is a new example.

#![deny(single_use_lifetimes)]

// error: lifetime parameter `'a` only used once
async fn bar<'a>(s1: String, s2: &'_ str, s3: &'a str) -> &'a str {
    s3
}

@sapessi
Copy link
Contributor

sapessi commented Oct 14, 2020

Makes sense. Then the behavior is no longer magical and makes a lot of sense.

@sapessi
Copy link
Contributor

sapessi commented Oct 15, 2020

Based on what I've seen the desugaring process is working correctly and the LifetimeContext object is keeping the correct state. The lifetime linting identifies the 'a lifetime as only used once within the return opaque type generated by the desugaring process. This behavior seems correct since we'd still want to catch this for an opaque type implementation from the user. My suggestion would be to include an exception in the linting specifically for opaque types of async functions that use a lifetime name already declared in the generic params of the parent function.

I have validated this simple change (I named the loop 'lifetimes) and propose to include it alongside the existing exception for issue 53738.

// if the current type is an opaque type that originates from an async function
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 {
            // check that the lifetimes used in the opaque type are already declared in the current scope. 
            // The defined_by map is created at the beginning of this method to collect all lifetimes in the scope
            for p in opaque.generics.params {
                if defined_by.contains_key(&p.name) {
                    continue 'lifetimes;
                }
            }
        }
    }
}

@sapessi
Copy link
Contributor

sapessi commented Oct 16, 2020

One more question for the team (@camelid, @Dylan-DPC), what would we expect in this case?

#[deny(single_use_lifetimes)]
async fn bar<'a>(s1: String, s2: &'_ str, s3: &'a str) -> String {
    s3.to_owned()
}

This compiles successfully, which is counter intuitive by looking at the function signature. It compiles because the desugared generator uses the same lifetime as the async function.

sapessi added a commit to sapessi/rust that referenced this issue Sep 4, 2021
As reported in issue rust-lang#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.
sapessi added a commit to sapessi/rust that referenced this issue Sep 4, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 5, 2021
Skip single use lifetime lint for generated opaque types

Fix: rust-lang#77175

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
@bors bors closed this as completed in 23afad6 Sep 18, 2021
wg-async work automation moved this from On deck to Done Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-lifetimes Area: lifetime related A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
7 participants