Fixing a Clippy crash
17 min read
3 weeks ago I set out to fix a crash in Clippy, this is what I learned along the way. I hope this blog post will be useful for other people diving into Clippy and maybe serve as motivation if things get difficult.
- Meet the crash
- Understanding the context
- Fixing the bug
Meet the crash
The crash has been reported in this Clippy issue. It is demonstrated with the following valid Rust program:
When executed with rustc, it compiles, but it produces the following ICE when it is executed with Clippy:
Traits.. generics.. lifetimes!? Oh no
The crashing snippet from above shows why I have been.. not so quick with fixing this. I have been avoiding dealing with explicit lifetime annotations since I started with Rust almost a year ago. Further, I have never been in a position where it made sense to implement my own traits, especially not generic traits, so it was pretty easy to avoid.
It’s safe to say that, at this point, I don’t really understand what this program means. Let’s figure it out step by step.
This is a trait called
Foo with no associated items.
Foo is also generic
over any type
A and it declares a lifetime parameter
'a. By itself, this
does not mean anything, but it allows for potential implementors of this trait
to use the lifetime
'a and the generic type
Here we define an implementation of our trait
Foo for any type
T. Again, we
declare a lifetime
'b and a generic type
T so that we can use them to implement
impl also declares no associated items.
The last line contains the cause of the Clippy crash. Again we declare that the
function is going to use a lifetime
'b. We then define a parameter that can
be any type that implements
Foo with the lifetime of
'b and a type of
The backtrace contains some first pointers where we can have a further look.
Let’s go through the backtrace from top to bottom.
This tells us where the crash was reported from inside rustc. I’m noting this down to have a look at the code later.
Next follow 30 to 40 lines of plumbing, like:
For now, I think all these lines are irrelevant to the crash in Clippy.
Further down we find more useful information:
This tells us from where Clippy invoked the chain of methods that caused the Crash. In other words, one of these places is probably using rustc internals incorrectly.
It also shows which lint caused the crash to happen.
needless_pass_by_value checks for functions that take arguments by value and
don’t consume those arguments. Clippy should suggest to pass these arguments by
And indeed, in our crashing example, we pass the argument by value:
We are now sure that this code should trigger the
but it crashes instead. Next, we are going to try and find out more about what’s
going on internally.
First steps to fixing a Clippy bug
The first thing we are going to do is making the crash easily reproducible by creating a test case. Luckily someone already provided a minimal crashing example in the issue. A minimized example is always super helpful.
We are going to add the code to the
run-pass test suite of Clippy. The
run-pass suite will fail if any of the code inside fails to compile.
Here’s our failing code:
We can now run the test easily using:
As expected, this fails to compile and crashes with the backtrace from above. Good!
Debugging with println!
Now that we are able to quickly run the test, we can start to debug the
crash. I usually use plain
println! calls to get all the possibly relevant
values printed to stdout.
In this case we want to know what we pass through to rustc before
predicate_must_hold is called:
Using the command from above, this will recompile the changed code and execute our test. Here is the result:
Understanding the context
Maybe you can spot something in the debug output already. However, I first want
to find out what all these new types mean.
I have dealt with
ParamEnv before, but
Obligation are new to me.
It looks like it’s time to learn about some rustc internals. Nice!
Learning about rustc internals usually means searching around in the
rustc-guide and in the rust compiler documentation
and putting all the information together.
ⓘ New concepts
ParamEnv is short for Parameter Environment. It contains information about the trait bounds. It can be used to check wether a type implements a certain trait, for example.
As I understand it, a `Binder` associates variables with arguments where they were defined. For example in the closure `|a, b| a + b`, the `a` and `b` in the `a + b ` are bound to the closure, and the closure signature `|a, b|` is a binder for the names `a` and `b`.
This also applies to lifetime parameters.
A predicate related to traits. A predicate in the rust type system describes something about the given type. For example, whether it is well formed, object safe or a subtype with another type. As far as I understand it, a TraitPredicate is a predicate that says whether a type implements a given trait.
Source: rustc-guide: Ty
An Obligation represents some trait reference (e.g. int:Eq) for which the vtable must be found. The process of finding a vtable is called "resolving" the Obligation. This process consists of either identifying an impl (e.g., impl Eq for int) that provides the required vtable, or else finding a bound that is in scope.
A vtable is a struct that contains the function pointers to the trait's associated functions. The pointers point directly to the concrete machine code for each method in the implementation.
With all that in mind, let’s review our debug output again:
What’s especially interesting about our debug output is that the
TraitPredicate that is different to the
Obligation in the
The reason for that is probably that we build the
Obligation in the lint code
ourselves, while the
ParamEnv is the one of the actual test case.
At this point I have been looking into this bug for more than two weeks in my free time. I have to say thanks to @arielb1 who pointed me in the right direction after I improved the output of this ICE in rust.
Fixing the bug
@arielb1 confirmed my suspicion: The
TraitPredicate in the
Obligation should contain the lifetime, too:
input_types, which drops the lifetime parameters out of
Bar, turning an OK
<S as Bar<'b, ()>>trait-ref into a not-OK
<&S as Bar<()>>trait ref. The not OK trait-ref has its indexes wrong because it’s missing a lifetime - at index 0, it should have a lifetime, not a type. So you get an ICE.
With that information, the fix was only a few hours away and turned out as a 9 line change:
You may wonder how this is fixing the issue exactly. We first removed the
input_types() call. This means we have to find a replacement to
get the collection of lifetimes and type parameters for the given trait
Let’s have a look at the source of
input_types() and see how it gets the type
parameters of the trait reference:
Substs is what we’re looking for. It contains the different parameters
of the type, including lifetimes. For example the
substs of a
i32> would more or less look like this:
In our example,
S: Foo<'b, ()>, that would be
[S, ReEarlyBound(0, 'b), ()].
The first element is the name of the type parameter,
S. The second one is the
name of the lifetime,
'b. The last one is unit.
S: Foo<'b, ()> will result in
substs directly, gives us what we want:
[S, ReEarlyBound(0, 'b), ()].
Instead of calling
types() on substs, we just iterate over the
directly and avoid the crash.
With the crash fixed, this concludes the post. I hope you were able to learn something new from this. Maybe I also inspired you to have a go at working on Clippy. If that’s the case I encourage you to look through the good first issue label, pick one that seems easy to you and dig in =)
You can find the final PR here.10 Oct 2018