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.

Sign up for GitHub

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

Fix some of the rustfmt fallout in Miri #67833

Closed
wants to merge 5 commits into from
Closed

Fix some of the rustfmt fallout in Miri #67833

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0581605
fix rustfmt fallout
RalfJung Jan 3, 2020
ff5b102
let rustfmt undo most of my edits :(
RalfJung Jan 3, 2020
e44f3e7
manually format Miri error message matches
RalfJung Jan 3, 2020
bc902c4
adjust Deref comment
RalfJung Jan 6, 2020
7a4a08f
add FIXME for rustfmt::skip
RalfJung Jan 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
292 changes: 159 additions & 133 deletions src/librustc/mir/interpret/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl ErrorHandled {
ErrorHandled::Reported => {}
ErrorHandled::TooGeneric => bug!(
"MIR interpretation failed without reporting an error \
even though it was fully monomorphized"
even though it was fully monomorphized"
),
}
}
Expand Down Expand Up @@ -438,139 +438,165 @@ pub enum UnsupportedOpInfo<'tcx> {
impl fmt::Debug for UnsupportedOpInfo<'tcx> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use UnsupportedOpInfo::*;
#[rustfmt::skip] // rustfmt and long matches do not go well together
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo the diff doesn't seem like that much of an improvement to justify this... I'm concerned about people starting to use this in various places throughout the compiler to fit their personal tastes rather than having a uniform style.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with @Centril here that I would like to avoid #[rustfmt::skip]. I'd rather find ways to structure the code that make rustfmt happy, or else open issues on rustfmt for extreme cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Tweaking rustfmt.toml seems ok too, though I'd prefer to avoid even that when we can.)

Copy link
Member Author

@RalfJung RalfJung Jan 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Uh... for me this diff is the difference between "unreadable garbage" and "reasonable code". Like, without this PR, the write! is formatted in one of three different ways depending on the length of the message and the phase of the moon. Sometimes it's entirely in the same line as the pattern, sometimes it is in a line on its own, and sometimes the write! is on the line of the pattern but its arguments are in separate lines. On top of that, some arms have curly braces and some don't, even though all just contain a single write!. How is that not just bad style? The style should be consistent across all arms of this match. But what rustfmt does is inconsistent, it's hard to parse visually. The entire point of rustfmt, I thought, is to get consistent formatting, but clearly it is doing the opposite here (and for matches in general). The code is very symmetric, but visually in rustfmt style, there's no symmetry at all.

I opened a rustfmt issue at rust-lang/rustfmt#3995. But until that gets fixed, I don't see any good reason to let a tool's deficiency (rustfmt overall works fairly poorly on match arms) dictate the format.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also Cc @Mark-Simulacrum, whose suggestion it was to use rustfmt::skip on this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's just useful to be able to format-on-save anywhere

I appreciate that's useful for folks who went through the hoops of setting that up. Even as someone who didn't, I appreciate the consistent formatting of function signatures, the sorted imports, stuff like that.

But when considering matches like this, I honestly don't understand how you can call this a "uniform reading experience" -- there's nothing uniform about how rustfmt formats this match. As far as I am concerned, the formatting of this match after rustfmt is literally worse than anything I ever saw anywhere in the rustc codebase before rustfmt. The way it formats matches will make it much harder for me to work on and review this code-base, due to how it degrades readability. I appreciate that is subjective, but the part where it is very inconsistent and using different styles to do the exact same thing is objectively neither uniform nor consistent.

I think the style in this diff is by far egregious enough to justify rustfmt::skip. I also don't get the principled opposition to that: we don't usually make ourselves slaves to our own tools, do we? Tidy has had exceptions that were and are applied where reasonable. Not allowing reasonable exceptions would be quite unprecedented. In fact, I count around 30 occurrences of rustfmt::skip in the rustc repo already (excluding tools and tests). Many of them are for some sort of table or another (but in some sense, this match is a table), but this one is not. And that makes sense -- rustfmt is a great tool, but like all tools it has its limitations. We should apply the tool where it makes sense, but not mindlessly submit ourselves to whatever the tool happens to do.

Copy link
Member

@pnkfelix pnkfelix Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'll just note that sometimes (sometimes) I find let val; ...; val = expr more readable.

(Not when its mutable state; that's almost never more readable. I'm talking about lazily initialized state here.)

Why do I find it more readable? Well, sometimes the `let val = { ... }; ends up with something where the result expression is so far from the binding that it doesn't fit in my editor's view at once, which means I've effectively lost the relationship (or at least have to keep in in my mental cache).

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do I find it more readable? Well, sometimes the `let val = { ... }; ends up with something where the result expression is so far from the binding that it doesn't fit in my editor's view at once, which means I've effectively lost the relationship (or at least have to keep in in my mental cache).

