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 long description and test for E0311 #100747

Merged

Conversation

MatthewPeterKelly
Copy link
Contributor

Adds a long description and unit test for the E0311 compiler error.

Fixes one line-item in #61137.

Adds a long description and unit test for the E0311 compiler error.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 19, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2022

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2022
@rust-log-analyzer

This comment has been minimized.

Comment on lines 1 to 2
E0311 occurs when there is insufficient information for the rust compiler to
prove that some time has a long enough lifetime.
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the contributions! This explanation could be a bit confusing, I don't know how it should be reworded into, cc @compiler-errors any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point -- it would be great to have a better description here. I'm still pretty new to rust, so this was my initial attempt at reverse engineering why the fix worked.

Copy link
Contributor Author

@MatthewPeterKelly MatthewPeterKelly left a comment

Choose a reason for hiding this comment

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

Thanks for the quick review. I applied your suggestions and we can wait to see if we get an improved description of the error message from the compiler team.

Comment on lines 1 to 2
E0311 occurs when there is insufficient information for the rust compiler to
prove that some time has a long enough lifetime.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point -- it would be great to have a better description here. I'm still pretty new to rust, so this was my initial attempt at reverse engineering why the fix worked.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

I'm okay with the current description with the suggestions applied. A more accurate description could be a followup.


In this example we have a trait that borrows some inner data element of type `V`
from an outer type `T`, through an intermediate type `U`. The compiler is unable
to prove that the livetime of `U` is long enough to support the reference. To
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to prove that the livetime of `U` is long enough to support the reference. To
to prove that the lifetime of `U` is long enough to support the reference. To

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@@ -0,0 +1,49 @@
This error occurs when there is insufficient information for the rust compiler
to prove that some time has a long enough lifetime.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to prove that some time has a long enough lifetime.
to prove that a type has a long enough lifetime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@MatthewPeterKelly
Copy link
Contributor Author

This explanation could be a bit confusing, I don't know how it should be reworded into, cc @compiler-errors any ideas?

