librasn / rasn

A Safe #[no_std] ASN.1 Codec Framework
Other
195 stars 47 forks source link

Replace type aliases with newtypes #76

Open repnop opened 2 years ago

repnop commented 2 years ago

Instead of directly reexporting types such as BigInt as Integer and Bytes as OctetString, I think it makes a lot more sense to newtype these and provide a more stable interface so that the implementations can be changed without affecting the public API. For instance, being able to switch from using num-bigint to ibig for parsing variable-sized integers could provide a significant speed increase, avoiding unnecessary allocations for small integers. However, changing the Integer alias to IBig would be a breaking change. Reexporting types also makes the docs quite a bit more confusing in my opinion, as the documentation for the types ends up using a completely, totally different type name, for example when I go to view the docs for OctetString, I'm met with the following docs:

A cheaply cloneable and sliceable chunk of contiguous memory.

Bytes is an efficient container for storing and operating on contiguous slices of memory. It is intended for use primarily in networking code, but could have applications elsewhere as well.

Bytes values facilitate zero-copy network programming by allowing multiple Bytes objects to point to the same underlying memory.

Which is a bit jarring when the type signature is named something completely different IMO

XAMPPRocky commented 2 years ago

Thank you for your issue! This is definitely planned, as I'm currently working on implementing PER and constraints, and constraints I want to eventually move the constraints impl to being completely const based, which requires having our own types to be able to add const parameters.

However, changing the Integer alias to IBig would be a breaking change.

I do want to be clear however that this is acceptable. This crate is still very firmly in 0.x phase of development. So preventing breakage is not priority compared to iterating and improving rasn's features and performance.

Which is a bit jarring when the type signature is named something completely different IMO

Yes, seperately I'd like to be able to massively revamp documentation, to provide more both informative and normative information on ASN.1, such that rasn's documentation is the primaary if not only source you need to use ASN.1 successfully.

Nicceboy commented 1 year ago

I think I could start working with this [newtype support] after I finish OER codec, if that is okay.

I am still quite new for Rust, so I might need some design help.

But the basic idea would be to use Crate level features and with that to change types?

For the higher type (Integer) and user facing API, we write our own functions, and then with crate features possibly map to different inner types.

XAMPPRocky commented 1 year ago

I am still quite new for Rust, so I might need some design help.

Right now we're mostly limited by Rust's const capabilities, once we have the ability to use enums it will make the design much more intuitive and straightforward.

You can see a simple version of this with the fixed octet string type.

https://github.com/XAMPPRocky/rasn/blob/a7a00c76d702e530a14d517d830adc899bd31dc4/src/types/strings/octet.rs#L7-L8

Nicceboy commented 2 months ago

Decoding functionality for any kind integer type was added in https://github.com/librasn/rasn/pull/256

I am trying to implement the encoding part now, but there are some design choices to be made.

Currently, Integer type is still type alias for BigInt. This prevents optimized encoding for primitive types and avoidance of heap allocation.

We could either remove the whole Integer type and use generic with the trait IntegerType and any type which implements the trait can be used as Integer type or we use enum like

#[derive(Debug, Clone, Ord, Hash, Eq, PartialEq, PartialOrd)]
#[non_exhaustive]
pub enum Integer {
    Primitive(PrimitiveInteger),
    Big(BigInt),
}

Or maybe something else. @XAMPPRocky do you have any ideas?

XAMPPRocky commented 2 months ago

I think the small integer optimisation of using an enum is the way to go forward.