Today the Rust analyzer is giving me an error that I find illogical, and I don't know if it's really correct or a false positive.
here's the portion of the code that gives the error :
#[poem::async_trait]
impl<E: Endpoint> Endpoint for JwtMiddlewareImpl<E> {
type Output = E::Output;
async fn call(&self, mut req: Request) -> Result<Self::Output> {
if let Some(value) = req
.headers()
.get(AUTHORIZATION)
.and_then(|value| value.to_str().ok())
.filter(|value| value.starts_with("Bearer "))
.map(|value| &value[7..])
{
//TODO Token decrypt..
let mut claims = Claims::default();
claims.sub = Some("test".to_string());
/* !!! --------------> the error occurs here */
req.extensions_mut()
.insert(Session::new(None, true, Some(value.clone().to_string()), Some(claims)));
} else {
// no token found
req.extensions_mut()
.insert(Session::new(None, false, None, None));
}
self.ep.call(req).await
}
}
this is the error message :
error[E0502]: cannot borrow `req` as mutable because it is also borrowed as immutable
--> app/src/security/xxxxxx.rs:80:13
|
56 | if let Some(value) = req
| ______________________________-
57 | | .headers()
| |______________________- immutable borrow occurs here
...
80 | req.extensions_mut().insert(Session::new(
| ^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
...
83 | Some(value.clone().to_string()),
| ------------- immutable borrow later used here
value.clone().to_string() it's this part that's causing the error, (I added clone() just to make sure it's not a borrowing problem.)
but if I put the "value" in a temporary variable, the error disappears :
// value2 : is the temporary variable
let value2 = value.to_string();
req.extensions_mut()
.insert(Session::new(None, true, Some(value2), Some(claims)));
I think it's a bug in the analyzer, or does it have a logical explanation?
Well, does it behave in the same way if you feed it to the compiler, rather than RA? (RA is often inconsistent with the compiler, and I have yet to see a real-life case when it's right and rustc is wrong.)
Without having the full code to check, no I think RA is correct.
You immutably borrow req on line 56. This borrow is later used for value.clone().to_string(). Thus, the borrow must be maintained until then. Before that point, you try to mutably borrow req in order to call extensions_mut(). This is because the call to extensions_mut happens before the arguments to insert are evaluated. Adding clone doesn't make it not a borrowing problem. It's the order of operations that matters here.
When you use value2, you are forcing the to_string call to happen before the call to extensions_mut, which allows the compiler to drop the immutable borrow from line 56 in order to take a new mutable borrow for the extensions_mut call.
it's not really a "bug" per se, it's a limitation (intentional or unintentional, I don't know) of the current borrow checker in the language when it comes to &mut self. consider the following code snippets:
struct Foo {}
impl Foo {
fn x(&self) -> isize { todo!() }
fn y(&mut self) -> isize { todo!() }
fn z(&mut self, x: isize) { todo!() }
}
fn test() {
let mut foo = Foo {};
// this compiles, as expected
foo.z(foo.x());
// this, however, doesn't compile
//foo.z(foo.y());
// this compiles as a workaround:
let y = foo.y();
foo.z(y);
}
I can't find a link to the original source, but from memory, I think it can be summarized as: when evaluate a "method call expression", the type checker need to first resolve the method itself (e.g. is it defined intrinsic on the type, or is it defined in a trait, or is there's auto ref/deref cocersion desugar needed, etc), during this procedure, the self is forced to be "borrowed" (and they decided to make it an immutable borrow, for reasons I can't remember), then the arguments of the call are evaluated. because the whole expression is within a single statement, the arguments conceptually are evaluated in the same scope, so the borrow check will complain if one of the argument borrows self mutably.
if anyone has a link to the the technical explanation, please share it.
Exactly how extensive is "the pattern", i.e., what is allowed and what is not? I'm not sure anyone knows, given that allowing it was accidental and there is no spec.
There are at least a couple other future compatibility errors where the current plan is to accept them, which makes the warning problematic as it asserts the lint will definitely become an error, and people use that to drive their reasoning in compiler development. A recipe for more unsoundness IMO. ↩︎
it's really interesting what you say,
Now I understand the origin of this error, but is there a way to avoid it without using an intermediate variable (causing a slight loss of performance just to bypass the borrow checker ) ?
Does it actually cause a loss of performance? Very likely it is optimized away. Even if it isn't, do you think it will matter in an application making async HTTP requests?
Satisfy, not bypass. Personally I almost always find the memory safety guarantees worth it even when they do incur some small cost (e.g. checked indexing). That includes cases where I know it's not necessary.
But performance isn't the real issue here, is it? I imagine you didn't write the code out thinking "this will be more performant", but instead wrote the code out without intermediate variables because it was the more intuitive thing to do. Then you got an error you found illogical.
I think the real issues are balancing ergonomics and intuition with optimizability and understandability while remaining confident that soundness is ensured. Compiler contributors also have to maintain it without losing that soundness or accidentally breaking sound, compiling code.
And by understandability, I mean both of code that errors but also code that compiles. On the one hand, the lack of two-phased borrows confuses people who expect the arguments to be evaluated before the call, as happened to you; hence why they were created in the first place.[1] On the other hand, two-phased borrows makes the borrow checking model more complicated, and confuses people trying to learn the model. They find code that doesn't seem like it should compile as a &mut and & are allowed to co-exist, say, and then report a bug in the borrow checker.
Two phased borrows isn't even well defined at this point, and that's a bad place to be. There's not a specification of what is accepted and what isn't, much less an explanation of why. Making all&mut borrow two-phased would be simpler then a laundry list of exceptions IMO, and I believe it would cover your OP.
I'm not sure as to the implications for optimizations or if others agree it's more understandable and learnable though.
Niko's blog post linked above contains a good explanation of why things work this way. ↩︎
It's very relevant what you're saying, when I asked about performance, I wasn't talking about this "HTTP" example, but in general.
What I find strange is the non-intuitive nature of the error message. If I hadn't had the idea of using a temporary variable, I'd probably have struggled to find the solution
What I consider a bypass is adding unnecessary code just to make code work that could normally work without it.
I understand what you've said, it's a price to pay for better code security, but I'd like this kind of case to be better documented, with a clearer error message, and why not make the compiler more flexible (either allow certain cases like this, or add a temporary variable in precompilation, then let the optimizer do its job).
Rust is a beautiful language, powerful and flexible enough, but sometimes I have the impression that we're thinking more like a compiler and getting away from the real problem.
It would be nice if it was smart enough to link to some longer-form explanation of why the general case cannot be accepted (like the top of Niko's post... updated for whatever the rules actually are).
This is the only part I might quibble with, but it's mostly a matter of perspective I think. I agree it's a workaround.
(It doesn't "normally work" in Rust, i.e. without some extended two-phase borrow, because it violates the aliasing rules -- which are exploitable by LLVM for example -- and is thus UB.)
and getting away from the real problem.