I made an attempt at improving the description for the E0311.md error message example, based on reading the relevant chapter in the rust book again. The updated description includes some more details about lifetimes and lifetime elision rules, and how they apply to this specific example. There are still some minor details to clean-up (I've added in-line comments to the PR) out, but I think it is getting closer to something that should provide more useful context to a user. I can make another pass at cleaning up the details in the next few days.

@fee1-dead
Copy link
Member

fee1-dead commented Aug 23, 2022

Let's do a rotation and find another reviewer who might have a better idea for the documentation :)

r? rust-lang/compiler

@rust-highfive rust-highfive assigned lcnr and unassigned fee1-dead Aug 23, 2022
Comment on lines 1 to 2
This error occurs when there is insufficient information for the rust compiler
to prove that a type has a long enough lifetime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This error occurs when there is insufficient information for the rust compiler
to prove that a type has a long enough lifetime.
This error occurs when there is an unsatisfied outlives bound on a generic type parameter or associated type.

something like this maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - that's much better.

-- update summary based on review
-- rewrite explanation to be more clear and correct
@@ -0,0 +1,70 @@
This error occurs when there is insufficient information for the rust compiler
to prove that a type has a long enough lifetime.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that type would be more correct as parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 31 to 33
trait for a type `T`, so the `&mut self` reference has the lifetime of `T`.
We know that `T` implements the `BorrowMut` trait returning a reference to `U`,
and that `U` implements the `BorrowMut` trait returning a reference to `V`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't quite right either: T is a type and cannot have a lifetime. An object of type T, such as self can have a lifetime.

Really what we have is more like:

  • suppose that self has a lifetime of 'b
  • the output object of type V has a lifetime of 'c

If we split up the lines in the example we get:

        let u_ref = self.borrow_mut();  // u_ref has lifetime `'b` matching `self`
        u_ref.borrow_mut() // u_ref has lifetime of `'c` matching the output    

The compiler problem then boils to do failing to prove that 'c will be longer than 'b.

Perhaps the right approach here is to copy the failing example above and then add the lifetime annotations that the compiler attempts to add implicitly, similar to how they are shown in the examples in https://doc.rust-lang.org/book/ch10-03-lifetime-syntax.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote this to be more clear and correct.

Comment on lines 1 to 2
This error occurs when there is insufficient information for the rust compiler
to prove that a type has a long enough lifetime.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - that's much better.

@lcnr
Copy link
Contributor

lcnr commented Sep 5, 2022

@bors delegate+

@bors
Copy link
Contributor

bors commented Sep 5, 2022

✌️ @MatthewPeterKelly can now approve this pull request

Comment on lines 29 to 30
compiler would have added an implied bound [implied
bound](https://rust-lang.github.io/rfcs/2089-implied-bounds.html), causing this
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
compiler would have added an implied bound [implied
bound](https://rust-lang.github.io/rfcs/2089-implied-bounds.html), causing this
compiler would have added an
[implied bound](https://rust-lang.github.io/rfcs/2089-implied-bounds.html), causing this

oh wow, we really don't have a user facing doc for implied bounds xx that sucks

the rfc is about implied trait bounds. implied bounds already existed before that and are stable, unlike this rfc. We should really have a page in the reference for that

Copy link
Contributor

Choose a reason for hiding this comment

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

opened rust-lang/reference#1261 so maybe we should wait for that to land so that you can point there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for opening the PR to add the reference material. I added some comments on it and will hold this PR until that one lands.

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Sep 12, 2022

marking as blocked on rust-lang/reference#1261

@lcnr lcnr added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2022
outlive `'anon` in `no_restriction()`.

If `no_restriction()` were to use `&T` instead of `&()` as an argument, the
compiler would have added an implied bound [implied
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
compiler would have added an implied bound [implied
compiler would have added an [implied

(Remove the double "implied bound")

Alternatively, remove the link for now, and then send a new PR when the reference PR lands. I don't think it's worth blocking on the reference PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had figured that updating the docs would be quick, but it looks like it's going to be a more involved process. For now I removed the implied bounds link and will plan to merge this PR. Once rust-lang/reference#1261 has been merged I'll plan to open a smaller PR to add the reference link into the E0311.md docs.

@MatthewPeterKelly MatthewPeterKelly force-pushed the mpk/add-long-error-message-for-E0311 branch from df4dc2b to 0d9c014 Compare September 27, 2022 00:53
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

2 nits, then r=me

@@ -0,0 +1,42 @@
This error occurs when there is an unsatisfied outlives bound involving an
elided region on a generic type parameter or associated type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
elided region on a generic type parameter or associated type.
elided region and a generic type parameter or associated type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

lifetime `'anon` in `no_restriction()`. The only information available to the
compiler is that `'anon` is valid for the duration of the function. When
calling `with_restriction()`, the compiler requires the completely unrelated
type parameter `T` to outlive `'anon` because of the `T: 'a bound` in
Copy link
Contributor

Choose a reason for hiding this comment

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

still relevant

@lcnr
Copy link
Contributor

lcnr commented Sep 27, 2022

@bors delegate+

@bors
Copy link
Contributor

bors commented Sep 27, 2022

✌️ @MatthewPeterKelly can now approve this pull request

@MatthewPeterKelly
Copy link
Contributor Author

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 28, 2022

📌 Commit c0d32fd has been approved by MatthewPeterKelly

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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Sep 28, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#100747 (Add long description and test for E0311)
 - rust-lang#102232 (Stabilize bench_black_box)
 - rust-lang#102288 (Suggest unwrapping `???<T>` if a method cannot be found on it but is present on `T`.)
 - rust-lang#102338 (Deny associated type bindings within associated type bindings)
 - rust-lang#102347 (Unescaping cleanups)
 - rust-lang#102348 (Tweak `FulfillProcessor`.)
 - rust-lang#102378 (Use already resolved `self_ty` in `confirm_fn_pointer_candidate`)
 - rust-lang#102380 (rustdoc: remove redundant mobile `.source > .sidebar` CSS)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 49bc668 into rust-lang:master Sep 28, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 28, 2022
@MatthewPeterKelly MatthewPeterKelly deleted the mpk/add-long-error-message-for-E0311 branch September 28, 2022 09:08
@klensy
Copy link
Contributor

klensy commented Sep 28, 2022

Merge commits kinda skipped review https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy

@MatthewPeterKelly
Copy link
Contributor Author

Merge commits kinda skipped review https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy

You're totally right, and thanks for letting me know. This is my first PR for rust and I missed that policy when I was reading the docs. I'll be sure to do it correctly next time. Let me know if there is anything I should fix for this PR, but I suspect that we're locked-in at this point.

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Rollup of 8 pull requests

Successful merges:

 - rust-lang#100747 (Add long description and test for E0311)
 - rust-lang#102232 (Stabilize bench_black_box)
 - rust-lang#102288 (Suggest unwrapping `???<T>` if a method cannot be found on it but is present on `T`.)
 - rust-lang#102338 (Deny associated type bindings within associated type bindings)
 - rust-lang#102347 (Unescaping cleanups)
 - rust-lang#102348 (Tweak `FulfillProcessor`.)
 - rust-lang#102378 (Use already resolved `self_ty` in `confirm_fn_pointer_candidate`)
 - rust-lang#102380 (rustdoc: remove redundant mobile `.source > .sidebar` CSS)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants