Skip to content

feat: Closure capture inlay hints #14742

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

Merged
merged 3 commits into from
May 8, 2023

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented May 5, 2023

I opted for a fictional move(foo, &bar, &mut qux) syntax here, disabled by default as these are not correct rust syntax and hence could cause confusion.
image

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2023
@Veykril Veykril force-pushed the closure-capture-inlays branch from 9876c83 to 2403dd1 Compare May 5, 2023 11:35
@Veykril Veykril force-pushed the closure-capture-inlays branch from 2403dd1 to 8081a65 Compare May 5, 2023 11:38
Comment on lines 159 to 169
// FIXME: &mut qux should be &unique qux
|| {
// ^ move
// ^ (
// ^ &mut baz
// ^ ,
// ^ &mut qux
// ^ )
baz = NonCopy;
*qux = NonCopy;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

@HKalbasi fwiw I still think this is wrong. qux is not a mutable binding so we shouldn't be allowed to take a mutable borrow here. Do you mind pointing me to the part in the rustc code base for this (assuming you have it at hand)?

Copy link
Member

Choose a reason for hiding this comment

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

qux is not mutable, but we are taking a mutable reference to the *qux, not qux itself. For example this code:

    let mut t1 = 2;
    let mut t2 = 5;
    let mut x = &mut t1;
    let mut y = || {
        //&x;
        *x = 3;
    };
    x = &mut t2;
    y();

compiles since y is capturing *x by mutable borrow, so x itself is free to mutate. But if the //&x; line becomes uncommented, y will capture the whole x by unique immutable borrow, and we will get compile error.

Copy link
Member

Choose a reason for hiding this comment

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

I think it will becomes less confusing if we render the whole place.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my confusion comes from thinking of old capture logic? Also the rust reference I suppose is outdated then. You are certainly right that we should be rendering the place we capture, as with this mere field captures will render incorrectly already I believe.

Copy link
Member

Choose a reason for hiding this comment

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

I think my confusion comes from thinking of old capture logic? Also the rust reference I suppose is outdated then

Yes the rust reference section for closure is pre edition-2021, and is currently in a misleading state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out there is a PR for this already so I'll dig into that for now rust-lang/reference#1059

Copy link
Member Author

Choose a reason for hiding this comment

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

@Veykril Veykril marked this pull request as ready for review May 5, 2023 12:18
@matklad
Copy link
Member

matklad commented May 5, 2023

🔥 🧨 🚒

@Veykril Veykril force-pushed the closure-capture-inlays branch from 9fbd202 to 1c99118 Compare May 7, 2023 07:43
@Veykril Veykril force-pushed the closure-capture-inlays branch from 1c99118 to 4c5fd19 Compare May 8, 2023 07:51
@Veykril
Copy link
Member Author

Veykril commented May 8, 2023

We probably want an image showcasing more complex captures for the changelog, will cook those up some later time this week
@bors r+

@bors
Copy link
Contributor

bors commented May 8, 2023

📌 Commit 4c5fd19 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 8, 2023

⌛ Testing commit 4c5fd19 with merge d3ce333...

@bors
Copy link
Contributor

bors commented May 8, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing d3ce333 to master...

@bors bors merged commit d3ce333 into rust-lang:master May 8, 2023
@Veykril Veykril deleted the closure-capture-inlays branch May 8, 2023 10:16
@lnicola
Copy link
Member

lnicola commented May 15, 2023

We probably want an image showcasing more complex captures for the changelog

I'll use the existing one, it seems complex enough to me 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants