Comments (30)
Whether or not this is desirable, this is a big expansion of the dead_code
lint in places it didn't affect before. According to the lint policy currently in FCP, a new lint for this case should at least be considered.
from rust.
So the argument is, the field really is private to the Windows OS, ideally not even the Rust module declaring them would have access?
Maybe we need an #[externally_constructed]
attribute or so. Though #[allow(unused)]
could play the same role, except currently that does not quite work: #126289.
from rust.
@fmease I would agree with all of that if this were all pure Rust code. But it isn't. libc has to represent structs that were defined in C, where zero-initialization is normal. Many of them already have fields used for padding, and the official C documentation requires zero-initialization. Can we please at least use a new name for this lint?
Another case is when the only way to initialize a struct is to first allocate it uninitialized, and then populate it with a C library function or a syscall. To declare such a structure in pure safe Rust, the syscall which initializes it would have to be its constructor. But defining Rusty wrappers for such syscalls is beyond the scope of the libc crate, which is only concerned with structs and function definitions.
from rust.
However, there is still a valid way for a user to construct it: std::mem::zeroed()
Personally speaking I find this argument quite weak because the private field align
is not part of the public API and thus upstream (the author of Foo
) is permitted to change the type of align
at any time without officially breaking any contact (as an aside, the exact output of size_of
, align_of
, Debug
is not considered to come with any stability guarantees; auto trait leakage is a slightly different story though).
For example, the author of Foo
is allowed to change the type of align
to NonZeroI16
rendering zeroed::<Foo>()
in the user code immediate undefined behavior. However that would be the fault of the downstream user of Foo
, they shouldn't've used zeroed
on Foo
in the first place.
from rust.
Ah! Here's a better reproducer:
#[repr(C)]
pub struct Foo(i16);
I don't know enough about the lint parts of the compiler to see how the diff in the regressing PR caused repr(C)
to stop making the field live, but that seems to be the bug.
from rust.
from rust.
from rust.
Certainly less so. However, I'd expect FFI types to have all-pub fields since presumably, the same definition exists in C, where there is no such thing as a private field. It is hardly accurate to say that a field is private when, in fact, code written outside the module -- in a different language even -- can construct values of this type with arbitrary field contents.
from rust.
Arbitrary C code can break those values, so what's the point of preventing Rust code from doing so?
from rust.
from rust.
allow(dead_code)
applying to both unused functions and unused structs is not at all intuitive.
from rust.
There are many thousands of public repr(transparent)
structs with private fields in the windows
crate. Many are created indirectly via the operating system. I'm allowing dead code as a workaround in the short term, but I ideally there would be a more elegant way to handle this. microsoft/windows-rs#3098
from rust.
Update: the libc crate has also opted to silence these warnings.
from rust.
Though #[allow(unused)] could play the same role, except currently that does not quite work: #126289.
#[allow(unused)]
works well now.
from rust.
kinda thought the dead code group had an unused field sublint already.
from rust.
The given reproducer is insufficient, compiling literally just this code in a library crate:
pub struct Foo {
pub i: i16,
align: i16
}
has been producing warnings for years. I don't know what's different about the libc crate, but the bisection using the entire libc crate is pretty easy and points to #125572.
from rust.
Ah! The difference is that the libc crate contains a zillion trait impls that aren't derived. This is a minimal reproducer:
pub struct Foo(i16);
impl Clone for Foo {
fn clone(&self) -> Self {
Self(self.0)
}
}
The change was that trait impls don't make the struct live anymore.
Given that, I do not think it makes a whole lot of sense to be reasoning about constructability via mem::zeroed
here. Or I think that would be a new level of reasoning for this lint.
from rust.
I would agree with all of that if this were all pure Rust code. But it isn't. libc has to represent structs that were defined in C, where zero-initialization is normal. Many of them already have fields used for padding, and the official C documentation requires zero-initialization. Can we please at least use a new name for this lint?
Right, I was going by Foo
being #[repr(Rust)]
in the original repro. It actually being #[repr(C)]
on the other hand changes the situation. I didn't take a look at the libc
issue earlier and didn't see the "recent regression" part ^^'.
from rust.
For variables we have a way of making them "intentionally unused", IMO something similar would make sense here. If you rename the field to _align
, is there still a warning?
IMO we should keep warning for the code as-is. That field is unused, and only in unusual situations does it matter anyway, libc is one of these unusual situations.
from rust.
Oh, I think this is an issue about unconstructed pub structs, like the warnings in the build failure in the downstream issue. For unused fields, we can just underline them. But for pub structs which don't provide pub constructors, adding a new lint to control dead code analysis on pub structs may be a solution.
IMO, adding pub constructors to these pub types or making all fields public is also an acceptable solution.
from rust.
"The type is constructed exclusively by mem::zeroed" is a pretty unusual scenario, I am not convinced it is worth designing our lints around that.
Would you call "the type is constructed exclusively by FFI" a pretty unusual scenario?
from rust.
I, for one, like the fact that you can relieve yourself from arbitrary rust code using the crate being able to break those values by using private fields.
from rust.
If you go there, arbitrary Rust code can break those values too, even with private fields. But the thing is the code on the other side of FFI is not necessarily arbitrary either. And it could also be C++, which has private fields, too.
from rust.
it'd be nice if all these unused field/variant/whatever lints had their own lint so when we want to allow them in a crate because we are pervasively doing that we can just... do that.
from rust.
Could we modify #125572 so that it doesn't lint structs with private fields when repr
is used? That may be a good enough indicator that the struct is going to be used for FFI.
from rust.
If not, then naming the lint something unique (maybe allow(unconstructed)
) would probably be best for libc and similar libraries employing FFI with private struct fields. Having a blanket #![allow(unconstructed)]
would sure beat having to either individually tag each struct with allow(dead_code)
or else ignore all dead code (#![allow(dead_code)]
).
from rust.
Given the discussion above, what is the reason for having private fields in these types?
To reiterate: the point of field privacy in Rust is to allow local reasoning. All code reading or writing those fields must be inside the current module. If you expect the struct to be constructed outside the module (e.g. by the OS), clearly that is not the case, so it seems that private fields don't serve much of a purpose?
from rust.
This approach provides an ideal abstraction to model the type-safety and semantic guarantees offered by certain Windows APIs and goes beyond that which can naturally be modeled in the Rust language. Using public fields would make these APIs substantially more dangerous to use since they could very easily be initialized incorrectly.
from rust.
Externally constructed is a good way to think about it, but ideally we could just get unused
to work in this case and not have to invent a new term.
from rust.
Interestingly, I found some code that technically does an exception for repr(transparent)
and repr(C)
structs/unions, but this is just a few minutes looking at the code, so I don't understand it yet.
rust/compiler/rustc_passes/src/dead.rs
Line 465 in 89a0cfe
from rust.
Related Issues (20)
- Use of match ergonomics affect borrow checking HOT 2
- Blanket, recursive impls cause bad compiler suggestions
- Chains of checked_add could get better codegen
- Review of Reference change regarding receivers and object safety HOT 1
- `Hash` implementation for `Path` ignores path separators HOT 4
- rust panic HOT 1
- Decide on Rule 4 variant for match ergonomics HOT 1
- opt-dist: separate dist-build from profiling HOT 4
- "SIGSEGV: invalid memory reference" when compiling with optimizations HOT 3
- Assigning the wrong fn type to a variable mentions 'pointer' vs 'item'
- ICE: `write_immediate_to_mplace: invalid Scalar layout:` with feature `const_mut_refs` and `const_refs_to_static`
- ICE: called `Option::unwrap()` on a `None` value HOT 1
- ICE: `type mismatch when copying!` HOT 1
- Can't use `vmsr` instruction in `global_asm!` on `armv7r-none-eabihf` without `codegen-units=1` HOT 7
- `tarpaulin` and check-cfg HOT 4
- panic: "failed to open bitcode file" when changing `-Ccodegen-units` in successive compiles.
- VecDeque::split_off has unexpectedly poor performance characteristics when splitting off from the front.
- Unhelpful `.clone()` suggestion when moving a mutable reference HOT 1
- Miscompilation on release profile when `std::ptr::read` is used to cast byte primitive to some tuples in unreachable code paths HOT 4
- Tracking Issue for PathBuf::add_extension and Path::with_added_extension
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rust.