fix W::bits signature

[couchand]

Fixes #475.

[rust-highfive]

r? @ryankurte

(rust_highfive has picked a reviewer for you, use r? to override)

[burrbull]

This code doesn't fix problem in my view, as REG:Writer and W are different types.

svd2rust/src/generate/register.rs

Line 84 in 1a2bf65

pub struct W(crate::W<#name_uc_spec>);

[burrbull]

I don't see any way (without traits) to implement generic bits function that will be work as before.

If we don't found better solution, I think it is better just delete generic unsafe fn bits.
And replace

svd2rust/src/generate/register.rs

Line 168 in 29129c0

self.0.bits(bits);

with:
self.bits = bits

[couchand]

I'd agree that it is a bit surprising at first that this works. I submitted the PR and went off to bed, but then had one of those moments lying in bed and thinking about it when I felt a sinking feeling that the fix would break everything. I got back up and did a bunch more testing to verify that nothing breaks.

The key insight is that there's a deref coercion happening. Since REG::Writer implements DerefMut<Target=W>, a mutable reference to the more specific type is just as good as a mutable reference to the more general type. However, that's not true the other direction (we have to explicitly .into() for that to work). This is why we need to relax the bounds at 1, 2, and 3.

A collection of resources I assembled while verifying that this was correct:

• playground
• DerefMut trait ref (specifically: Values of type &mut T are coerced to values of type &mut U)
• the book Chapter 15.2

[couchand]

(As an aside, all this chicanery would be avoided with #478).

[burrbull]

See what problem I see:

    reg.write(|w| { w
         .bits(10)    // <-- DerefMut here
         .field1().set_bit()  // <-- error! generic W<REG> doesn't have `field1`
    });

[couchand]

Ah, that's a fair point. Though it would be a fairly marginal use-case, wouldn't it? Setting the bits for the whole register explicitly and then overriding just one field?

Is that example from real code somewhere?

[couchand]

Oh, thanks @burrbull for bringing up the specialized version of bits at

svd2rust/src/generate/register.rs

Lines 167 to 170 in 29129c0

	pub unsafe fn bits(&mut self, bits: #rty) -> &mut Self {
	self.0.bits(bits);
	self
	}

In all the hubbub I had forgotten about it. That would certainly explain why in any normal code #475 didn't manifest as a bug, since the compiler will pick the impl directly on the specific type before even trying to deref.

So #475 will only manifest in code written to be generic over Writable, so I'm almost certainly the only one affected.

To close the loop, your example at #477 (comment) will be fine, since w refers to the specific register writer, so w.bits refers to the version that returns self, not the generic version implemented here.

[burrbull]

Is that example from real code somewhere?

I already saw something like this.

On the other hand this should work:

reg.write(|w| {
      w.bits(10);    // <-- DerefMut here, but drop ref
      w.field1().set_bit()
});

Can you test this?

[couchand]

I already saw something like this.

Do you mean just that you've seen that pattern before or that it exhibited a bug? My current thinking is that there's no bug in that pattern, since the w.bits that gets called there is not the one added by this PR.

I've not been able to trigger #475 on master or any suspected bug from this PR in any testing (and it does seem like it would have to be both or neither, not one or the other). If you have an example of somewhere that has an issue, please point me to it.

[couchand]

Ping @burrbull. I don't believe there's a bug in any code except my own. Happy to look at any counter-example that you might come up with.

Are there any other outstanding issues with this PR? Thanks!

[couchand]

@burrbull, have you been able to identify any issues with this change? To reiterate, I'm pretty confident that the only one affected by the bug is me.

@ryankurte it looks like you're still the @rust-highfive reviewer here, would you by chance be able to take a look at this PR? Thanks!

[ryankurte]

thanks for the fix! i am not 100% across the related issue but, looks okay to me.

bors r+

[bors]

Build succeeded:

• ci-linux (1.37.0, Nordic, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Atmel, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Freescale, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Fujitsu, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Holtek, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Nordic, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Nuvoton, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, NXP, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, RISC-V, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, rustfmt, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, SiliconLabs, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Spansion, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, STMicro, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, Toshiba, x86_64-unknown-linux-gnu, linux)
• ci-linux (stable, x86_64-apple-darwin, osx)
• ci-linux (stable, x86_64-pc-windows-msvc, windows)

[couchand]

Thanks for the reviews!