See if we can catch common errors with a lint

[sgrif]

Some of the most common issues that people run into are:

• Column order doesn't match struct field order
• Column type doesn't match struct field type
• Struct doesn't have a field for all the selected columns

I'd eventually like to allow the order to differ from the table definition, but I'm not confident that we will ever be able to do that. While I do want to make sure that the errors from the compiler alone give at least enough information for experienced users to figure out what's going wrong, we can do better. I've often thought about trying to do more validation inside of #[derive(Queryable)]. However, the issue there is that we can't get information about the code other than the item being annotated, meaning we can't ask things about the table mod. We also have no database connection, so we can't go to the schema to ask questions either.

One issue I have with just validating from that derive though is that it assumes:

• The user is using infer_schema! to define that table
• The struct is meant to be one-to-one with a table that has the same name
• The select clause used to construct this will contain all the columns from a given table

And I've tried to avoid all of those assumptions in the design. So I think a lint makes a lot more sense, as it has access to way more information (meaning we don't need a database connection), and they can be much more granularly allowed by the user.

I'm unsure if these lints would be able to occur early enough to catch the most common cases, but we should definitely explore this option. Ideally I'd like to avoid using a database connection for this, and have it only affect types which implement HasTable, find the module based on the value of that associated type, and base the linting off the value of AllColumns and SqlType. I'd like to try and provide a specific error message with suggestions on how to correct for all of the following, probably in this order:

• struct order doesn't match column order (this one should only fail if all the field names match a column name, and the number of fields matches the number of columns)
• struct does not have a field for each column
• two or more struct fields have the same name as a column but appear in swapped positions
• field type doesn't match column type (error message should ideally include which Rust types could map to the column type, or what SQL type they could change their column to match the rust type)

One other slightly less common mistake I've seen people make, which we could definitely catch with a lint is people thinking that they should be putting diesel::types::Timestamp on their struct directly.

[killercup]

Yeah, having lints for that would be awesome, even if to just add notes on Rust errors. Maybe we can give users some suggestions on how to make their code cleaner as well (though that is by far not as important as actually catching and explaining errors). IIRC, clippy (cc @llogiq) contains a lint to catch invalid regexes, maybe they have a story for supporting other external crates? (Though regex is, to me, "practically in std"!)

…

Am 12.01.2017 um 15:16 schrieb Sean Griffin ***@***.***>: Some of the most common issues that people run into are: • Column order doesn't match struct field order • Column type doesn't match struct field type • Struct doesn't have a field for all the selected columns I'd eventually like to allow the order to differ from the table definition, but I'm not confident that we will ever be able to do that. While I do want to make sure that the errors from the compiler alone give at least enough information for experienced users to figure out what's going wrong, we can do better. I've often thought about trying to do more validation inside of #[derive(Queryable)]. However, the issue there is that we can't get information about the code other than the item being annotated, meaning we can't ask things about the table mod. We also have no database connection, so we can't go to the schema to ask questions either. One issue I have with just validating from that derive though is that it assumes: • The user is using infer_schema! to define that table • The struct is meant to be one-to-one with a table that has the same name • The select clause used to construct this will contain all the columns from a given table And I've tried to avoid all of those assumptions in the design. So I think a lint makes a lot more sense, as it has access to way more information (meaning we don't need a database connection), and they can be much more granularly allowed by the user. I'm unsure if these lints would be able to occur early enough to catch the most common cases, but we should definitely explore this option. Ideally I'd like to avoid using a database connection for this, and have it only affect types which implement HasTable, find the module based on the value of that associated type, and base the linting off the value of AllColumns and SqlType. I'd like to try and provide a specific error message with suggestions on how to correct for all of the following, probably in this order: • struct order doesn't match column order (this one should only fail if all the field names match a column name, and the number of fields matches the number of columns) • struct does not have a field for each column • two or more struct fields have the same name as a column but appear in swapped positions • field type doesn't match column type (error message should ideally include which Rust types could map to the column type, or what SQL type they could change their column to match the rust type) One other slightly less common mistake I've seen people make, which we could definitely catch with a lint is people thinking that they should be putting diesel::types::Timestamp on their struct directly. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[sgrif]

It looks like EarlyLintPass does run early enough to catch these cases.

[llogiq]

Feel free to open clippy issues with suggestions. I personally don't know enough about the inner workings of Diesel, so I don't know how to typecheck the schema, but I think at least linting fields of a fixed type within a struct is definitively easy.

[killercup]

Orthogonal to lints, but related to nicer errors: https://scribbles.pascalhertleif.de/hook-into-rustc-errors.html

[sgrif]

Additional case we should lint for: References on Insertable structs for types other than str and [T]. Error message is super unhelpful in that case

[sgrif]

I'm going to stick this on the milestone. I think this would be a great thing to have done for 1.0. Right now I don't think that'll be possible, but if we have some more people come in to help with documentation I should have some more bandwidth to work on this.

[llogiq]

Ok I get this right: We want to lint structs that have a #[derive(Insertable)] annotation and contain refs?

The problem here is that the error message is likely emitted (and compilation stopped) before lints run, so there's not much we can do.

[sgrif]

This isn't happening in time for 1.0.