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

Nice codegen errors #920

Closed
killercup opened this Issue May 22, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@killercup
Member

killercup commented May 22, 2017

Make sure our codegen (#[derive(…)]) generates useful error messages. They should be tested by a compile-fail test.

Especially edge cases like empty structs should be caught where necessary. For example,

#[derive(Insertable)]
#[table_name="users"]
pub struct NewUser {}

currently fails with

no rules expected the token ]

It should probably say something like this instead (maybe with a link to docs or some other feature the user might've been meaning to use):

Failed to derive Insertable on NewUser struct because it has no fields.

We already alert the user to some things, e.g. how we currently don't support generic structs, with code like this.

Feel free to add more examples in the comments.

DorianListens added a commit to DorianListens/diesel that referenced this issue Jun 3, 2017

Add error message for derive(Insertable) on empty struct
This change adds a new error message when attempting to derive
Insertable on a struct with no fields, and a compile-fail test to
ensure it is displayed appropriately.

Fixes diesel-rs#920

DorianListens added a commit to DorianListens/diesel that referenced this issue Jun 4, 2017

Add error message for derive(Insertable) on empty struct
This change adds a new error message when attempting to derive
Insertable on a struct with no fields, and a compile-fail test to
ensure it is displayed appropriately.

See diesel-rs#920
@jaroslaw-weber

This comment has been minimized.

jaroslaw-weber commented Aug 29, 2017

Error messages in diesel look too much like tokio errors - if you mistype or make a mistake it tends to throw out very long errors. although it is scary at first, fixing wasn't that difficult (just start at the top)
Even if you can still read through messages and fix the problem, I miss the simple error messages without diesel.

@willmurphyscode

This comment has been minimized.

Contributor

willmurphyscode commented Sep 14, 2017

Is there work left to be done on this issue? It looks like there are two merged pull requests, one of which specifically addresses #[derive(Insertable)].

@berkus

This comment has been minimized.

berkus commented May 10, 2018

@willmurphyscode there's definitely more to be done:

error: proc-macro derive panicked
  --> src/models.rs:68:10
   |
68 | #[derive(Insertable)]
   |          ^^^^^^^^^^
   |
   = help: message: failed to parse type: failed to parse all tokens

in the recent diesel 1.2.2 and I have no clue what it's talking about.

the struct in question for posterity:

#[derive(Insertable)]
#[table_name = "alerts"]
pub struct NewAlert<'a> {
    pub guid: &'a str,
    pub title: &'a str,
    #[column_name = "type"]
    pub alert_type: &'a str,
    #[column_name = "startdate"]
    pub start_date: NaiveDateTime,
    #[column_name = "expirydate"]
    pub expiry_date: Option<NaiveDateTime>,
    pub faction: Option<String>,
    pub flavor: Option<String>,
}

Upd: it apparently blows on

    #[column_name = "type"]
    pub alert_type: &'a str,

I changed it to use #[column_name = "type_"] but not sure if this is correct - it's not documented anywhere.

@sgrif

This comment has been minimized.

Member

sgrif commented May 10, 2018

Unfortunately, since syn always panics when something fails, there's nothing we can do to improve error messages as a result of giving invalid syntax. I've asked them to look into returning a result in the past (dtolnay/syn#7), but they are uninterested in doing so. I'd open an issue over there if you want to see movement on that front. I'm open to new issues if there's any specific places we can improve the error messages, but I think this can probably be closed at this point.

@sgrif sgrif closed this May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment