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

Update to nightly-2018-07-17 #1789

Merged
merged 2 commits into from
Jul 19, 2018
Merged

Update to nightly-2018-07-17 #1789

merged 2 commits into from
Jul 19, 2018

Conversation

sgrif
Copy link
Member

@sgrif sgrif commented Jul 18, 2018

I would have preferred to do this in smaller pieces, but unfortunately
we had to do this all in one go. Clippy is no longer publishing to
crates.io, so we have to switch to using cargo clippy. There are also
a few bugs in recent versions that we have to work around.

The version of proc-macro that we were using no longer compiles on the
most recent nightlies, so we had to bump that as well (and therefore
also bump our version of syn). The biggest change here is that
syn::Ident is now proc_macro2::Ident, which does not implement
From<&str>, AsRef<str>, or Copy.

Finally, our UI tests regressed significantly, as Rust is no longer
giving us any useable span information for any piece of attributes in
derive macros. There's nothing we can do to work around this, we'll just
have to wait for Rust to fix it on their end.

I would have preferred to do this in smaller pieces, but unfortunately
we had to do this all in one go. Clippy is no longer publishing to
crates.io, so we have to switch to using `cargo clippy`. There are also
a few bugs in recent versions that we have to work around.

The version of proc-macro that we were using no longer compiles on the
most recent nightlies, so we had to bump that as well (and therefore
also bump our version of `syn`). The biggest change here is that
`syn::Ident` is now `proc_macro2::Ident`, which does not implement
`From<&str>`, `AsRef<str>`, or `Copy`.

Finally, our UI tests regressed significantly, as Rust is no longer
giving us any useable span information for any piece of attributes in
derive macros. There's nothing we can do to work around this, we'll just
have to wait for Rust to fix it on their end.
@sgrif sgrif requested a review from a team July 18, 2018 19:59
Copy link
Member

@Eijebong Eijebong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the dummy mod question, this looks good to me.

@@ -108,7 +108,7 @@ pub struct StatementCache<DB: Backend, Statement> {
pub cache: RefCell<HashMap<StatementCacheKey<DB>, Statement>>,
}

#[cfg_attr(feature = "clippy", allow(len_without_is_empty, new_without_default_derive))]
#[cfg_attr(feature = "cargo-clippy", allow(len_without_is_empty, new_without_default_derive))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is set by clippy when you run it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@@ -1,3 +1,5 @@
#![cfg_attr(feature = "cargo-clippy", allow(expect_fun_call))] // My calls are so fun
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

@@ -78,8 +78,8 @@ fn sql_type(field: &Field, model: &Model) -> syn::Type {
parse_quote!(diesel::dsl::SqlTypeOf<#table_name::#column_name>)
} else {
let field_name = match field.name {
FieldName::Named(ref x) => x.as_ref(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be to_string instead of creating another Ident. That would make it clearer that it's not going to be used in a quote! call.

quote! {
#[allow(non_snake_case, unused_extern_crates, unused_imports)]
mod #const_name {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hu ? This isn't a dummy mod then

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure -- Just didn't really feel like churning every call. Probably should have named it dummy_scope or something since we've now changed it from const to mod to fn

let bad_span_debug = "#0 bytes(0..0)";

if format!("{:?}", fallback) == bad_span_debug {
// On recent rust nightlies, even our fallback span is bad.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/

@nabijaczleweli
Copy link

Does this also fix the docs not building on latest nightly?

@sgrif
Copy link
Member Author

sgrif commented Jul 18, 2018

I have no idea. We build our docs on stable.

@nabijaczleweli
Copy link

Well, FTR, this is what the error looks like (can you cfg() on is-doc?, seems like simplest solution).

@sgrif
Copy link
Member Author

sgrif commented Jul 18, 2018

You're using 0.16.0, which hasn't been supported for nearly a year

@nabijaczleweli
Copy link

nabijaczleweli commented Jul 18, 2018

Whoops! I based this project off an another, older one, and there seemed to not be any kind of (migration, deprecation?) notice on diesel_codegen, so I matched diesel version to diesel_codegen's. Will explore that.

However, the error still occurs on latest diesel (CI link):

note: lint level defined here
   --> D:\Users\nabijaczleweli\.cargo\registry\src\github.com-1ecc6299db9ec823\diesel-1.3.2\src\lib.rs:131:9
    |
131 | #![deny(warnings, missing_debug_implementations, missing_copy_implementations, missing_docs)]
    |         ^^^^^^^^
    = note: #[deny(intra_doc_link_resolution_failure)] implied by #[deny(warnings)]
    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`

This was referenced Jul 19, 2018
@@ -108,7 +108,7 @@ pub struct StatementCache<DB: Backend, Statement> {
pub cache: RefCell<HashMap<StatementCacheKey<DB>, Statement>>,
}

#[cfg_attr(feature = "clippy", allow(len_without_is_empty, new_without_default_derive))]
#[cfg_attr(feature = "cargo-clippy", allow(len_without_is_empty, new_without_default_derive))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@nabijaczleweli
Copy link

nabijaczleweli commented Jul 19, 2018

Okay, so I've the simple solution for the aforementioned problem; that is to change srd/lib.rs#L131 to:

#![cfg_attr(feature = "dox", deny(warnings, missing_debug_implementations, missing_copy_implementations, missing_docs))]

This will only disallow warnings on non-doc builds, meaning that docs, while still warninging like hell (I'll probably PR that), do actually build.

@sgrif sgrif merged commit 765b782 into master Jul 19, 2018
@sgrif sgrif deleted the sg-bump-nightly branch July 19, 2018 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants