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

Unused endpoints should produce a lint warning #116

Merged
merged 4 commits into from
Jun 9, 2021
Merged

Unused endpoints should produce a lint warning #116

merged 4 commits into from
Jun 9, 2021

Conversation

ahl
Copy link
Collaborator

@ahl ahl commented Jun 4, 2021

Fixes #114

@ahl ahl requested a review from smklein June 7, 2021 17:32
@ahl
Copy link
Collaborator Author

ahl commented Jun 7, 2021

@smklein can I ask you to take a quick look? I think you've been in this vicinity before

Copy link
Contributor

@smklein smklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change seems reasonable, test seems useful - hopefully this helps more dropshot users.

Although I doubt it'll hit anyone hard, this is technically a breaking change - we should probably add a note in the changelog.

@@ -212,8 +212,18 @@ fn do_endpoint(
})
.collect::<Vec<_>>();

// For reasons that are not well understood unused constants that use the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spooky, but at least we have the negative test.

@@ -0,0 +1,11 @@
error: constant is never used: `unused_endpoint`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the behavior is "constant is never used", instead of "endpoint not registered", this presumably wouldn't fire with a pub symbol, right?

(I think that's probably okay, we can't catch everything, but just checking my understanding)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Good catch. If the function is pub then I'm not sure there's much we can do. This is unfortunate as one is more likely to lose track of an endpoint in a larger program and one is also more likely to have endpoints in several files in larger programs.

Perhaps in the future we could consider adding endpoints to a distributed slice (https://github.com/dtolnay/linkme) to avoid manual registration entirely.

At least this change helps in simple cases as folks ramp up on Dropshot...

@ahl ahl merged commit df3b363 into main Jun 9, 2021
@ahl ahl deleted the use_me branch June 9, 2021 20:52
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.

Should warn or error on unregistered handlers
2 participants