Check for incompatible feature combinations

[Kixunil]

In rust-bitcoin/rust-secp256k1#408 (comment) I describe annoying issue with errors and features and I remember seeing features and conditional enum variants here, so we should check if they have the problem I describe there and fix it if we do.

[thomaseizinger]

Yes, conditional enum variants are a big no-no. I remember this being a known-issue but it just hasn't been addressed yet so thanks for tracking this here!

[Kixunil]

That's something else. Conditional enum variants will be solved with #[non_exhaustive] which is planned after MSRV bump.

[Kixunil]

See this as a potential, likely good solution: https://github.com/LNP-BP/ln-types/pull/7/files#diff-f9353ba4d2ec2320d0f4b3ea4ed6409acebcab6567fd1c4dba72469a87228b5bR363 (whole + chunk)

[apoelstra]

Fixed by #1026

[Kixunil]

That PR does something completely different though.

[apoelstra]

Can you point to any features that, when enabled, break compilation? AFAIK since #1026 there are none.

[Kixunil]

This is an audit issue so someone has to go and check. AFAIK nobody did yet. :)

[apoelstra]

I checked before closing this.

[tcharding]

I checked before closing this.

Issue is open still, should this be closed already?

[Kixunil]

@apoelstra since you referenced a PR that is unrelated to this I don't think you understood the issue. Probably because the comment I referenced describes two separate things and #[non_exhaustive] fixes only one of them. Sorry for confusion.

Hopefully more clearly: for every enum SomeError which references error types from optional external crates we need to conditionally compile impl std::error::Error such that the source() method returns Some IFF there is external-crate-std feature enabled. Where external-crate-std depends on external-crate/std.

For mandatory dependencies we only have to make sure std enables their std feature too.

[apoelstra]

You're right, I don't understand the issue. The issue that #1026 fixes is that adding features would literally break compilation by adding enum variants that people may have been matching on. By labelling these #[non_exhaustive] compilation no longer breaks. So to the extent that this is "incompatible feature combinations", the PR is very much related.

Separately, we shoud check that source() is implemented correctly regardless of feature combinations, yes -- but I'd consider that buggy code rather than "incompatible fetaure combinations" and it wasn't obvious to me that this issue had anything to do with that.

[Kixunil]

Yes it is a bug to have incompatible feature combinations because they can occur unexpectedly.

[apoelstra]

I still don't understand in what sense there are "incompatible feature combinations" :(

[Kixunil]

The situation is very obscure. Consider this code:

Crate foo - lib.rs

pub struct Error {
    // fields
}

#[cfg(feature = "std")]
impl std::error::Error for Error {}

Crate bar - lib.rs

#[non_exhaustive]
pub enum Error {
    #[cfg(feature = "foo")]
    Foo(foo::Error),
    IntegerOverflow,
}

#[cfg(feature = "std")]
impl std::error::Error for Error {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self {
            #[cfg(feature = "foo")]
            Error::Foo(foo) => Some(foo),
            IntegerOverflow => None,
        }
    }
}

Crate bar - Cargo.toml

[features]
default = ["std"]
std = []

[dependencies]
foo = { version = "0.1", default-features = false, optional = true }

Crate baz - Cargo.toml

[dependencies]
foo = { version = "0.1", default-features = false }

Binary crate - Cargo.toml

[dependencies]
bar = { version = "0.1", features = ["foo"] }
baz = "0.1"

This produces compile error saying foo::Error doesn't implement std::error::Error. Annoyingly, if baz didn't depend on foo but started later it would cause breakage in minor version. The reason is bar doesn't know whether foo has std feature turned on or not and it chose the wrong default.

The solution (without bumping MSRV) is to have foo-std feature and if std is on but foo-std off return None from source.

[apoelstra]

Thank you. I understand now.

[Kixunil]

I randomly found one offender: bitcoinconsensus::Error - I did not audit everything.

[apoelstra]

I think this is actually the only error variant which is feature-gated at all.

[tcharding]

In my opinion, having been through all the error types a million times in the last six months this issue can be closed.

[Kixunil]

Yeah, I've seen a bunch of code a number of times and AFAICT all features are additive.