Repository: diesel-rs/diesel (language: rust)

Issue:
AsChangeset and Insertable produce unhelpful error messages
Consider the following code (using `AsChangeset` instead / in addition to `Insertable` produces the same error message):

```rust
#[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: https://github.com/diesel-rs/diesel/issues/2792#issuecomment-846581411


Discussion / hints:
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 https://github.com/rust-lang/rust/issues/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](https://github.com/diesel-rs/diesel/blob/e78ae2311f087d56b7c371b0a203f00bb04583be/diesel_derives/src/meta.rs#L79) to say something more meaningful. We are happy to receive a PR doing that.
I think there is a 3rd option: Make the proc-macro return something like

```rust
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).
> 
> 
> I think there is a 3rd option: Make the proc-macro return something like
> 
> ```rust
> 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!
@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.
@msrd0 Is this still an issue? We released diesel 2.0 after this.
@pksunkara That's not fixed yet.
Yeah, it looks like it's a different error we are getting now. But I thought we already had a deprecated test for this.
@pksunkara At least for me exactly the same error message is generated:

```rust
use diesel::prelude::*;

table! {
    foos {
        id -> Integer,
    }
}

#[derive(Insertable)]
struct Foo {
    #[diesel(column_name = "type")]
    ty: String,
}

```

generates

```
error: proc-macro derive panicked
 --> src/main.rs:9:10
  |
9 | #[derive(Insertable)]
  |          ^^^^^^^^^^
  |
  = help: message: expected identifier
```

the problem is likely this line of code: https://github.com/diesel-rs/diesel/blob/8d64a4011bed57851e3cd747df6d855d44ab9993/diesel_derives/src/attrs.rs#L99
Oh, I was trying out the deprecated derive format. IIRC a different error message came up.


```rust
use diesel::prelude::*;

table! {
    foos {
        id -> Integer,
    }
}

#[derive(Insertable)]
struct Foo {
    #[column_name = "type"]
    ty: String,
}
```

```
warning: #[column_name] attribute form is deprecated
  = help: use `#[diesel(column_name = type)]` instead

error: expected string literal
  --> src/main.rs:11:21
   |
11 |     #[column_name = "type"]
   |                     ^^^^^^
```
@pksunkara That error message is even worse: "type" is most certainly a string literal.

Fix patch:
diff --git a/diesel_derives/src/as_changeset.rs b/diesel_derives/src/as_changeset.rs
index 0344f9c5e81b..5d436cb58084 100644
--- a/diesel_derives/src/as_changeset.rs
+++ b/diesel_derives/src/as_changeset.rs
@@ -140,6 +140,7 @@ fn field_changeset_ty(
     treat_none_as_null: bool,
 ) -> TokenStream {
     let column_name = field.column_name();
+    column_name.valid_ident();
     if !treat_none_as_null && is_option_ty(&field.ty) {
         let field_ty = inner_of_option_ty(&field.ty);
         quote!(std::option::Option<diesel::dsl::Eq<#table_name::#column_name, #lifetime #field_ty>>)
@@ -157,6 +158,7 @@ fn field_changeset_expr(
 ) -> TokenStream {
     let field_name = &field.name;
     let column_name = field.column_name();
+    column_name.valid_ident();
     if !treat_none_as_null && is_option_ty(&field.ty) {
         if lifetime.is_some() {
             quote!(self.#field_name.as_ref().map(|x| #table_name::#column_name.eq(x)))
@@ -175,6 +177,7 @@ fn field_changeset_ty_serialize_as(
     treat_none_as_null: bool,
 ) -> TokenStream {
     let column_name = field.column_name();
+    column_name.valid_ident();
     if !treat_none_as_null && is_option_ty(&field.ty) {
         let inner_ty = inner_of_option_ty(ty);
         quote!(std::option::Option<diesel::dsl::Eq<#table_name::#column_name, #inner_ty>>)
@@ -191,6 +194,7 @@ fn field_changeset_expr_serialize_as(
 ) -> TokenStream {
     let field_name = &field.name;
     let column_name = field.column_name();
+    column_name.valid_ident();
     let column: Expr = parse_quote!(#table_name::#column_name);
     if !treat_none_as_null && is_option_ty(&field.ty) {
         quote!(self.#field_name.map(|x| #column.eq(::std::convert::Into::<#ty>::into(x))))
diff --git a/diesel_derives/src/attrs.rs b/diesel_derives/src/attrs.rs
index 9da55c748dea..540b8a31e2e9 100644
--- a/diesel_derives/src/attrs.rs
+++ b/diesel_derives/src/attrs.rs
@@ -48,6 +48,18 @@ impl SqlIdentifier {
     pub fn span(&self) -> Span {
         self.span
     }
+
+    pub fn valid_ident(&self) {
+        if syn::parse_str::<Ident>(&self.field_name).is_err() {
+            abort!(
+                self.span(),
+                "Expected valid identifier, found `{0}`. \
+                 Diesel automatically renames invalid identifiers, \
+                 perhaps you meant to write `{0}_`?",
+                self.field_name
+            )
+        }
+    }
 }
 
 impl ToTokens for SqlIdentifier {
diff --git a/diesel_derives/src/insertable.rs b/diesel_derives/src/insertable.rs
index 0882d2dd0ec1..c5e3f62f74c7 100644
--- a/diesel_derives/src/insertable.rs
+++ b/diesel_derives/src/insertable.rs
@@ -151,6 +151,7 @@ fn field_ty_serialize_as(
     treat_none_as_default_value: bool,
 ) -> TokenStream {
     let column_name = field.column_name();
+    column_name.valid_ident();
     let span = field.span;
     if treat_none_as_default_value {
         let inner_ty = inner_of_option_ty(ty);
@@ -179,6 +180,7 @@ fn field_expr_serialize_as(
 ) -> TokenStream {
     let field_name = &field.name;
     let column_name = field.column_name();
+    column_name.valid_ident();
     let column = quote!(#table_name::#column_name);
     if treat_none_as_default_value {
         if is_option_ty(ty) {
@@ -198,6 +200,7 @@ fn field_ty(
     treat_none_as_default_value: bool,
 ) -> TokenStream {
     let column_name = field.column_name();
+    column_name.valid_ident();
     let span = field.span;
     if treat_none_as_default_value {
         let inner_ty = inner_of_option_ty(&field.ty);
@@ -228,6 +231,7 @@ fn field_expr(
 ) -> TokenStream {
     let field_name = &field.name;
     let column_name = field.column_name();
+    column_name.valid_ident();
 
     let column: Expr = parse_quote!(#table_name::#column_name);
     if treat_none_as_default_value {


Test patch:
diff --git a/diesel_compile_tests/tests/fail/derive/bad_column_name.rs b/diesel_compile_tests/tests/fail/derive/bad_column_name.rs
index 0b36d9057ac4..ebfe47acb416 100644
--- a/diesel_compile_tests/tests/fail/derive/bad_column_name.rs
+++ b/diesel_compile_tests/tests/fail/derive/bad_column_name.rs
@@ -5,6 +5,8 @@ table! {
     users {
         id -> Integer,
         name -> Text,
+        #[sql_name = "type"]
+        tpe -> Text,
     }
 }
 
@@ -29,4 +31,20 @@ struct User3 {
     name: String,
 }
 
+
+#[derive(Insertable)]
+#[diesel(table_name = users)]
+struct User4 {
+    #[diesel(column_name = "type")]
+    ty: String,
+}
+
+
+#[derive(AsChangeset)]
+#[diesel(table_name = users)]
+struct User5 {
+    #[diesel(column_name = "type")]
+    ty: String,
+}
+
 fn main() {}
diff --git a/diesel_compile_tests/tests/fail/derive/bad_column_name.stderr b/diesel_compile_tests/tests/fail/derive/bad_column_name.stderr
index 7f8c95e6207a..6c004664dc39 100644
--- a/diesel_compile_tests/tests/fail/derive/bad_column_name.stderr
+++ b/diesel_compile_tests/tests/fail/derive/bad_column_name.stderr
@@ -1,19 +1,31 @@
 error: unexpected end of input, expected `=`
-  --> tests/fail/derive/bad_column_name.rs:14:25
+  --> tests/fail/derive/bad_column_name.rs:16:25
    |
-14 |     #[diesel(column_name)]
+16 |     #[diesel(column_name)]
    |                         ^
    |
    = help: The correct format looks like `#[diesel(column_name = foo)]`
 
 error: expected `=`
-  --> tests/fail/derive/bad_column_name.rs:21:25
+  --> tests/fail/derive/bad_column_name.rs:23:25
    |
-21 |     #[diesel(column_name(another))]
+23 |     #[diesel(column_name(another))]
    |                         ^
 
 error: expected string literal
-  --> tests/fail/derive/bad_column_name.rs:28:28
+  --> tests/fail/derive/bad_column_name.rs:30:28
    |
-28 |     #[diesel(column_name = true)]
+30 |     #[diesel(column_name = true)]
    |                            ^^^^
+
+error: Expected valid identifier, found `type`. Diesel automatically renames invalid identifiers, perhaps you meant to write `type_`?
+  --> tests/fail/derive/bad_column_name.rs:38:28
+   |
+38 |     #[diesel(column_name = "type")]
+   |                            ^^^^^^
+
+error: Expected valid identifier, found `type`. Diesel automatically renames invalid identifiers, perhaps you meant to write `type_`?
+  --> tests/fail/derive/bad_column_name.rs:46:28
+   |
+46 |     #[diesel(column_name = "type")]
+   |                            ^^^^^^