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

const-oid: an unfortunate tradeoff in PartialEq/Eq definitions #1293

Closed
tarcieri opened this issue Jan 4, 2024 · 3 comments
Closed

const-oid: an unfortunate tradeoff in PartialEq/Eq definitions #1293

tarcieri opened this issue Jan 4, 2024 · 3 comments

Comments

@tarcieri
Copy link
Member

tarcieri commented Jan 4, 2024

#1212 switched from derived PartialEq/Eq impls on ObjectIdentifier to handwritten ones which are generic and allow two ObjectIdentifiers with different backing storage to be compared, which makes it possible to used slice-backed storage for the database.

However, in trying to upgrade the other crates in this repo I ran into the following limitation: only constants with a derived PartialEq/Eq implementation can be used in match statements:

error: to use a constant of type `spki::ObjectIdentifier<Buffer<39>>` in a pattern, `spki::ObjectIdentifier<Buffer<39>>` must be annotated with `#[derive(PartialEq, Eq)]`
   --> pkcs5/src/pbes1.rs:192:13
    |
192 |             PBE_WITH_MD2_AND_DES_CBC_OID => Ok(Self::PbeWithMd2AndDesCbc),
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: the traits must be derived, manual `impl`s are not sufficient
    = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralEq.html for details

To go this route, all of these match statements would need to be rewritten with if statements, which is doable but slightly less than ideal.

Perhaps instead of allowing the storage to be fully generic, it could just be const generic around the size of the backing buffer, which would still allow OIDs in match statements so long as their backing buffer size were the same.

However, the database requires OIDs to all be the same type, so that would preclude the optimization that the slice-backed storage was going for: making sure each OID only uses as much memory as is required, rather than having additional unused overhead.

@tarcieri
Copy link
Member Author

tarcieri commented Jan 4, 2024

For now I plan to revert #1212 which will get us the derived PartialEq/Eq back.

Going forward, I think it may make sense to make ObjectIdentifierRef its own type (i.e. a newtype around a byte slice), and make ObjectIdentifier simply const generic (with a default) around its buffer size, rather than the current approach of ObjectIdentifierRef being a type alias for an ObjectIdentifier which is generic around backing storage. The duplication wouldn't be too bad since much of the core functionality is in the Arcs struct anyway (which defines the iterator over the BER encoding).

That would allow a derived PartialEq/Eq impl, but also overlapping impls to compare ObjectIdentifier and ObjectIdentifierRef (and vice versa).

tarcieri added a commit that referenced this issue Jan 4, 2024
This reverts commit 2f6303b.

Per #1293 to keep the ergonomics of the v0.9 release we need derived
`Eq`/`PartialEq` impls which allow the constants to be used in `match`
expressions.

That's not really possible with the generic backing storage approach
that the v0.10 prereleases have been trying, and also precludes
comparing OIDs with different backing storages.

While an `ObjectIdentifierRef` is still worth experimenting with, it's
clear it isn't ready for prime time yet, so this begins the process of
backing out some changes so the database isn't yet dependent on its
existence.
@tarcieri
Copy link
Member Author

tarcieri commented Jan 4, 2024

#1299 backs out the usages of ObjectIdentifierRef in the database until such a time as the concerns in this issue have been addressed

tarcieri added a commit that referenced this issue Jan 4, 2024
…1299)

This reverts commit 2f6303b.

Per #1293 to keep the ergonomics of the v0.9 release we need derived
`Eq`/`PartialEq` impls which allow the constants to be used in `match`
expressions.

That's not really possible with the generic backing storage approach
that the v0.10 prereleases have been trying, and also precludes
comparing OIDs with different backing storages.

While an `ObjectIdentifierRef` is still worth experimenting with, it's
clear it isn't ready for prime time yet, so this begins the process of
backing out some changes so the database isn't yet dependent on its
existence.
tarcieri added a commit that referenced this issue Jan 4, 2024
Previously the v0.10-pre release series has attempted to make
`ObjectIdentifier` generic around a backing buffer containing the BER
encoding and bounded on `AsRef<[u8]>`.

This approach has a drawback though: we can't use derived
`PartialEq`/`Eq`, which means it isn't possible to use in `match`
expressions anymore, an ergonomics drawback noted in #1293, with the
implementation reverted in #1299.

An alternative way to go for what it was trying to implement: an
`ObjectIdentifierRef` backed by a `&[u8]` is to use a separate struct.
With that approach, there could be overlapping `PartialEq`/`Eq` impls
that would allow the two to be compared.

As a start towards going that route, this gets rid of the generic
backing buffer and opts instead to make the struct const generic around
its size with a default.
tarcieri added a commit that referenced this issue Jan 4, 2024
Previously the v0.10-pre release series has attempted to make
`ObjectIdentifier` generic around a backing buffer containing the BER
encoding and bounded on `AsRef<[u8]>`.

This approach has a drawback though: we can't use derived
`PartialEq`/`Eq`, which means it isn't possible to use in `match`
expressions anymore, an ergonomics drawback noted in #1293, with the
implementation reverted in #1299.

An alternative way to go for what it was trying to implement: an
`ObjectIdentifierRef` backed by a `&[u8]` is to use a separate struct.
With that approach, there could be overlapping `PartialEq`/`Eq` impls
that would allow the two to be compared.

As a start towards going that route, this gets rid of the generic
backing buffer and opts instead to make the struct const generic around
its size with a default.
@tarcieri
Copy link
Member Author

Going to close this. We now have derived PartialEq/Eq back and use owned ObjectIdentifier for the database, so using OIDs in patterns works once again.

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

No branches or pull requests

1 participant