GREsau / okapi

OpenAPI (AKA Swagger) document generation for Rust projects
MIT License
573 stars 102 forks source link

Okapi obfuscates compiler errors #12

Open ThouCheese opened 4 years ago

ThouCheese commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

johnlepikhin commented 4 years ago

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 commented 4 years ago

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

GREsau commented 4 years ago

I'm not sure, but I think the cause of this may be the span bug mentioned in https://github.com/rust-lang/rust/pull/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 commented 4 years ago

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

ThouCheese commented 4 years ago

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

ThouCheese commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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