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

[6/n] [dropshot-endpoint] add extra validation for parameters #1011

Open
wants to merge 11 commits into
base: sunshowers/spr/main.dropshot-endpoint-add-extra-validation-for-parameters
Choose a base branch
from

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented May 16, 2024

We are forced to do some of this validation for trait-based servers, and
catching things like non-static lifetime parameters early can make this a nicer
experience. We don't try and capture everything, just some of the things that
are either required for, or closely related to, trait-based servers.

Depends on #1009.

Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the title [dropshot-endpoint] add extra validation for parameters [5/n] [dropshot-endpoint] add extra validation for parameters May 16, 2024
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the title [5/n] [dropshot-endpoint] add extra validation for parameters [6/n] [dropshot-endpoint] add extra validation for parameters May 24, 2024
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Is it the case today that we're not doing the appropriate checks for lifetimes?


/// A representation of the RequestContext type.
#[derive(Clone, Eq, PartialEq)]
enum RqctxTy<'ast> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is what we're going to add to for the trait-based approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what we're going to add to for the trait-based approach?

Yeah.

@@ -834,6 +1044,57 @@ mod tests {
);
}

// These argument types are close to being invalid, but are actually valid.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it that these are odd but valid types? or could you elaborate on this proximity to invalidity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expanded the comment here to describe the changes which would make these types invalid.

@sunshowers
Copy link
Contributor Author

Is it the case today that we're not doing the appropriate checks for lifetimes?

It won't succeed at compiling, but it'll produce some pretty rough error messages: https://gist.github.com/sunshowers/c83254c5f1002b410ae05f9426c78b2d

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
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