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

Suggest using anonymous lifetime in `impl Trait` return #58919

Merged
merged 2 commits into from Apr 1, 2019

Conversation

Projects
None yet
6 participants
@estebank
Copy link
Contributor

commented Mar 4, 2019

@estebank

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

This is a blatant hack, but wanted to have a PR out so we can talk about the desired output.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Hmm that output is interesting.

I wonder, as a matter of coding style and/or readability, if people would prefer to see the following suggested:

 fn elided<'a>(x: &'a i32) -> impl Copy + 'a { x }

(but I don't know, I think I often err on the side of explicitness when people would prefer elision...)

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Oh; I guess that test case, in its full form, even includes an example where we do suggest the form with the explicit named lifetime, if you start with fn elided<'a>(x: &'a i32) -> impl Copy { x }.

So its really a matter of style, and I suspect the prevailing style is towards what @estebank is proposing here

@estebank

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

We can expand this code to also suggest

help: you can add an explicit named lifetime `'a` to the type of `x` and the return type
   |
LL | fn elided<'a>(x: &'a i32) -> impl Copy + 'a { x }
   |          ^^^^     ^^                   ^^^^

but the code itself to make that suggestion might be quite brittle.

@Centril

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

I think the form suggested here is quite reasonable and I suspect doing the more elaborate suggestion is more complex impl wise...

To reduce Niko's backlog, r? @pnkfelix

@Centril

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Ping from triage, @pnkfelix -- shall we go with @estebank's idea for now?

Suggest using anonymous lifetime in `impl Trait` return without hacks
Fallback to `static_impl_trait` for nice error message by peeking at the
return type and the lifetime type. Point at the return type instead of
the return expr/stmt in NLL mode.

@estebank estebank force-pushed the estebank:impl-trait-return-lifetime branch from 68c28b6 to 30c247f Mar 31, 2019

@estebank

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

I had a talk with Niko and he clarified what a better approach would be.

I looked around and arrived to the current solution, with which I'm relatively happy with (even though I think we could improve the static_impl_trait output in a separate PR). I feel that this PR is now reasonable/reviewable/mergeable.

|
LL | fn elided(x: &i32) -> impl Copy { x }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: you can add a constraint to the return type to make it last less than `'static` and match the anonymous lifetime #1 defined on the function body at 3:1

This comment has been minimized.

Copy link
@Centril

Centril Mar 31, 2019

Contributor

Btw... "constraint" here is odd; we typically use "bound" for things in A + B + ....

This comment has been minimized.

Copy link
@estebank

estebank Mar 31, 2019

Author Contributor

We've been using that wording for 6 months, but I agree we could improve on the wording on these errors.

@@ -84,6 +86,13 @@ impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
{
return None;
}
if let FunctionRetTy::Return(ty) = &fndecl.output {
if let (TyKind::Def(_, _), ty::ReStatic) = (&ty.node, sub) {
// This is an impl Trait return that evaluates de need of 'static.

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Apr 1, 2019

Member

"evaluates de need of 'static" ?

What is this?

This comment has been minimized.

Copy link
@estebank

estebank Apr 1, 2019

Author Contributor

A partial rewording of the prior comment 😬

I believe something along the lines of "that evaluates to a need of static".

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

📌 Commit 30c247f has been approved by pnkfelix

Centril added a commit to Centril/rust that referenced this pull request Apr 1, 2019

Rollup merge of rust-lang#58919 - estebank:impl-trait-return-lifetime…
…, r=pnkfelix

Suggest using anonymous lifetime in `impl Trait` return

Fix rust-lang#48467.

r? @nikomatsakis

@Centril Centril referenced this pull request Apr 1, 2019

Closed

Rollup of 5 pull requests #59602

bors added a commit that referenced this pull request Apr 1, 2019

Auto merge of #59602 - Centril:rollup, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #58919 (Suggest using anonymous lifetime in `impl Trait` return)
 - #59041 (fixes #56766)
 - #59586 (Fixed URL in cargotest::TEST_REPOS)
 - #59593 (Print the appveyor agent version at the start of the build)
 - #59601 (Fix small typo)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Apr 1, 2019

Rollup merge of rust-lang#58919 - estebank:impl-trait-return-lifetime…
…, r=pnkfelix

Suggest using anonymous lifetime in `impl Trait` return

Fix rust-lang#48467.

r? @nikomatsakis

@Centril Centril referenced this pull request Apr 1, 2019

Merged

Rollup of 7 pull requests #59606

bors added a commit that referenced this pull request Apr 1, 2019

Auto merge of #59606 - Centril:rollup, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #58507 (Add a -Z time option which prints only passes which runs once)
 - #58919 (Suggest using anonymous lifetime in `impl Trait` return)
 - #59041 (fixes #56766)
 - #59586 (Fixed URL in cargotest::TEST_REPOS)
 - #59595 (Update rustc-guide submodule)
 - #59601 (Fix small typo)
 - #59603 (stabilize ptr::hash)

Failed merges:

r? @ghost

@bors bors merged commit 30c247f into rust-lang:master Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.