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

Provide structured suggestion on unsized fields and fn params #74228

Merged
merged 7 commits into from
Jul 14, 2020

Conversation

estebank
Copy link
Contributor

  • Suggest borrowing or boxing unsized fields
  • Suggest borrowing fn parameters
  • Remove some verbosity of unsized errors
  • Remove on_unimplemented note from trait Sized

Fix #23286, fix #28653.

r? @davidtwco

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2020
|
LL | value: &T,
| ^
help: boxed trait objects have a statically known size
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this help slightly misleading? T is a type parameter, not a trait object here

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 is a good point. I could say boxed types have... and that would be more accurate always at the cost of not giving people some jargon to google... I'll see if I can make it conditional on the type.

Copy link
Contributor Author

@estebank estebank Jul 11, 2020

Choose a reason for hiding this comment

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

What about "heap allocated types always have a statically known size"? Edit: last commit has this wording.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I personally don't really like "heap allocated types", as an object of the type is allocated on the heap, not the type itself 😅

Not sure what's the best wording here, but maybe "boxed types always have a statically known size" which seems more correct to me

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 wanted to introduce a bit of jargon here, as Box is already mentioned in the code suggestion itself, to tie the concept of "heap allocations" and "Box". That being said I see your point. Lets merge this as is to avoid bitrot (I don't think the wording is outright wrong, just suboptimal), but let's open a follow up PR to improve the wording?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lcnr what about "the Box type always has a statically known size and allocates its contents in the heap"?

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, r=me after squashing a little

@estebank
Copy link
Contributor Author

after squashing a little

Should I squash all into one commit or just squash the fixup commits where they belong?

@davidtwco
Copy link
Member

Should I squash all into one commit or just squash the fixup commits where they belong?

Whichever you prefer, I tend to do the latter.

@estebank
Copy link
Contributor Author

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2020

📌 Commit 2d0f3eb has been approved by estebank

@bors
Copy link
Contributor

bors commented Jul 12, 2020

🌲 The tree is currently closed for pull requests below priority 20, this pull request will be tested once the tree is reopened

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 13, 2020
Provide structured suggestion on unsized fields and fn params

* Suggest borrowing or boxing unsized fields
* Suggest borrowing fn parameters
* Remove some verbosity of unsized errors
* Remove `on_unimplemented` note from `trait Sized`

Fix rust-lang#23286, fix rust-lang#28653.

r? @davidtwco
@Manishearth
Copy link
Member

@bors r-

#74305 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2020
@estebank
Copy link
Contributor Author

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Jul 14, 2020

📌 Commit ff75395 has been approved by davidtwco

@bors
Copy link
Contributor

bors commented Jul 14, 2020

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

@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-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 14, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2020
…arth

Rollup of 11 pull requests

Successful merges:

 - rust-lang#73759 (Add missing Stdin and StdinLock examples)
 - rust-lang#74211 (Structured suggestion when not using struct pattern)
 - rust-lang#74228 (Provide structured suggestion on unsized fields and fn params)
 - rust-lang#74252 (Don't allow `DESTDIR` to influence LLVM builds)
 - rust-lang#74263 (Slight reorganization of sys/(fast_)thread_local)
 - rust-lang#74271 (process_unix: prefer i32::*_be_bytes over manually shifting bytes)
 - rust-lang#74272 (pprust: support multiline comments within lines)
 - rust-lang#74332 (Update cargo)
 - rust-lang#74334 (bootstrap: Improve wording on docs for `verbose-tests`)
 - rust-lang#74336 (typeck: use `item_name` in cross-crate packed diag)
 - rust-lang#74340 (lint: use `transparent_newtype_field` to avoid ICE)

Failed merges:

r? @ghost
@bors bors merged commit a364c0a into rust-lang:master Jul 14, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.
Projects
None yet
7 participants