Skip to content

Add standard Rust attributes.#24

Merged
PiotrSikora merged 3 commits intoproxy-wasm:masterfrom
yskopets:cleanup/use-non-exhaustive
Jul 1, 2021
Merged

Add standard Rust attributes.#24
PiotrSikora merged 3 commits intoproxy-wasm:masterfrom
yskopets:cleanup/use-non-exhaustive

Conversation

@yskopets
Copy link
Copy Markdown
Contributor

Summary

  • add #[non_exhaustive] attribute to enums to reserve an option to add more values in a backwards-compatible way
  • add #[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]

@yskopets yskopets requested a review from PiotrSikora as a code owner August 10, 2020 21:28
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
@yskopets yskopets force-pushed the cleanup/use-non-exhaustive branch from e3ad939 to d0b5d8d Compare August 10, 2020 21:28
@PiotrSikora
Copy link
Copy Markdown
Member

While technically correct, I'm wondering why do you need #[non_exhaustive]?

Also, once we move to the new ABI, those values will be broken in non-backwards-compatible way, so I'm not sure if adding those guarantees and attributes now is such a good idea.

@yskopets
Copy link
Copy Markdown
Contributor Author

It's about reserving a room for changes that otherwise would be backwards incompatible.

The problem comes form match statements.

Without #[non_exhaustive], if a user creates a match statement with all values of Status, then adding a new Status value will be a breaking change.

You will have to do a major release of the library to stick to semantic versioning.

@lizan
Copy link
Copy Markdown
Member

lizan commented Aug 12, 2020

@yskopets I don't think we need non-exaustive for all enums, for some of them it doesn't make sense. If ABI changes, those match statements need to be rewritten or it will break at runtime.

Copy link
Copy Markdown
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding #[non_exhaustive], I think that we could add it for now (but we should remove it once the ABI is frozen), though it doesn't really makes sense to do an exhaustive match on any of the enums defined here (other than the LogLevel).

Regarding the other traits, Copy and Clone are fine, but I'm a bit confused as to why do we need PartialEq, Eq and Hash. Could you elaborate on each one?

@yskopets
Copy link
Copy Markdown
Contributor Author

to why do we need PartialEq, Eq and Hash

Overall, implementing these traits is advised by the Rust API Guidelines (interoperability).

Practically, Eq and PartialEq come in pair and necessary for assertions like

#[test]
fn test_log_level_eq() {
    assert_ne!(LogLevel::Critical, LogLevel::Debug);
}

All 3 Eq, PartialEq, Hash are mostly necessary in the context of user-defined composite types, which are common in unit testing.

…e-non-exhaustive

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora changed the title add standard Rust attributes Add standard Rust attributes. Jul 1, 2021
Copy link
Copy Markdown
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

FWIW, I still think that #[non_exhaustive] shouldn't be necessary long-term, but the gRPC PRs (#100, #101) added new values, so I'm going to merge this to avoid breaking existing plugins.

@PiotrSikora PiotrSikora merged commit edb5cd5 into proxy-wasm:master Jul 1, 2021
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.

3 participants