Skip to content

feat: Add moved-out-of-ref diagnostic #14789

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 1 commit into from
May 18, 2023
Merged

feat: Add moved-out-of-ref diagnostic #14789

merged 1 commit into from
May 18, 2023

Conversation

HKalbasi
Copy link
Member

This is marked as experimental, since spans are terrible, there are some false positives due #14754, and it doesn't play well with unknown types. But I hope we can soon lift it.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 12, 2023
Comment on lines 122 to 124
// FIXME: we should not show error for this.
*x
//^^ error: cannot move `X<{unknown}>` out of reference
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 added a {unknown}: Copy clause in trait env for chalk, but it only works for the trivial case. Any idea why?

Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding that clause we should just ignore the diagnostics for types containing errors I think

Copy link
Member Author

Choose a reason for hiding this comment

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

That is also an option, I tried to avoid false negative in cases like Vec<{unknown}> which is definitely not copy regardless of the inner type, but if it doesn't work we should probably do that.

@lowr do you know why adding a {unknown}: Copy is not enough for chalk to proof that X<{unknown}> is copy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like X<{unknown}>: Copy produces two subgoals, {unknown}: Copy and {unknown}: Sized, and chalk fails to prove the latter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, so it also will have problems with other bounds on X, I guess we should replace the error types with some variable and solve exists<T> X<T>: Copy right? I think we can start with ignoring types containing errors for now. Thanks for investigating.

@HKalbasi
Copy link
Member Author

Can we merge this, since it is an experimental diagnostic? Or should we wait until monday release?

@Veykril
Copy link
Member

Veykril commented May 14, 2023

Let's wait til monday, even if its experimental. That way people on stable r-a don't have to fiddle with the setting if we manage to make it trigger less in a week.

@bors
Copy link
Contributor

bors commented May 18, 2023

☔ The latest upstream changes (presumably #14787) made this pull request unmergeable. Please resolve the merge conflicts.

@HKalbasi
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2023

📌 Commit b55fbd3 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 18, 2023

⌛ Testing commit b55fbd3 with merge 54129fa...

@bors
Copy link
Contributor

bors commented May 18, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 54129fa to master...

@bors bors merged commit 54129fa into rust-lang:master May 18, 2023
@lnicola lnicola changed the title Add moved-out-of-ref diagnostic feat: Add moved-out-of-ref diagnostic May 18, 2023
@lnicola
Copy link
Member

lnicola commented May 22, 2023

image

@HKalbasi
Copy link
Member Author

This is the rustc diagnostic. Ours is very ugly :)

But why it is not showing here? Are experimental diagnostics enabled?

@lnicola
Copy link
Member

lnicola commented May 22, 2023

Oops, sorry, updated. They were disabled.

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