This was originally a github comment that, due to my writing style (and desire to teach), spun a bit out of control (or at least, didn’t match the overall style we try to follow for comments on rust-lang/rust repository).
So now, this is a blog post instead, where I get to do fun things like have better control over formatting and whatnot. (Based on what feedback I get on the post, maybe I will next work to promote it to a chapter in the rustc dev guide.)
Issue #81658, “Invalid `field is never read: ` lint warning”, is a bug filed against rustc.
These are mentorship instructions for how to go about fixing the bug. They are also an overview of some steps I use to get started looking at a bug like this.
Issue #81658, “Invalid `field is never read: ` lint warning”, is a bug filed against rustc. It was filed at the beginning of February 2021.
The heart of the problem is that a diagnostic lint was firing, saying a
particular field was never read, and mentioning the diagnostic resulted due to a
warn(dead_code) lint setting.
It was quickly prioritized as a medium priority issue (as lints often are).
The issue reporter was quite concerned that the issue reached the
beta channel without being addressed, because they felt that the diagnostic text
could be misinterpreted as saying that the field in question is 100% dead
code, which would imply that it could be removed with no effect to the semantics
of the program.
The issue reporter asked for the lint to be narrowed in scope to not fire in
these cases, as they are false positives, and the
rustc compiler has a
principle of eschewing lints that signal false positives.
The observable behavior of lints is under the purview of the Rust language design team. the issue was nominated for discussion at one of the team’s weekly triage meetings.
After some discussion, the conclusion of the language design team was that we were not going to revise the lint itself to be more restrictive in when it fires. Of course the discussion was a bit more nuanced than what I’ve written here. There was a bit more discussion about what the lint would do in an ideal world, in terms of firing only on “truly dead” code, but the realities of systems programming (as well as the halting problem) mean we do not live in that ideal world. We saw the example code, and the collective reaction was “the lint is fine here. The right thing for the developer to do in such cases is to prepend an underscore to their field or variable name”.
But when I was reviewing the issue comment thread after the meeting, something
resonated with me: The issue reporter pointed out another principle of
If rustc warns about something, it should mention a clear way how to fix the warning.
I certainly agree with that. And, importantly: The end consensus of the language
team (“prepend an underscore”) is not reflected anywhere in the diagnostic text issued
So, I posted a note saying we should fix the latter aspect of the diagnostic. It is almost always easier to fine-tune the english text of a diagnostic rather than revise when the diagnostic fires. (Or, at least, it is less risky to our users, in terms of what impact a mistake might have in each of the two cases.) It will be an easy bug to fix. Its a good way for new contributors to get involved, and I’ll mentor it.
Thus, these are my mentorship instructions.
Our goal is to improve the diagnostic in this case to inform the user of their options: If the field is truly unused, they can remove it. If the field is used in some implicit way, then they can silence the lint itself, or they can prepend its name with an underscore (which serves as a signal that the field has some purpose, despite it not being read by expressions visible in the control flow of the program).
Test driven development
There are a number of example usages of these “unread fields” given in the comment thread.
It would be a good idea to start by writing simple test cases based on those examples. The main ones I see discussed in this thread and thus might deserve specific focus (both in test cases and in potentially variations in the diagnostic output) are these two:
Values whose types implement
Drop, and where that drop has some side-effect beyond the value itself (e.g. mutexes that unlock on drop).
Unsafe code accesses, including (as a very important case), fields that end up being referenced by foreign code that we reach via the foreign function interface (FFI).
Hacking on the diagnostic
Now that you have made tests, you should be able to run the compiler on them and observe the current (sub-par) diagnostic output, which will say something like:
There are different strategies for tracking down where to look in the rustc source for where these diagnostics are emitted.
I do hold a special spot in my own heart for running
rr reversible debugger, a spot that is probably meme-worthy at
Some bad initial ideas include:
- fire up
rustcunder a debugger and step through its behavior, or
- make a local build of
rustcwith debug logging turned on and then use
RUSTC_LOG=debugto observe all of that logging output.
The two options above are both bad ideas because they will take forever to wade through.
Here’s a better idea, though primitive: search the source code for the
preferably with a fast tool like
that has better defaults, like automatically recursing through subdirectories of the source tree
without needing to pass an extra option asking for that behavior.
Two immediate guesses for
patterns we can derive from the diagnostic are “dead_code” (the name of the
lint) and the text “field is never read:”
You’ll quickly discover that grepping for the string “dead_code” yields a lot of
false-positives (because the lint is
#![allow]‘ed heavily in the Rust source
tree). You can filter many of those out with a regexp pattern like:
[^\(]dead_code. But even then, its a lot to sift through.
At least, I saw something on the order of 56 matches, and I think we can do better.
We could try to refine the pattern further, but before we try that, lets try the other pattern that we saw.
The second pattern, “field is never read”, has far fewer false-positives:
1 2 3
The problem now is that we don’t have any positives. What happened?
Well, the diagnostic strings are constructed dynamically (so that they can include information about the source code, like field names). And as part of that construction, they are often abstracted in other ways, e.g. to treat the word “field”/“variable” as just another parameter.
So, in this scenario, I recommend making the search pattern less specific: drop the word “field”:
1 2 3 4 5 6 7 8 9 10
More promising, but if you look carefully, none of these strings could actually yield the diagnostic we’re seeking. So our pattern is still too specific.
Searching for “is never” will work, but it ends up yielding 53 matches (and 53≈56, which I rejected above as being too many to wade through).
So here’s the trick: Include the Rust formatting curly braces in the search string:
1 2 3 4 5
Now we have found what is almost certainly the exact place where we need to edit the code to improve the diagnostic in question.
What kind of code to write
Here is my short-term advice:
It would be good to focus first on a simple patch that is easy to backport to
It turns out
that we are almost certainly going to revert PR #81473 rather than backport other fixes to beta, but the advice about making the first version here simple still holds.
I.e. all the bells and whistles about inspecting
Drop do not need to be in beta.
A simple general change to the diagnostic text is all I would want (and I would expect the team to balk at approving a larger beta backport).
After the code is written
You need to run your tests. You’ll probably discover that even if the tests you added pass (which is pretty unlikely, unless you are very good at predicting how the diagnostic you added will actually get emitted), a whole bunch of other tests are going to fail.
Review them. Most of them are going to be failing because the diagnostic output you added is not expected by the test.
You could try to update all those tests by hand. But that is not a good use of developer effort.
x.py test --bless to automatically generate the new expected output, and then
manually review the output of
git diff to double-check that the changes to the expected output
all make sense.
Ping me on Zulip if you want to help with this, and I’ll be happy to provide guidance (either here on github, or directly via text chat).