oxide-auth icon indicating copy to clipboard operation
oxide-auth copied to clipboard

Fix a bunch of clippy lints

Open aumetra opened this issue 1 year ago • 6 comments

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.

aumetra avatar Dec 21 '23 13:12 aumetra

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.

aumetra avatar Dec 21 '23 13:12 aumetra

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.

HeroicKatora avatar Dec 21 '23 15:12 HeroicKatora

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.

HeroicKatora avatar Dec 21 '23 15:12 HeroicKatora

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.

HeroicKatora avatar Dec 21 '23 16:12 HeroicKatora

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 memmoves.

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.

aumetra avatar Dec 21 '23 16:12 aumetra

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).

aumetra avatar Dec 21 '23 16:12 aumetra