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

Always create elided lifetime parameters for functions #97720

Merged
merged 4 commits into from
Jul 14, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jun 3, 2022

Anonymous and elided lifetimes in functions are sometimes (async fns) --and sometimes not (regular fns)-- desugared to implicit generic parameters.

This difference of treatment makes it some downstream analyses more complicated to handle. This step is a pre-requisite to perform lifetime elision resolution on AST.

There is currently an inconsistency in the treatment of argument-position impl-trait for functions and async fns:

trait Foo<'a> {}
fn foo(t: impl Foo<'_>) {} //~ ERROR missing lifetime specifier
async fn async_foo(t: impl Foo<'_>) {} //~ OK
fn bar(t: impl Iterator<Item = &'_ u8>) {} //~ ERROR missing lifetime specifier
async fn async_bar(t: impl Iterator<Item = &'_ u8>) {} //~ OK

The current implementation reports "missing lifetime specifier" on foo, but accepts it in async_foo.
This PR proposes to accept the anonymous lifetime in both cases as an extra generic lifetime parameter.
This change would be insta-stable, so let's ping t-lang.
Anonymous lifetimes in GAT bindings keep being forbidden:

fn foo(t: impl Foo<Assoc<'_> = Bar<'_>>) {}
                         ^^        ^^
                       forbidden   ok

I started a discussion here: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Anonymous.20lifetimes.20in.20universal.20impl-trait/near/284968606

r? @petrochenkov

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 3, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 5, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Jun 5, 2022

@rust-lang/lang

This PR proposes to accept having anonymous lifetimes inside argument-position impl-trait, and to treat them as an extra generic lifetime parameter on the function.

There is currently an inconsistency in the treatment of argument-position impl-trait for functions and async fns:

trait Foo<'a> {}
fn foo(t: impl Foo<'_>) {} //~ ERROR missing lifetime specifier
async fn async_foo(t: impl Foo<'_>) {} //~ OK (since 1.39.0)

The semantics would be:

fn foo<'1, _T>(t: _T) where _T: Foo<'1> {}

Anonymous lifetimes in GAT bindings keep being forbidden:

fn foo(t: impl Foo<Assoc<'_> = Bar<'_>>) {}
                         ^^        ^^
                         ERROR     OK

I started a discussion here: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Anonymous.20lifetimes.20in.20universal.20impl-trait/near/284968606
where I expose what I understood of the anonymous lifetime resolution algorithm.

@cjgillot cjgillot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 5, 2022
@petrochenkov
Copy link
Contributor

I've added an example with '_ in associated item constraints to the PR description, otherwise it wasn't clear whether it was accepted previously or not.

src/test/ui/static/static-reference-to-fn-2.stderr Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

(I still need to review some parts of compiler/rustc_resolve/src/late.rs, will finish a bit later.)

compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov 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 Jun 5, 2022
@cjgillot cjgillot force-pushed the all-fresh branch 2 times, most recently from cacdbf7 to 5983b4a Compare June 6, 2022 07:23
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 6, 2022
@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2022
@bors

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@petrochenkov petrochenkov added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jul 2, 2022
@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 13, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2022

📌 Commit 5a20834 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2022
@@ -148,6 +148,8 @@ declare_features! (
/// below (it has to be checked before expansion possibly makes
/// macros disappear).
(active, allow_internal_unstable, "1.0.0", None, None),
/// Allows using anonymous lifetimes in argument-position impl-trait.
(active, anonymous_lifetime_in_impl_trait, "1.63.0", None, None),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or 1.64.0?

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 14, 2022
Always create elided lifetime parameters for functions

Anonymous and elided lifetimes in functions are sometimes (async fns) --and sometimes not (regular fns)-- desugared to implicit generic parameters.

This difference of treatment makes it some downstream analyses more complicated to handle.  This step is a pre-requisite to perform lifetime elision resolution on AST.

There is currently an inconsistency in the treatment of argument-position impl-trait for functions and async fns:
```rust
trait Foo<'a> {}
fn foo(t: impl Foo<'_>) {} //~ ERROR missing lifetime specifier
async fn async_foo(t: impl Foo<'_>) {} //~ OK
fn bar(t: impl Iterator<Item = &'_ u8>) {} //~ ERROR missing lifetime specifier
async fn async_bar(t: impl Iterator<Item = &'_ u8>) {} //~ OK
```

The current implementation reports "missing lifetime specifier" on `foo`, but **accepts it** in `async_foo`.
This PR **proposes to accept** the anonymous lifetime in both cases as an extra generic lifetime parameter.
This change would be insta-stable, so let's ping t-lang.
Anonymous lifetimes in GAT bindings keep being forbidden:
```rust
fn foo(t: impl Foo<Assoc<'_> = Bar<'_>>) {}
                         ^^        ^^
                       forbidden   ok
```
I started a discussion here: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Anonymous.20lifetimes.20in.20universal.20impl-trait/near/284968606

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97720 (Always create elided lifetime parameters for functions)
 - rust-lang#98315 (Stabilize `core::ffi:c_*` and rexport in `std::ffi`)
 - rust-lang#98705 (Implement `for<>` lifetime binder for closures)
 - rust-lang#99126 (remove allow(rustc::potential_query_instability) in rustc_span)
 - rust-lang#99139 (Give a better error when `x dist` fails for an optional tool)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f5e9cb5 into rust-lang:master Jul 14, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 14, 2022
@cjgillot cjgillot deleted the all-fresh branch July 14, 2022 16:47
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
Always create elided lifetime parameters for functions

Anonymous and elided lifetimes in functions are sometimes (async fns) --and sometimes not (regular fns)-- desugared to implicit generic parameters.

This difference of treatment makes it some downstream analyses more complicated to handle.  This step is a pre-requisite to perform lifetime elision resolution on AST.

There is currently an inconsistency in the treatment of argument-position impl-trait for functions and async fns:
```rust
trait Foo<'a> {}
fn foo(t: impl Foo<'_>) {} //~ ERROR missing lifetime specifier
async fn async_foo(t: impl Foo<'_>) {} //~ OK
fn bar(t: impl Iterator<Item = &'_ u8>) {} //~ ERROR missing lifetime specifier
async fn async_bar(t: impl Iterator<Item = &'_ u8>) {} //~ OK
```

The current implementation reports "missing lifetime specifier" on `foo`, but **accepts it** in `async_foo`.
This PR **proposes to accept** the anonymous lifetime in both cases as an extra generic lifetime parameter.
This change would be insta-stable, so let's ping t-lang.
Anonymous lifetimes in GAT bindings keep being forbidden:
```rust
fn foo(t: impl Foo<Assoc<'_> = Bar<'_>>) {}
                         ^^        ^^
                       forbidden   ok
```
I started a discussion here: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Anonymous.20lifetimes.20in.20universal.20impl-trait/near/284968606

r? ``@petrochenkov``
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97720 (Always create elided lifetime parameters for functions)
 - rust-lang#98315 (Stabilize `core::ffi:c_*` and rexport in `std::ffi`)
 - rust-lang#98705 (Implement `for<>` lifetime binder for closures)
 - rust-lang#99126 (remove allow(rustc::potential_query_instability) in rustc_span)
 - rust-lang#99139 (Give a better error when `x dist` fails for an optional tool)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
…henkov

Resolve function lifetime elision on the AST

~Based on rust-lang#97720

Lifetime elision for functions is purely syntactic in nature, so can be resolved on the AST.
This PR replicates the elision logic and diagnostics on the AST, and replaces HIR-based resolution by a `delay_span_bug`.

This refactor allows for more consistent diagnostics, which don't have to guess the original code from HIR.

r? `@petrochenkov`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jul 28, 2022
Resolve function lifetime elision on the AST

~Based on rust-lang/rust#97720

Lifetime elision for functions is purely syntactic in nature, so can be resolved on the AST.
This PR replicates the elision logic and diagnostics on the AST, and replaces HIR-based resolution by a `delay_span_bug`.

This refactor allows for more consistent diagnostics, which don't have to guess the original code from HIR.

r? `@petrochenkov`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language 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