Note that this is against the 0.32 branch and intended for a point release, so it conflicting with other PRs isn't a big deal. We can do something smarter, like you link to, for new releases.

Pull Request Test Coverage Report for Build 10406462165

Details

• 55 of 59 (93.22%) changed or added relevant lines in 21 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 83.113%

Totals
Change from base Build 9429226336:	0.0%
Covered Lines:	19200
Relevant Lines:	23101

💛 - Coveralls

Let's not change it just to change it again later.

This isn't a change, this is an API relaxation for the current version as people upgrade to it. And, honestly, not sure we'll ever upgrade past 0.32, so I don't care too much about future breakage :).

I don't care too much about future breakage

You might not but I do.

Sure, if you care about future breakage and are already on 0.32 this doesn't effect you at all :)

I was thinking that this cannot be really non-breaking and add the proper fix because of trait objects. So instead I suggest these changes:

• add consensus_decode_slow() method to Decodable using the Read bound call into the fast one with one-byte buffer
• by default call consensus_decode_slow() from the fast method
• Move the current implementations that don't use the buffer from fast method to slow one

Downstream users that want to use Read will call into slow and they can change it later if they want to.

I offer making a PR for this.

I'd prefer _unbuffered rather than _slow. It is both less loaded and takes longer to type, so it should discourage use while being easier to find. But concept ACK doing that.

The doc should still say it may be slower though.

I was thinking that this cannot be really non-breaking and add the proper fix because of trait objects.

Oh? I don't see why this is breaking.

I think it might be ok. This code works on the playground:

fn myfn<T: std::io::Read + ?Sized>(test: &mut T) {
    // blah blah
}

fn myfn2<T: std::io::BufRead + ?Sized>(test: &mut T) {
    // blah blah
}

fn main() {
    let mut works_with_old_code = std::io::BufReader::new(std::io::stdin());
    
    myfn2(&mut works_with_old_code as &mut dyn std::io::BufRead);
    myfn(&mut works_with_old_code as &mut dyn std::io::BufRead);
}

Yea, I went and did this and it seems to work:

trait A {}
trait B: A{}
struct BI {}
impl A for BI {}
impl B for BI {}

fn fb<T: B + ?Sized>(t: &T) {}
fn fa<T: A + ?Sized>(t: &T) {}

fn dynb(t: &dyn B) { fa(t); }

fn main() {
    let b = BI {};
    fb(&b);
    fa(&b);
    let bt: &dyn B = &b;
    fa(bt);
    fb(bt);
}

I meant adding the associated types.

Ah, right, okay. Yea, the "proper fix" (assuming we don't end up on a push-based decode total API rewrite anyway) I assume we'd want to do in a new release anyway, leaving this for 0.32.

I would like to do this in 0.32.

I think that a proper fix would involve having an API that can take either a Read or a BufRead and (potentially) does different things depending what's available. (As I said in the other issue I don't think it's reasonable to force direct users to propagate BufRead bounds outward, nor is it reasonable to force them to use a Read->BufRead adaptor that "buffers" a byte at a time.)

So we will need to break things relative to master anyway, and changing 0.32 to use Read won't result in extra breakage.

The failing CI look unrelated to this PR.

I rebased this locally on top of #3418 and it seems good to go in on top of that.