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

Okapi obfuscates compiler errors #12

Open
ThouCheese opened this issue Jan 28, 2020 · 15 comments
Open

Okapi obfuscates compiler errors #12

ThouCheese opened this issue Jan 28, 2020 · 15 comments

Comments

@ThouCheese
Copy link
Contributor

Thank you for this project! I've been struggling with documenting an API I build, and keeping that documentation up to date. I'm trying to add okapi to my project to have the documentation be created automatically. Unfortunately, I'm running into some issues:

As it stands, Rocket is able to provide neat Rust error messages. Consider the following example:

#![feature(decl_macro, proc_macro_hygiene)]

use rocket::{get, routes};
use rocket_contrib::json::Json;

#[derive(serde::Serialize)]
struct Response {
    reply: String,
}

#[get("/")]
fn my_controller() -> Json<Response> {
    Json(Response {
        reply: 0,
    })
}


fn main() {
    rocket::ignite()
        .mount("/", routes![my_controller])
        .launch();
}

This fails with a compile error:

[me@mycomputer tmp]$ cargo check
    Checking tmp v0.1.0 (/home/me/code/rust/tmp)
error[E0308]: mismatched types
  --> src/main.rs:17:16
   |
17 |         reply: 0,
   |                ^
   |                |
   |                expected struct `std::string::String`, found integer
   |                help: try using a conversion method: `0.to_string()`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `tmp`.

However, when the code is changed to use Okapi:

#![feature(decl_macro, proc_macro_hygiene)]

use rocket::get;
use rocket_contrib::json::Json;
use rocket_okapi::{openapi, routes_with_openapi};
use schemars::JsonSchema;

#[derive(serde::Serialize, JsonSchema)]
struct Response {
    reply: String,
}

#[openapi]
#[get("/")]
fn my_controller() -> Json<Response> {
    Json(Response {
        reply: 0,
    })
}


fn main() {
    rocket::ignite()
        .mount("/", routes_with_openapi![my_controller])
        .launch();
}

The error message changes:

[me@mycomputer tmp]$ cargo check
    Checking tmp v0.1.0 (/home/me/code/rust/tmp)
error[E0308]: mismatched types

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `tmp`.

The useful information about what caused the compilation error is gone, making it really hard to debug. Is there any way to get it back?

@ThouCheese
Copy link
Contributor Author

I did some digging and it seems that the problem could be resolved using quote_spanned!. I don't understand the proc_macro API well enough to make the change myself however.

@GREsau
Copy link
Owner

GREsau commented Feb 17, 2020

tl;dr: a fix will be out soon

So this was a weird one - I think the root cause is rust-lang/rust#43081

You can see the same problem with the following repro, where both no_op1 and no_op2 should both pass through tokens unmodified (although no_op2 does it by parsing them in then quoting them back out):

///// MACRO CRATE: /////
#[macro_use]
extern crate quote;
extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_attribute]
pub fn no_op1(_: TokenStream, input: TokenStream) -> TokenStream {
    input
}

