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

Tracking issue for enabling -Zdrop-tracking by default #97331

Closed
6 of 7 tasks
eholk opened this issue May 23, 2022 · 10 comments
Closed
6 of 7 tasks

Tracking issue for enabling -Zdrop-tracking by default #97331

eholk opened this issue May 23, 2022 · 10 comments
Assignees
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting.

Comments

@eholk
Copy link
Contributor

eholk commented May 23, 2022

We tried to do this a while ago (#91032) but discovered a number of regressions soon afterwards so it was disabled (#93165). This issue tracks known regressions and their fixes. Note that many regressions have been fixed but are not listed here since they were done before I filed this bug.

@eholk
Copy link
Contributor Author

eholk commented May 23, 2022

@rustbot label +A-async-await +AsyncAwait-Triaged

@rustbot rustbot added A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. labels May 23, 2022
@eholk
Copy link
Contributor Author

eholk commented May 23, 2022

@rustbot claim

@jyn514
Copy link
Member

jyn514 commented Jul 14, 2022

The last two issues have both been fixed :) and the first has been mitigated (it doesn't have the same error as it used to, but the new error is much more readable than previously).
Let me know if there's any way I can help with the must_not_suspend lint.

@eholk
Copy link
Contributor Author

eholk commented Jul 14, 2022

I suspect the first issue can be marked as fixed too. I'll double check that it still looks okay and if so go ahead and close the issue.

For must_not_suspend, if we're lucky this may just need someone to try it out, since it seems like #98754 might have fixed that as well.

@davidbarsky
Copy link
Contributor

davidbarsky commented Jul 16, 2022

For must_not_suspend, if we're lucky this may just need someone to try it out, since it seems like #98754 might have fixed that as well.

Would it be helpful if tracing added #[must_not_suspend] behind an RUSTFLAGS? I can then try enabling it at work's monorepo and see what breaks. I'm not sure if we have #98754, but I can double-check.

@jyn514
Copy link
Member

jyn514 commented Aug 18, 2022

I think all of these have been fixed :) @eholk what are the next steps? Another Crater run?

@eholk
Copy link
Contributor Author

eholk commented Aug 18, 2022

@jyn514 - Yup! Let me rebase #97334 and then we should be able to run just the failed ones from last time.

@eholk
Copy link
Contributor Author

eholk commented Aug 18, 2022

Bummer, looks like there's another test that broke in the meantime: #97334 (comment)

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 1, 2022
[drop tracking] Use parent expression for scope, not parent node

Previously we were just using the parent node as the scope for a temporary value, but it turns out this is too narrow. For example, in an expression like

    Foo {
        b: &42,
        a: async { 0 }.await,
    }

the scope for the &42 was set to the ExprField node for `b: &42`, when we actually want to use the Foo struct expression.

We fix this by recursively searching through parent nodes until we find a Node::Expr. It may be that we don't find one, and if so that's okay, we will just fall back on the enclosing temporary scope which is always sufficient.

Helps with rust-lang#97331

r? `@jyn514`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 2, 2022
[drop tracking] Use parent expression for scope, not parent node

Previously we were just using the parent node as the scope for a temporary value, but it turns out this is too narrow. For example, in an expression like

    Foo {
        b: &42,
        a: async { 0 }.await,
    }

the scope for the &42 was set to the ExprField node for `b: &42`, when we actually want to use the Foo struct expression.

We fix this by recursively searching through parent nodes until we find a Node::Expr. It may be that we don't find one, and if so that's okay, we will just fall back on the enclosing temporary scope which is always sufficient.

Helps with rust-lang#97331

r? `@jyn514`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 3, 2022
[drop tracking] Use parent expression for scope, not parent node

Previously we were just using the parent node as the scope for a temporary value, but it turns out this is too narrow. For example, in an expression like

    Foo {
        b: &42,
        a: async { 0 }.await,
    }

the scope for the &42 was set to the ExprField node for `b: &42`, when we actually want to use the Foo struct expression.

We fix this by recursively searching through parent nodes until we find a Node::Expr. It may be that we don't find one, and if so that's okay, we will just fall back on the enclosing temporary scope which is always sufficient.

Helps with rust-lang#97331

r? ``@jyn514``
@StyMaar
Copy link

StyMaar commented Mar 28, 2024

Shouldn't this issue be closed now that everything in it is done?

@eholk
Copy link
Contributor Author

eholk commented Apr 3, 2024

Yes!

@eholk eholk closed this as completed Apr 3, 2024
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 AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting.
Projects
None yet
Development

No branches or pull requests

5 participants