Skip to content

Don't panic in <BorrowedCursor as io::Write>::write #115460

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 8, 2023
Merged

Don't panic in <BorrowedCursor as io::Write>::write #115460

merged 1 commit into from
Nov 8, 2023

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Sep 2, 2023 •
edited
Loading

Instead of panicking if the BorrowedCursor does not have enough capacity for the whole buffer, just return a short write, like <&mut [u8] as io::Write>::write does.

(cc @ChayimFriedman2 #78485 (comment))

(I'm not sure if this needs an ACP? since it's not changing the "API", just what the function does)

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 2, 2023
@zachs18 zachs18 marked this pull request as ready for review September 2, 2023 02:42
djc
djc approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

This makes sense to me!

@djc
Copy link
Contributor

djc commented Sep 20, 2023

@thomcc gentle ping? Should this get another reviewer?

@joboet
Copy link
Member

joboet commented Oct 21, 2023

I'm going to take the liberty of assigning this to someone from libs-api since this is technically a user-facing change, even if it merely fixes a bug.

r? libs-api
@rustbot label +T-libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 21, 2023
@rustbot rustbot assigned Amanieu and unassigned thomcc Oct 21, 2023
@Amanieu
Copy link
Member

Amanieu commented Oct 27, 2023

This is a very minor API change, but ultimately I think the new behavior is better than the old one. It is also consistent with other Write implementations.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 27, 2023 •
edited by BurntSushi
Loading

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 27, 2023
@thomcc
Copy link
Member

thomcc commented Oct 27, 2023

Should libs be on this FCP?

@Amanieu
Copy link
Member

Amanieu commented Oct 27, 2023

Hmm probably not but at this point I don't think it really hurts.

@the8472
Copy link
Member

the8472 commented Oct 27, 2023

BorrowedCursor is unstable anyway and write_all turns this into an Errorkind::WriteZero. Seems fine.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 28, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 28, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 7, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 7, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@dtolnay
Copy link
Member

dtolnay commented Nov 7, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 7, 2023

📌 Commit 11a64a1 has been approved by dtolnay

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 7, 2023
@dtolnay dtolnay assigned dtolnay and unassigned Amanieu Nov 8, 2023
fmease added a commit to fmease/rust that referenced this pull request Nov 8, 2023
@fmease
Rollup merge of rust-lang#115460 - zachs18:borrowedcursor_write_no_pa…
7b11383 
…nic, r=dtolnay

Don't panic in `<BorrowedCursor as io::Write>::write`

Instead of panicking if the BorrowedCursor does not have enough capacity for the whole buffer, just return a short write, [like `<&mut [u8] as io::Write>::write` does](https://doc.rust-lang.org/src/std/io/impls.rs.html#349).

(cc `@ChayimFriedman2` rust-lang#78485 (comment))

(I'm not sure if this needs an ACP? since it's not changing the "API", just what the function does)
@fmease fmease mentioned this pull request Nov 8, 2023
Closed
@bors
Copy link
Collaborator

bors commented Nov 8, 2023

⌛ Testing commit 11a64a1 with merge 28acba3...

@bors
Copy link
Collaborator

bors commented Nov 8, 2023

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 28acba3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2023
@bors bors merged commit 28acba3 into rust-lang:master Nov 8, 2023
@rustbot rustbot added this to the 1.75.0 milestone Nov 8, 2023
This was referenced Nov 8, 2023
Merged
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (28acba3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.9% [0.5%, 1.4%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.7% [-0.9%, -0.4%] 4
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.1% [-0.9%, 1.4%] 6

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 663.194s -> 662.725s (-0.07%)
Artifact size: 308.72 MiB -> 308.73 MiB (0.00%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 9, 2023
celinval added a commit to celinval/rust-dev that referenced this pull request Jun 4, 2024
@github-actions @celinval
Automatic toolchain upgrade to nightly-2023-11-09 (rust-lang#2868)
8bed969 
Update Rust toolchain from nightly-2023-11-08 to nightly-2023-11-09
without any other source changes.
This is an automatically generated pull request. If any of the CI checks
fail, manual intervention is required. In such a case, review the
changes at https://github.com/rust-lang/rust from
rust-lang@7adc89b
up to
rust-lang@fdaaaf9.
The log for this commit range is:
rust-lang@fdaaaf9f92 Auto merge of
rust-lang#116930 - RalfJung:raw-ptr-match, r=davidtwco
rust-lang@30588657b7 avoid unnecessary
nested conditionals
rust-lang@90fdc1fc27 Auto merge of
rust-lang#117716 - GuillaumeGomez:rollup-83gnhll, r=GuillaumeGomez
rust-lang@9d3c80248b Rollup merge of
rust-lang#117713 - GuillaumeGomez:document-hidden-json, r=notriddle
rust-lang@d8c52b378d Rollup merge of
rust-lang#117702 - davidtwco:target-tier-refactors, r=petrochenkov
rust-lang@5d00a5d936 Rollup merge of
rust-lang#117679 - aDotInTheVoid:yes-core, r=GuillaumeGomez
rust-lang@c828371179 Rollup merge of
rust-lang#117282 - clubby789:recover-wrong-function-header, r=TaKO8Ki
rust-lang@e05e4f38b5 Rollup merge of
rust-lang#117263 - onur-ozkan:change-id-fix, r=saethlin
rust-lang@341efb1017 Auto merge of
rust-lang#117560 - lqd:issue-117146, r=matthewjasper
rust-lang@33edea60f0 Add test for
reexported hidden item with `--document-hidden-items`
rust-lang@28acba3c61 Auto merge of
rust-lang#115460 - zachs18:borrowedcursor_write_no_panic, r=dtolnay
rust-lang@755629fe59 Auto merge of
rust-lang#117706 - matthiaskrgr:rollup-lscx7dg, r=matthiaskrgr
rust-lang@e0cb1cc296 bootstrap: add more
detail on change-id comments
rust-lang@e878100386 bootstrap: improve
`fn check_version`
rust-lang@ae4d18b2da handle the case when
the change-id isn't found
rust-lang@7e4ffa98b5 Rollup merge of
rust-lang#117700 - Zalathar:rename-run-coverage, r=onur-ozkan
rust-lang@55306535dd Rollup merge of
rust-lang#117698 - nnethercote:space_between-2, r=petrochenkov
rust-lang@b1b5a8ea9d Rollup merge of
rust-lang#117667 - Alexendoo:doc-clippy-config, r=albertlarsan68
rust-lang@adf4981969 Rollup merge of
rust-lang#117663 - klensy:bump-deps, r=davidtwco
rust-lang@8198864377 Rollup merge of
rust-lang#117650 - saethlin:inline-me-please, r=davidtwco
rust-lang@ba7ec56639 Rollup merge of
rust-lang#117531 - fmease:rustdoc-effects-properly-elide-x-crate-host-args,
r=GuillaumeGomez
rust-lang@b74a84c0bc Rollup merge of
rust-lang#114316 - ecnelises:aix_doc, r=workingjubilee
rust-lang@fab1054e17 Auto merge of
rust-lang#117542 - compiler-errors:only-normalize-predicate, r=lcnr
rust-lang@750c2ecd15 Auto merge of
rust-lang#116881 - LuuuXXX:issue-110087, r=onur-ozkan
rust-lang@b9b7982f72 Add AIX
platform-support doc
rust-lang@ef7ebaa788 rustc_target: move
file for uniformity
rust-lang@1af256fe8a targets: move target
specs to spec/targets
rust-lang@76aa83e3e1 target: move base
specs to spec/base
rust-lang@a573880373 coverage: Rename the
`run-coverage` test mode to `coverage-run`
rust-lang@7cc997d373 Auto merge of
rust-lang#117699 - weihanglo:update-cargo, r=weihanglo
rust-lang@0670466e2c Update cargo
rust-lang@438b9a6e82 More tests for token
stream pretty-printing with adjacent punctuation.
rust-lang@783d4b8b26 Clarify
`space_between`.
rust-lang@91cfcb0219 Auto merge of
rust-lang#117484 - Zalathar:tests, r=cjgillot
rust-lang@97c9d8f405 Only use
normalize_param_env when normalizing predicate in check_item_bounds
rust-lang@1b8dee19e8 Fix issue rust-lang#110087
rust-lang@0d5ec963bb Auto merge of
rust-lang#117692 - matthiaskrgr:rollup-umaf5pr, r=matthiaskrgr
rust-lang@f72e974e3f Rollup merge of
rust-lang#117655 - compiler-errors:method-tweaks, r=estebank
rust-lang@7552dd19ad Rollup merge of
rust-lang#117625 - nnethercote:clippy-perf, r=cuviper
rust-lang@3c6307240c Rollup merge of
rust-lang#116399 - WaffleLapkin:erase_small_things, r=cjgillot
rust-lang@b724d9c90e Rollup merge of
rust-lang#113925 - clubby789:const-ctor-repeat, r=estebank
rust-lang@fcdd99edca Add
-Zcross-crate-inline-threshold=yes
rust-lang@e8cf29b584 rustdoc: minor
changes suggested by clippy perf lints.
rust-lang@ff0b4b6091 Auto merge of
rust-lang#117672 - lqd:ci-gcc-lld, r=Kobzol
rust-lang@1b3733e5a4 rustc: minor changes
suggested by clippy perf lints.
rust-lang@eca9a1533f Add an explanation
for `transmute_unchecked`
rust-lang@1d1fe9a205 add note to remember
to update a url when bumping to gcc 10.1.0
rust-lang@434b69a1d6 tests/rustdoc-json:
Rewrite tests no not use `#![no_core]`.
rust-lang@0875f456f1 tests/rustdoc-json:
Remove more needless uses of `#![no_core]`.
rust-lang@f784fa7bd9 tests/rustdoc-json:
Remove some needless uses of `#![no_core]`.
rust-lang@d73eaaac5f ci: bump gcc on dist
x64 linux builder to 9.5
rust-lang@f2fd8ad788 Document
clippy_config in nightly-rustc docs
rust-lang@eed89185bb bump some deps
rust-lang@0add056dee Rework
print_disambiguation_help
rust-lang@88a37acb26 Yeet
MethodCallComponents
rust-lang@4e6f438d2a coverage: Register
`test::Coverage` as the test suite for `tests/coverage`
rust-lang@49127c64d6 coverage: Migrate
`tests/coverage-map` into `tests/coverage`
rust-lang@e9d04c5e24 coverage: Migrate
`tests/run-coverage` into `tests/coverage`
rust-lang@aea7c27eae coverage: Set up a
macro for declaring unified coverage test suites
rust-lang@3509aed632 coverage: Add
`./x.py test coverage`, an alias for `coverage-map` and `run-coverage`
rust-lang@e585a99230 coverage: Give each
coverage test mode a separate output directory
rust-lang@211d4cee8e coverage: Copy all
remaining run-coverage tests into coverage-map
rust-lang@4b76b97bc8 coverage: Copy all
remaining coverage-map tests into run-coverage
rust-lang@f5df56b26b coverage: Flatten
`coverage-map/status-quo/` into its parent directory
rust-lang@8eef39f082 coverage: Remove
`tests/coverage-map/if.rs`
rust-lang@7f8a6de72c coverage: Use
`-Copt-level=2` by default in run-coverage tests
rust-lang@1dcdf83927 rustdoc: properly
elide cross-crate host effect args
rust-lang@2dff90dc23 add test for issue
117146
rust-lang@de7a8305ae traverse region
graph instead of SCCs to compute polonius loan scopes
rust-lang@03b24f2756 remove some dead
code
rust-lang@70a8e157ab make
pointer_structural_match warn-by-default
rust-lang@af6c7e0ca1 also lint against fn
ptr and raw ptr nested inside the const
rust-lang@bec88ad4aa patterns: reject raw
pointers that are not just integers
rust-lang@be0b42fabe Recover from
incorrectly ordered/duplicated function keywords
rust-lang@b3d50255d9 Use consisntent
style of `size_of` in query type erasure
rust-lang@61361bb212 Use
`transmute_unchecked` and make the types explicit in query type erasure
rust-lang@11a64a1834 don't panic in
BorrowedCursor::write
rust-lang@86b112204a Improve diagnostic
for const ctors in array repeat expressions

Co-authored-by: celinval <celinval@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@djc djc djc approved these changes

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.75.0
Development

Successfully merging this pull request may close these issues.