Blanket impls for &mut can cause trait methods to be preferred over inherent methods.

[Dirbaio]

trafficstars

I found one ugly side effect of the &mut blanket impls while experimenting updating stm32f4xx-hal's GPIO.

it's making the trait methods take priority over the inherent methods in some cases: https://github.com/Dirbaio/stm32f4xx-hal/commit/091d9450fc92e85e5ad0271db6a761ff2959bef0#diff-a49fe1ed5910761241be27a9c10bb8e11cac30ec0a80b839278ae3a1630384bcR102

It seems to be a consequence of the autoderef rules: https://github.com/rust-lang/rust/issues/26007 . It happens when you mix &self with &mut self. Since the blanket impl is for &mut self, it matches "first" before Rust tries to autoderef.

I don't think this is especially bad. It might be annoying in HAL code, but non-generic end user code won't be importing the traits if they're indending to call inherent methods.

What should we do?

• Change StatefulOutputPin to require &mut self
• Remove that blanket impl.
• Nothing.

[eldruin]

Ugh. Ugly indeed. Nice catch, though. What I would not do would be to change the StatefulOutputPin to get a &mut self since the methods have no side effects. Now, why do we have a blanket impl for &mut T if the StatefulOutputPin methods only need &self? Was this an oversight or would the blanket impl not be selected somehow due to the trait being StatefulOutputPin : OutputPin (and OutputPin requiring &mut self)?

[Dirbaio]

Now, why do we have a blanket impl for &mut T if the StatefulOutputPin methods only need &self?

StatefulOutputPin requires OutputPin, so impl StatefulOutputPin for &T would require impl OutputPin for &T which can't be because OutputPin requires &mut self.

Perhaps another option is to make StatefulOutputPin not require OutputPin, and add a blanket impl for &T....? I'm not sure it fixes all cases of accidentally prioritzing the trait method though.

[Dirbaio]

fixed by #547

[GrantM11235]

For future reference, #547 didn't fix this, and it now affects InputPin as well as StatefulOutputPin.

We decided that this is okay because the problem is rare and the solution is easy: replace self.is_set_high() with (*self).is_set_high()