#[proc_macro_attribute]
pub fn no_op2(_: TokenStream, input: TokenStream) -> TokenStream {
    let parsed_input = syn::parse::<syn::Item>(input).unwrap();
    quote!(#parsed_input).into()
}

///// APP CRATE: /////
use codegen::{no_op1, no_op2};

#[no_op1]
#[no_op1]
fn one_one() {
    let one_one: i32 = "nan";
}

#[no_op1]
#[no_op2]
fn one_two() {
    let one_two: i32 = "nan";
}

#[no_op2]
#[no_op1]
fn two_one() {
    let two_one: i32 = "nan";
}

#[no_op2]
#[no_op2]
fn two_two() {
    let two_two: i32 = "nan";
}

The output of cargo check is then:

error[E0308]: mismatched types
  |
  = note: expected type `i32`
             found type `&'static str`

error[E0308]: mismatched types
 --> app\src\main.rs:6:24
  |
6 |     let one_one: i32 = "nan";
  |                        ^^^^^ expected i32, found reference
  |
  = note: expected type `i32`
             found type `&'static str`

error[E0308]: mismatched types
  --> app\src\main.rs:18:24
   |
18 |     let two_one: i32 = "nan";
   |                        ^^^^^ expected i32, found reference
   |
   = note: expected type `i32`
              found type `&'static str`

error[E0308]: mismatched types
  --> app\src\main.rs:24:24
   |
24 |     let two_two: i32 = "nan";
   |                        ^^^^^ expected i32, found reference
   |
   = note: expected type `i32`
              found type `&'static str`

Note that one_two loses its span information, obfuscating the error. no_op1/no_op2 interact exactly the same way the openapi/get attributes do. The good news is that this also gives us a way to work-around it: by making the openapi attribute behave more like no_op2 (by passing the tokens through syn/quote), span information will be retained.

I'll try to get the fix out imminently

GREsau added a commit that referenced this issue Feb 17, 2020
@GREsau
Copy link
Owner

GREsau commented Feb 17, 2020

This should be fixed in rocket_okapi v0.3.2, which is now published on crates.io - could you check that it works for you?

@ThouCheese
Copy link
Contributor Author

This does indeed fix the error messages! Thank you for this fix.

@johnlepikhin
Copy link

johnlepikhin commented Feb 19, 2020

There are still some issues and I cannot pinpoint the source. Here is example of code with obfuscated error message:

#[openapi]
#[get("/")]
pub fn test(
) -> Option<Option<()>> {
    asdf
}

If you remove the second Option<> error reporting will work as expected.

@ThouCheese
Copy link
Contributor Author

You are right, I just encountered this issue myself 😄. It seems that any generic type nested with another generic does not get handled well. Reopening the issue.

@ThouCheese ThouCheese reopened this Feb 19, 2020
@GREsau
Copy link
Owner

GREsau commented Feb 20, 2020

I'm not sure, but I think the cause of this may be the span bug mentioned in rust-lang/rust#48258. And just like it says in this comment on the related issue, our issue goes away when wrapping the inner generic type in parentheses e.g.

#[openapi]
#[get("/")]
pub fn test() -> Option<(Option<()>)> {
    asdf
}

And that helped me find a work-around for this too - if the #[openapi] proc macro wraps the inner generic type(s) in parentheses, then the needed span information is preserved and compiler errors are reported correctly. This feels like a horrible hack, but it seems to do the job...

I'll publish the new fix to crates.io tonight

GREsau added a commit that referenced this issue Feb 20, 2020
@GREsau
Copy link
Owner

GREsau commented Feb 20, 2020

Done - https://crates.io/crates/rocket_okapi 0.3.3 is available, hopefully this will fix everything!

@ThouCheese
Copy link
Contributor Author

It now indeed works for (deeply) nested generic types. Thank you for you update!

@ThouCheese
Copy link
Contributor Author

ThouCheese commented Mar 3, 2020

In the latest installment of this issue, I have found another instance of #[openapi] not producing the right error messages. It happens when I specify in the #[post] macro that the payload should be parsed, but then forget to add the argument to the function. For example

#[post("/", data = "<body>")]
fn myroute() -> String {
    "hi".to_string()
}

This produces the following error:

error: unused dynamic parameter
  --> src/main.rs:7:21
   |
7  | #[post("/", data = "<body>")]
   |                     ^^^^^^
   |
note: expected argument named `body` here
  --> src/main.rs:8:1
   |
8  | / fn myroute() -> String {
9  | |     "hi".to_string()
10 | | }
   | |_^

If I add in #[openapi] like so

#[openapi]
#[post("/", data = "<body>")]
fn myroute() -> String {
    "hi".to_string()
}

I get the following error message:

error: macros that expand to items must be delimited with braces or followed by a semicolon
 --> src/main.rs:6:1
  |
6 | #[openapi]
  | ^^^^^^^^^^
  |
help: change the delimiters to curly braces
  |
6 | {[openapi}
  | ^        ^
help: add a semicolon
  |
6 | #[openapi];
  |           ^

error: unused dynamic parameter
  --> src/main.rs:7:21
   |
7  | #[post("/", data = "<body>")]
   |                     ^^^^^^
   |
note: expected argument named `body` here
  --> src/main.rs:8:1
   |
8  | / fn myroute() -> String {
9  | |     "hi".to_string()
10 | | }
   | |_^

error: Could not find argument body matching data param.
 --> src/main.rs:6:1
  |
6 | #[openapi]
  | ^^^^^^^^^^

@ThouCheese ThouCheese reopened this Mar 3, 2020
@ThouCheese
Copy link
Contributor Author

ThouCheese commented Mar 12, 2020

Additionally, it is still possible to create unspanned error message using some functional constructs on Results and Options. Interestingly, the following code produces normal error messages:

#![feature(decl_macro, proc_macro_hygiene)]

use rocket_okapi::openapi;
use rocket::post;

#[openapi]
#[post("/sign")]
fn sign() {
    Some(1).and_then(|i| 1);
}

fn main() { }

The error message here is

error[E0308]: mismatched types
 --> src/main.rs:9:26
  |
9 |     Some(1).and_then(|i| 1);
  |                          ^
  |                          |
  |                          expected enum `std::option::Option`, found integer
  |                          help: try using a variant of the expected enum: `Some(1)`
  |
  = note: expected enum `std::option::Option<_>`
             found type `{integer}`

error: aborting due to previous error

However, if I trigger a slightly different error message like so:

#![feature(decl_macro, proc_macro_hygiene)]

use rocket_okapi::openapi;
use rocket::post;

#[openapi]
#[post("/sign")]
fn sign() {
    Some(1).and_then(|| Some(1));
}

fn main() { }

Then the error message is placed at the beginning of the file:

error[E0593]: closure is expected to take 1 argument, but it takes 0 arguments
  |
help: consider changing the closure to take and ignore the expected argument
  |
1 | |_|#![feature(decl_macro, proc_macro_hygiene)]
  | ^^^

error: aborting due to previous error

I sorry for spamming this issue without providing any help on fixing the issue, maybe if I have time somewhere by the end of this month I'll look into the proc macro api and the codebase of rocket_okapi.

GREsau added a commit that referenced this issue May 24, 2020
I don't believe this is necessary in newer nightlies.
It also causes its own warnings (#27).
@GREsau
Copy link
Owner

GREsau commented May 24, 2020

@ThouCheese I removed the above workarounds in v0.5.0, and I can no longer reproduce any of these cases. I assume this is due to internal rustc improvements, but I can't be sure.

Do you still encounter any span problems? I'm using rustc 1.45.0-nightly (7ebd87a7a 2020-05-08)

@ThouCheese
Copy link
Contributor Author

Hi @GREsau,

They all generate sensible errors except for one! The one that remains is that if I do this:

#[openapi]
#[get("/", data = "<body>")]
fn myroute() -> String {
    "hi".to_string()
}

I get the following error (in addition to the warning that Rocket generates):

error: Could not find argument body matching data param.
  --> src/main.rs:13:1
   |
13 | #[openapi]
   | ^^^^^^^^^^
   |
   = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

@ralpha
Copy link
Collaborator

ralpha commented May 25, 2020

You get that error because there should be an argument body in the handler/function.
See: https://rocket.rs/v0.4/guide/requests/#body-data

To indicate that a handler expects body data, annotate it with data = "<param>", where param is an argument in the handler. The argument's type must implement the FromData trait.

So it should look something like this:

#[openapi]
#[get("/", data = "<body>")]
fn myroute(body: SomeType) -> String {
    "hi".to_string()
}

@ThouCheese
Copy link
Contributor Author

Yes I understand that the code is not correct, but since rocket doesn't refuse to compile I think that neither should okapi.

mautamu pushed a commit to mautamu/okapi that referenced this issue Apr 23, 2021
I don't believe this is necessary in newer nightlies.
It also causes its own warnings (GREsau#27).
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

No branches or pull requests

4 participants