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

Reset Formatter flags on exit from pad_integral #67784

Merged
merged 1 commit into from Jan 15, 2020

Conversation

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 1, 2020

This fixes a bug where after calling pad_integral with appropriate flags, the
fill and alignment flags would be set to '0' and 'Right' and left as such even
after exiting pad_integral, which meant that future calls on the same Formatter
would get incorrect flags reported.

This is quite difficult to observe in practice, as almost all formatting
implementations in practice don't call Display::fmt directly, but rather use
write! or a similar macro, which means that they cannot observe the effects of
the wrong flags (as write! creates a fresh Formatter instance). However, we
include a test case.

A manual check leads me to believe this is the only case where we failed to reset the flags appropriately, but I could have missed something.

This fixes a bug where after calling pad_integral with appropriate flags, the
fill and alignment flags would be set to '0' and 'Right' and left as such even
after exiting pad_integral, which meant that future calls on the same Formatter
would get incorrect flags reported.

This is quite difficult to observe in practice, as almost all formatting
implementations in practice don't call `Display::fmt` directly, but rather use
`write!` or a similar macro, which means that they cannot observe the effects of
the wrong flags (as `write!` creates a fresh Formatter instance). However, we
include a test case.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 1, 2020

r? @cramertj

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

Copy link
Member Author

Mark-Simulacrum left a comment

Note that this is technically a breaking change, but I think it's a reasonable one (unlikely that someone is genuinely hoping for this). We can run crater, but I personally lean towards not doing so.

post_padding.write(self.buf)
post_padding.write(self.buf)?;
self.fill = old_fill;
self.align = old_align;

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Jan 1, 2020

Author Member

It's not obvious just where we should be resetting the flags, but this seems like a "not wrong" place at least.

}
}

assert_eq!(format!("{:<03}", Bar), "1 0051 ");

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Jan 1, 2020

Author Member

Diff from before this PR:

-1  005001
+1  0051  
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Jan 2, 2020

Let's r? @Amanieu tentatively here as this is likely to need some libs team attention with regards to the breakage here

@rust-highfive rust-highfive assigned Amanieu and unassigned cramertj Jan 2, 2020
@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Jan 2, 2020

I'm going to defer this to someone more familiar with the formatting code. r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned Amanieu Jan 2, 2020
Copy link
Member

dtolnay left a comment

Thanks! This looks good to me. I don't think we need to treat this as a breaking change.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Jan 14, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2020

📌 Commit 73996df has been approved by dtolnay

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 14, 2020
…al, r=dtolnay

Reset Formatter flags on exit from pad_integral

This fixes a bug where after calling pad_integral with appropriate flags, the
fill and alignment flags would be set to '0' and 'Right' and left as such even
after exiting pad_integral, which meant that future calls on the same Formatter
would get incorrect flags reported.

This is quite difficult to observe in practice, as almost all formatting
implementations in practice don't call `Display::fmt` directly, but rather use
`write!` or a similar macro, which means that they cannot observe the effects of
the wrong flags (as `write!` creates a fresh Formatter instance). However, we
include a test case.

A manual check leads me to believe this is the only case where we failed to reset the flags appropriately, but I could have missed something.
bors added a commit that referenced this pull request Jan 14, 2020
Rollup of 7 pull requests

Successful merges:

 - #66329 (Add unreachable propagation mir optimization pass)
 - #67784 (Reset Formatter flags on exit from pad_integral)
 - #67914 (Don't run const propagation on items with inconsistent bounds)
 - #68012 (Update some of Cargo's dependencies)
 - #68096 (Clean up some diagnostics by making them more consistent)
 - #68118 (perf: Eagerly convert literals to consts)
 - #68123 (Implement Cursor for linked lists. (RFC 2570).)

Failed merges:

r? @ghost
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 15, 2020
…al, r=dtolnay

Reset Formatter flags on exit from pad_integral

This fixes a bug where after calling pad_integral with appropriate flags, the
fill and alignment flags would be set to '0' and 'Right' and left as such even
after exiting pad_integral, which meant that future calls on the same Formatter
would get incorrect flags reported.

This is quite difficult to observe in practice, as almost all formatting
implementations in practice don't call `Display::fmt` directly, but rather use
`write!` or a similar macro, which means that they cannot observe the effects of
the wrong flags (as `write!` creates a fresh Formatter instance). However, we
include a test case.

A manual check leads me to believe this is the only case where we failed to reset the flags appropriately, but I could have missed something.
bors added a commit that referenced this pull request Jan 15, 2020
Rollup of 7 pull requests

Successful merges:

 - #67784 (Reset Formatter flags on exit from pad_integral)
 - #68096 (Clean up some diagnostics by making them more consistent)
 - #68123 (Implement Cursor for linked lists. (RFC 2570).)
 - #68194 (Fix CI for embedded ARM targets)
 - #68223 (Use 3.6 instead of 3.5 in float fract() documentation)
 - #68231 (Better support for cross compilation on Windows.)
 - #68233 (Update compiler_builtins with changes to fix 128 bit integer remainder for aarch64 windows.)

Failed merges:

r? @ghost
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 15, 2020
…al, r=dtolnay

Reset Formatter flags on exit from pad_integral

This fixes a bug where after calling pad_integral with appropriate flags, the
fill and alignment flags would be set to '0' and 'Right' and left as such even
after exiting pad_integral, which meant that future calls on the same Formatter
would get incorrect flags reported.

This is quite difficult to observe in practice, as almost all formatting
implementations in practice don't call `Display::fmt` directly, but rather use
`write!` or a similar macro, which means that they cannot observe the effects of
the wrong flags (as `write!` creates a fresh Formatter instance). However, we
include a test case.

A manual check leads me to believe this is the only case where we failed to reset the flags appropriately, but I could have missed something.
bors added a commit that referenced this pull request Jan 15, 2020
Rollup of 12 pull requests

Successful merges:

 - #67784 (Reset Formatter flags on exit from pad_integral)
 - #67914 (Don't run const propagation on items with inconsistent bounds)
 - #68141 (use winapi for non-stdlib Windows bindings)
 - #68211 (Add failing example for E0170 explanation)
 - #68219 (Untangle ZST validation from integer validation and generalize it to all zsts)
 - #68222 (Update the wasi-libc bundled with libstd)
 - #68226 (Avoid calling tcx.hir().get() on CRATE_HIR_ID)
 - #68227 (Update to a version of cmake with windows arm64 support)
 - #68229 (Update iovec to a version with no winapi dependency)
 - #68230 (Update libssh2-sys to a version that can build for aarch64-pc-windows…)
 - #68231 (Better support for cross compilation on Windows.)
 - #68233 (Update compiler_builtins with changes to fix 128 bit integer remainder for aarch64 windows.)

Failed merges:

r? @ghost
@bors bors merged commit 73996df into rust-lang:master Jan 15, 2020
4 checks passed
4 checks passed
pr Build #20200101.38 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.