Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark MappingTarget as #[non_exhaustive] for increased semver flexibility #2012

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

Enselic
Copy link
Collaborator

@Enselic Enselic commented Jan 2, 2022

This will allow us to add new enum variants in the future without breaking
semver compatibility. See
https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute

Since we already added an enum variant since v0.18.3, now is a good time to mark
it as #[non_exhaustive]. Even if we currently always bump major version (since we are on 0.x.x) there will come a time when we don't, and then it's nice to have this prepared.

…ibility

This will allow us to add new enum variants in the future without breaking
semver compatibility. See
https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute

Since we already added an enum variant since v0.18.3, now is a good time to mark
it as `#[non_exhaustive]`.
@sharkdp
Copy link
Owner

sharkdp commented Jan 3, 2022

Nice. If I understand correctly, we still get exhaustiveness-checking within this crate ("Within the defining crate, non_exhaustive has no effect."). And the change to src/bin/bat/main.rs is purely cosmetical?

Or do we only keep exhaustiveness-checking within bat-the-library?

@Enselic
Copy link
Collaborator Author

Enselic commented Jan 3, 2022

It is correct that within the lib, we don't need any wildcard. This code compiles when added to the lib:

fn in_lib() {
    let e = MappingTarget::MapToUnknown;
    match e {
        MappingTarget::MapTo(_) => todo!(),
        MappingTarget::MapToUnknown => todo!(),
        MappingTarget::MapExtensionToUnknown => todo!(),
    }
}

The change in src/bin/bat/main.rs was not primarily cosmetical, but required to fix the build. Since the binary is its own crate, non_exhaustive takes effect. The same code as above in the lib fails to compile in the bin:

fn in_bin() {
    let e = MappingTarget::MapToUnknown;
    match e {
        MappingTarget::MapTo(_) => todo!(),
        MappingTarget::MapToUnknown => todo!(),
        MappingTarget::MapExtensionToUnknown => todo!(),
    }
}
error[E0004]: non-exhaustive patterns: `_` not covered
  --> src/bin/bat/main.rs:39:11
   |
39 |     match e {
   |           ^ pattern `_` not covered
   |

@sharkdp
Copy link
Owner

sharkdp commented Jan 3, 2022

Since the binary is its own crate

Ok, I wasn't sure about the precise boundaries/terminology here. I thought we have a "lib" part of the bat crate and a "bin" part of the bat crate.

Thank you for the explanation!

@Enselic Enselic merged commit 6852898 into sharkdp:master Jan 4, 2022
@Enselic Enselic deleted the non_exhaustive-MappingTarget branch January 4, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants