The Wayback Machine - https://web.archive.org/web/20211217011432/https://github.com/diesel-rs/diesel/issues/2793
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

AsChangeset and Insertable produce unhelpful error messages #2793

Open
msrd0 opened this issue May 23, 2021 · 4 comments
Open

AsChangeset and Insertable produce unhelpful error messages #2793

msrd0 opened this issue May 23, 2021 · 4 comments

Comments

@msrd0
Copy link

@msrd0 msrd0 commented May 23, 2021

Consider the following code (using AsChangeset instead / in addition to Insertable produces the same error message):

#[derive(Insertable)]
struct Foo {
        #[column_name = "type"]
        ty: String
}

This code is wrong because eventhough the table has a column called type, the auto-generated schema.rs file renames that to type_ to avoid using a keyword as an identifier, and that's the column name that has to be used. Unfortunately, the error message produced by the macro has an unhelpful message and an incorrect span:

error: proc-macro derive panicked
 --> src/main.rs:3:10
  |
3 | #[derive(Insertable)]
  |          ^^^^^^^^^^
  |
  = help: message: expected identifier

Ideally, the error message would look similar to this:

error[E0560]: struct `Foo` has no field named `type`
 --> src/main.rs:7:9
  |
5 |        #[column_name = "type"]
  |                        ^^^^^^ help: a field with a similar name exists: `type_`

This error message does not only help when trying to use a name that is an invalid identifier in rust, but also when there is a typo in the column name.

Related: #2792 (comment)

@msrd0 msrd0 added the bug label May 23, 2021
@weiznich
Copy link
Member

@weiznich weiznich commented May 25, 2021

That's for opening this issue. After having spend some time to dig into this I fear there is no much we can do here. From a technical point of view there are two possibilities to generate this "nicer" error message":

  1. The proc macro itself generates the diagnostic. Unfortunately this is nothing that is possible yet using stable rust. See rust-lang/rust#54140. Even with that API implemented it wouldn't be possible to emit the additional help line, as again there is no API to query if such a similar named field exists. Having both API's would be really nice, but that's unfortunately nothing that can be fixed on diesels side only. Consider working on the compiler side of things here, if you want work on improvements.
  2. The proc macro itself can generate "valid" rust code, where the compiler itself emits the corresponding error message, as this would be done for any other rust code. Unfortunately this is not possible here, as type is never a valid identifier. Technically speaking we could likely emit code containing type as field name, but this will generate an error message just stating that type is no valid identifier which would at least point to the correct span. Unfortunately this comes with an really large cost: We couldn't use syn anymore, which would mean we need to implement support for parsing and generating rust code all on our own. That is not something I consider to do, at least not without much larger gains.

The only thing that could actually be done is to improve this error message to say something more meaningful. We are happy to receive a PR doing that.

@msrd0
Copy link
Author

@msrd0 msrd0 commented May 25, 2021

I think there is a 3rd option: Make the proc-macro return something like

syn::Error::new(self.meta.span(), format!(
    "Expected valid identifier, found `{name}`. Diesel automatically renames invalid identifiers, perhaps you meant to write `{name}_`?",
    name = self.name().segments.first().unwrap().ident.clone()
)).into_compile_error()

This will produce an error message like

error: Expected valid identifier, found `type`. Diesel automatically renames invalid identifiers, perhaps you meant to write `type_`?"
 --> src/main.rs:7:9
  |
5 |        #[column_name = "type"]
  |                        ^^^^^^

This is much better than using panic!, as rust can point to the exact code location instead of the #[derive(Insertable)] statement, and we can provide a help message inside the error message (we can't output help messages on stable rust afaik).

@Pyzyryab
Copy link

@Pyzyryab Pyzyryab commented Jul 30, 2021

I think there is a 3rd option: Make the proc-macro return something like

syn::Error::new(self.meta.span(), format!(
    "Expected valid identifier, found `{name}`. Diesel automatically renames invalid identifiers, perhaps you meant to write `{name}_`?",
    name = self.name().segments.first().unwrap().ident.clone()
)).into_compile_error()

This will produce an error message like

error: Expected valid identifier, found `type`. Diesel automatically renames invalid identifiers, perhaps you meant to write `type_`?"
 --> src/main.rs:7:9
  |
5 |        #[column_name = "type"]
  |                        ^^^^^^

This is much better than using panic!, as rust can point to the exact code location instead of the #[derive(Insertable)] statement, and we can provide a help message inside the error message (we can't output help messages on stable rust afaik).

Why doesn't anyone give him some credit? It's a nice solution!

@weiznich
Copy link
Member

@weiznich weiznich commented Jul 31, 2021

@Pyzyryab Feel free to submit a PR implementing this strategy + adding some tests to show that it works via tests. Otherwise our issue tracker is for our contributors to track bugs and regressions. It's not a place for general discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
@weiznich @msrd0 @Pyzyryab and others