Split your functions Felix, so that "doesn't fit" doesn't happen :P

Copy link
Member

Choose a reason for hiding this comment

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

Split your functions Felix, so that "doesn't fit" doesn't happen :P

Sure, when that's an option, it can work well. And I suppose you might also suggest that I ensure that the enums I process never have more than four or five variants.

but sometimes you have to play the hand you are dealt, and I'm glad that I have let x; ... x = ...; as a tool in my back pocket, even you all aren't willing to use it yourself.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I suppose you might also suggest that I ensure that the enums I process never have more than four or five variants.

Not really, but you can split match arms into separate functions when the match gets too long, e.g. as in https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_typeck/check/expr.rs.html#213-289 or https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_typeck/check/pat.rs.html#119-219 which were unreadable messes before I split those functions apart.

match self {
PointerOutOfBounds { ptr, msg, allocation_size } => write!(
f,
"{} failed: pointer must be in-bounds at offset {}, \
but is outside bounds of allocation {} which has size {}",
msg,
ptr.offset.bytes(),
ptr.alloc_id,
allocation_size.bytes()
),
ValidationFailure(ref err) => write!(f, "type validation failed: {}", err),
NoMirFor(ref func) => write!(f, "no MIR for `{}`", func),
FunctionAbiMismatch(caller_abi, callee_abi) => write!(
f,
"tried to call a function with ABI {:?} using caller ABI {:?}",
callee_abi, caller_abi
),
FunctionArgMismatch(caller_ty, callee_ty) => write!(
f,
"tried to call a function with argument of type {:?} \
passing data of type {:?}",
callee_ty, caller_ty
),
TransmuteSizeDiff(from_ty, to_ty) => write!(
f,
"tried to transmute from {:?} to {:?}, but their sizes differed",
from_ty, to_ty
),
FunctionRetMismatch(caller_ty, callee_ty) => write!(
f,
"tried to call a function with return type {:?} \
passing return place of type {:?}",
callee_ty, caller_ty
),
FunctionArgCountMismatch => {
write!(f, "tried to call a function with incorrect number of arguments")
}
ReallocatedWrongMemoryKind(ref old, ref new) => {
write!(f, "tried to reallocate memory from `{}` to `{}`", old, new)
}
DeallocatedWrongMemoryKind(ref old, ref new) => {
write!(f, "tried to deallocate `{}` memory but gave `{}` as the kind", old, new)
}
InvalidChar(c) => {
write!(f, "tried to interpret an invalid 32-bit value as a char: {}", c)
}
AlignmentCheckFailed { required, has } => write!(
f,
"tried to access memory with alignment {}, but alignment {} is required",
has.bytes(),
required.bytes()
),
TypeNotPrimitive(ty) => write!(f, "expected primitive type, got {}", ty),
PathNotFound(ref path) => write!(f, "cannot find path {:?}", path),
IncorrectAllocationInformation(size, size2, align, align2) => write!(
f,
"incorrect alloc info: expected size {} and align {}, \
got size {} and align {}",
size.bytes(),
align.bytes(),
size2.bytes(),
align2.bytes()
),
InvalidMemoryAccess => write!(f, "tried to access memory through an invalid pointer"),
DanglingPointerDeref => write!(f, "dangling pointer was dereferenced"),
DoubleFree => write!(f, "tried to deallocate dangling pointer"),
InvalidFunctionPointer => {
write!(f, "tried to use a function pointer after offsetting it")
}
InvalidBool => write!(f, "invalid boolean value read"),
InvalidNullPointerUsage => write!(f, "invalid use of NULL pointer"),
ReadPointerAsBytes => write!(
f,
"a raw memory access tried to access part of a pointer value as raw \
bytes"
),
ReadBytesAsPointer => {
write!(f, "a memory access tried to interpret some bytes as a pointer")
}
ReadForeignStatic => write!(f, "tried to read from foreign (extern) static"),
InvalidPointerMath => write!(
f,
"attempted to do invalid arithmetic on pointers that would leak base \
addresses, e.g., comparing pointers into different allocations"
),
DeadLocal => write!(f, "tried to access a dead local variable"),
DerefFunctionPointer => write!(f, "tried to dereference a function pointer"),
ExecuteMemory => write!(f, "tried to treat a memory pointer as a function pointer"),
OutOfTls => write!(f, "reached the maximum number of representable TLS keys"),
TlsOutOfBounds => write!(f, "accessed an invalid (unallocated) TLS key"),
CalledClosureAsFunction => {
write!(f, "tried to call a closure through a function pointer")
}
VtableForArgumentlessMethod => {
write!(f, "tried to call a vtable function without arguments")
}
ModifiedConstantMemory => write!(f, "tried to modify constant memory"),
ModifiedStatic => write!(
f,
"tried to modify a static's initial value from another static's \
initializer"
),
ReallocateNonBasePtr => write!(
f,
"tried to reallocate with a pointer not to the beginning of an \
existing object"
),
DeallocateNonBasePtr => write!(
f,
"tried to deallocate with a pointer not to the beginning of an \
existing object"
),
HeapAllocZeroBytes => write!(f, "tried to re-, de- or allocate zero bytes on the heap"),
ReadFromReturnPointer => write!(f, "tried to read from the return pointer"),
UnimplementedTraitSelection => {
write!(f, "there were unresolved type arguments during trait selection")
}
InvalidBoolOp(_) => write!(f, "invalid boolean operation"),
UnterminatedCString(_) => write!(
f,
"attempted to get length of a null-terminated string, but no null \
found before end of allocation"
),
ReadUndefBytes(_) => write!(f, "attempted to read undefined bytes"),
HeapAllocNonPowerOfTwoAlignment(_) => write!(
f,
"tried to re-, de-, or allocate heap memory with alignment that is \
not a power of two"
),
Unsupported(ref msg) => write!(f, "{}", msg),
ConstPropUnsupported(ref msg) => {
write!(f, "Constant propagation encountered an unsupported situation: {}", msg)
}
PointerOutOfBounds { ptr, msg, allocation_size } =>
write!(
f,
"{} failed: pointer must be in-bounds at offset {}, \
but is outside bounds of allocation {} which has size {}",
msg,
ptr.offset.bytes(),
ptr.alloc_id,
allocation_size.bytes()
),
ValidationFailure(ref err) =>
write!(f, "type validation failed: {}", err),
NoMirFor(ref func) =>
write!(f, "no MIR for `{}`", func),
FunctionAbiMismatch(caller_abi, callee_abi) =>
write!(
f,
"tried to call a function with ABI {:?} using caller ABI {:?}",
callee_abi, caller_abi
),
FunctionArgMismatch(caller_ty, callee_ty) =>
write!(
f,
"tried to call a function with argument of type {:?} \
passing data of type {:?}",
callee_ty, caller_ty
),
TransmuteSizeDiff(from_ty, to_ty) =>
write!(
f,
"tried to transmute from {:?} to {:?}, but their sizes differed",
from_ty, to_ty
),
FunctionRetMismatch(caller_ty, callee_ty) =>
write!(
f,
"tried to call a function with return type {:?} \
passing return place of type {:?}",
callee_ty, caller_ty
),
FunctionArgCountMismatch =>
write!(f, "tried to call a function with incorrect number of arguments"),
ReallocatedWrongMemoryKind(ref old, ref new) =>
write!(f, "tried to reallocate memory from `{}` to `{}`", old, new),
DeallocatedWrongMemoryKind(ref old, ref new) =>
write!(f, "tried to deallocate `{}` memory but gave `{}` as the kind", old, new),
InvalidChar(c) =>
write!(f, "tried to interpret an invalid 32-bit value as a char: {}", c),
AlignmentCheckFailed { required, has } =>
write!(
f,
"tried to access memory with alignment {}, but alignment {} is required",
has.bytes(),
required.bytes()
),
TypeNotPrimitive(ty) =>
write!(f, "expected primitive type, got {}", ty),
PathNotFound(ref path) =>
write!(f, "cannot find path {:?}", path),
IncorrectAllocationInformation(size, size2, align, align2) =>
write!(
f,
"incorrect alloc info: expected size {} and align {}, \
got size {} and align {}",
size.bytes(),
align.bytes(),
size2.bytes(),
align2.bytes()
),
InvalidMemoryAccess =>
write!(f, "tried to access memory through an invalid pointer"),
DanglingPointerDeref =>
write!(f, "dangling pointer was dereferenced"),
DoubleFree =>
write!(f, "tried to deallocate dangling pointer"),
InvalidFunctionPointer =>
write!(f, "tried to use a function pointer after offsetting it"),
InvalidBool =>
write!(f, "invalid boolean value read"),
InvalidNullPointerUsage =>
write!(f, "invalid use of NULL pointer"),
ReadPointerAsBytes =>
write!(
f,
"a raw memory access tried to access part of a pointer value as raw \
bytes"
),
ReadBytesAsPointer =>
write!(f, "a memory access tried to interpret some bytes as a pointer"),
ReadForeignStatic =>
write!(f, "tried to read from foreign (extern) static"),
InvalidPointerMath =>
write!(
f,
"attempted to do invalid arithmetic on pointers that would leak base \
addresses, e.g., comparing pointers into different allocations"
),
DeadLocal =>
write!(f, "tried to access a dead local variable"),
DerefFunctionPointer =>
write!(f, "tried to dereference a function pointer"),
ExecuteMemory =>
write!(f, "tried to treat a memory pointer as a function pointer"),
OutOfTls =>
write!(f, "reached the maximum number of representable TLS keys"),
TlsOutOfBounds =>
write!(f, "accessed an invalid (unallocated) TLS key"),
CalledClosureAsFunction =>
write!(f, "tried to call a closure through a function pointer"),
VtableForArgumentlessMethod =>
write!(f, "tried to call a vtable function without arguments"),
ModifiedConstantMemory =>
write!(f, "tried to modify constant memory"),
ModifiedStatic =>
write!(
f,
"tried to modify a static's initial value from another static's \
initializer"
),
ReallocateNonBasePtr =>
write!(
f,
"tried to reallocate with a pointer not to the beginning of an \
existing object"
),
DeallocateNonBasePtr =>
write!(
f,
"tried to deallocate with a pointer not to the beginning of an \
existing object"
),
HeapAllocZeroBytes =>
write!(f, "tried to re-, de- or allocate zero bytes on the heap"),
ReadFromReturnPointer =>
write!(f, "tried to read from the return pointer"),
UnimplementedTraitSelection =>
write!(f, "there were unresolved type arguments during trait selection"),
InvalidBoolOp(_) =>
write!(f, "invalid boolean operation"),
UnterminatedCString(_) =>
write!(
f,
"attempted to get length of a null-terminated string, but no null \
found before end of allocation"
),
ReadUndefBytes(_) =>
write!(f, "attempted to read undefined bytes"),
HeapAllocNonPowerOfTwoAlignment(_) =>
write!(
f,
"tried to re-, de-, or allocate heap memory with alignment that is \
not a power of two"
),
Unsupported(ref msg) =>
write!(f, "{}", msg),
ConstPropUnsupported(ref msg) =>
write!(f, "Constant propagation encountered an unsupported situation: {}", msg),
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/mir/interpret/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,8 @@ impl<'tcx> AllocMap<'tcx> {
let next = self.next_id;
self.next_id.0 = self.next_id.0.checked_add(1).expect(
"You overflowed a u64 by incrementing by 1... \
You've just earned yourself a free drink if we ever meet. \
Seriously, how did you do that?!",
You've just earned yourself a free drink if we ever meet. \
Seriously, how did you do that?!",
);
next
}
Expand Down
7 changes: 3 additions & 4 deletions src/librustc_mir/interpret/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,10 +580,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap();
Ok((layout.size, layout.align.abi))
}
Some(GlobalAlloc::Memory(alloc)) =>
// Need to duplicate the logic here, because the global allocations have
// different associated types than the interpreter-local ones.
{
Some(GlobalAlloc::Memory(alloc)) => {
// Need to duplicate the logic here, because the global allocations have
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// different associated types than the interpreter-local ones.
Ok((alloc.size, alloc.align))
}
Some(GlobalAlloc::Function(_)) => bug!("We already checked function pointers above"),
Expand Down
18 changes: 8 additions & 10 deletions src/librustc_mir/interpret/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,16 +734,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let variant_index = variants_start
.checked_add(variant_index_relative)
.expect("oveflow computing absolute variant idx");
assert!(
(variant_index as usize)
< rval
.layout
.ty
.ty_adt_def()
.expect("tagged layout for non adt")
.variants
.len()
);
let variants_len = rval
.layout
.ty
.ty_adt_def()
.expect("tagged layout for non adt")
.variants
.len();
assert!((variant_index as usize) < variants_len);
(u128::from(variant_index), VariantIdx::from_u32(variant_index))
} else {
(u128::from(dataful_variant.as_u32()), dataful_variant)
Expand Down
11 changes: 5 additions & 6 deletions src/librustc_mir/interpret/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,12 +391,11 @@ where
// happens at run-time so that's okay.
let align = match self.size_and_align_of(base.meta, field_layout)? {
Some((_, align)) => align,
None if offset == Size::ZERO =>
// An extern type at offset 0, we fall back to its static alignment.
// FIXME: Once we have made decisions for how to handle size and alignment
// of `extern type`, this should be adapted. It is just a temporary hack
// to get some code to work that probably ought to work.
{
None if offset == Size::ZERO => {
// An extern type at offset 0, we fall back to its static alignment.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// FIXME: Once we have made decisions for how to handle size and alignment
// of `extern type`, this should be adapted. It is just a temporary hack
// to get some code to work that probably ought to work.
field_layout.align.abi
}
None => bug!("Cannot compute offset for extern type field at non-0 offset"),
Expand Down
Loading