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

Keep SDK annotated functions in scope #1499

Merged
merged 1 commit into from
May 18, 2023

Conversation

fibonacci1729
Copy link
Contributor

No description provided.

@fibonacci1729 fibonacci1729 requested a review from lann May 16, 2023 19:53
@fibonacci1729
Copy link
Contributor Author

Closes #1498

#func

match #func_name(req.try_into().expect("cannot convert from Spin HTTP request")) {
match self::#func_name(req.try_into().expect("cannot convert from Spin HTTP request")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the self annotation necessary? I'm not sure why it would be

Copy link
Collaborator

Choose a reason for hiding this comment

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

It lets you name your function req.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I didn't think about local variables... 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Really we should probably be even more careful with our item paths in here with e.g. ::spin_http

Copy link
Contributor Author

Choose a reason for hiding this comment

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

::spin_http won't work; as of rust 2018 paths prefixed with :: must be followed by the name of a crate. We could however say crate::spin_http to prevent a collision with a dependency named spin_http.

ref: https://doc.rust-lang.org/reference/paths.html#path-qualifiers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rylev great point; the dunder possibility slipped my brain and i think that is a better choice in this case.

Copy link
Contributor Author

@fibonacci1729 fibonacci1729 May 17, 2023

Choose a reason for hiding this comment

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

Unfortunately, it doesn't seem like the standalone option is surfaced in the version of the wit-bindgen macro we are using however alternatively we could do something like:

mod __spin_http {
  wit_bindgen_rust::export!({src["spin_http"]: #HTTP_COMPONENT_WIT});
  pub use spin_http::*;
}

struct SpinHttp;

impl __spin_http::SpinHttp for SpinHttp {
  ...
}

Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errr actually i am remembering why this won't work.

The code wit-bindgen generates expects a type SpinHttp in the namespace outside of the generated module, i.e. (from the generated module):

...
let result = <super::SpinHttp as SpinHttp>::handle_http_request

The SDK is currently structured the way it is because the version of wit-bindgen we are using dictates a certain module structure. However, i still think crate::..., albeit less idiomatic, mitigates any collision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

crate:: assumes that the generated code is at the top of the crate. This would prevent using the macro anywhere but at the top of the crate namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah good catch.

I think this will work:

quote!(
  #func

  mod __spin_http {
    wit_bindgen_rust::export!({src["spin_http"]: #HTTP_COMPONENT_WIT});

    struct SpinHttp;

    impl self::spin_http::SpinHttp for SpinHttp {
      ...
    }
  }
)

Signed-off-by: Brian H <brian.hardock@fermyon.com>
@fibonacci1729 fibonacci1729 merged commit f484e2e into fermyon:main May 18, 2023
@fibonacci1729 fibonacci1729 deleted the sdk-1498 branch May 18, 2023 15:23
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.

3 participants