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

Fix a bunch of clippy lints #183

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

aumetra
Copy link
Contributor

@aumetra aumetra commented Dec 21, 2023

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.

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.

@aumetra
Copy link
Contributor Author

aumetra commented Dec 21, 2023

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.

@HeroicKatora
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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.

@aumetra
Copy link
Contributor Author

aumetra commented Dec 21, 2023

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
Copy link
Contributor Author

aumetra commented Dec 21, 2023

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

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.

None yet

2 participants