oxide-auth
oxide-auth copied to clipboard
Fix a bunch of clippy lints
This PR has no real public changes, more of a generic fixing of clippy lints.
I just noticed a bunch of warnings since my RA install runs cargo clippy
instead of cargo check
.
Thought it might be a few nice fixes, especially reducing the complexity of some function signatures by removing unnecessary explicit lifetimes.
- [x] I have read the contribution guidelines
I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.
Right, so about the last remaining lints, they fall in the following categories:
- Clippy complaining about
Result<T, ()>
since it doesn't want a unit type in the position of the error. Doesn't really matter, can be muted. - Missing default implementations for types with an
fn new()
without parameters: Is this desired? - Large size difference between variants: This applies to both errors and the internal state machines. This one actually makes sense. We shouldn't really have the state machines and errors blow up just because of one variant.
Clippy complaining about Result<T, ()> since it doesn't want a unit type in the position of the error. Doesn't really matter, can be muted.
The largest reason this isn't some more specific newtype ('do not leak any failure cause that might be weaponized to the client') is backwards compatibility. It was probably a mistake not to use a more specifically named newtype instead. If clippy lints, it should be fixed with the next major version. Does clippy accept some name type aliases instead?
pub type OAuthOpaqueError = ();
This would be a one-liner to change all the definitions in another PR.
Missing default implementations for types with an fn new() without parameters: Is this desired?
This might be a matter of accepting whether it should be thought of as a 'default'. The other choice here is providing a more descriptive name to the constructor, I tend to agree with clippy that new
and default
are too synonymous but not necessarily with its Default resolution. If you intend to change it, please provide like a one sentence justificiation for each instance of this.
Large size difference between variants: This applies to both errors and the internal state machines. This one actually makes sense. We shouldn't really have the state machines and errors blow up just because of one variant.
Bordering controversy, but the state machines shouldn't be changed. Semantically they should be written like coroutines / generators which just isn't possible right now. I don't think that clippy lints when coroutines consume more space in one particular await than others. This isn't ideal but note that the state is usually stack-allocated anyways and barely moved. Changing the implementation's types or code flow for this lint would at best complicate the readability of a securitiy-critical part, the gains would need to be overwhelming to be convincing imo. It shouldn't really matter greatly for performance (I can be convinced with measurement if you think doing one is worth it). Maybe this should be added to the documentation.
What are the instances on error types (and probably the Template
one)? These could be discussed in more detail.
Oh, it's usually not about changing any paths. It's usually just about boxing a variant of the enum, thereby reducing the stack allocated size, improving performance on memmove
s.
But yeah, if they aren't moved a lot, then that's not relevant.
As for the error, I'll post the exact ones it complains about later.
What are the instances on error types (and probably the Template one)?
The only one Clippy seems to have a problem with is Error
inside the code_grant/authorization.rs
file.
The issue is that the ErrorUrl
variant blows up the error size, where the most of that type's size comes from AuthorizationError
.
AuthorizationError
and the Url
together blow the error up to a stack size of 160 bytes, which seems a little large for an error (and I guess Clippy agrees here).