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

[stdsimd] cargo fmt --all succeeds but subsequent cargo fmt --all -- --check fails #2787

Closed
gnzlbg opened this issue Jun 15, 2018 · 10 comments
Closed
Labels
bug Panic, non-idempotency, invalid code, etc. p-high

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 15, 2018

With this PR cargo fmt --all currently outputs nothing and returns 0 (EXIT_SUCCESS). A subsequent call to cargo fmt --all -- --check returns 101 and sometimes (1/5 times) prints the following diff. Other times it doesn't print anything, just fails with that error (e.g. like here: https://travis-ci.org/rust-lang-nursery/stdsimd/jobs/392688468#L472):

Diff in stdsimd/crates/stdsimd-verify/src/lib.rs at line 95:
 
 fn to_type(t: &syn::Type) -> proc_macro2::TokenStream {
     match *t {
-        syn::Type::Path(ref p) => match extract_path_ident(&p.path).to_string().as_ref() {
-            "__m128" => quote! { &M128 },
-            "__m128d" => quote! { &M128D },
-            "__m128i" => quote! { &M128I },
-            "__m256" => quote! { &M256 },
-            "__m256d" => quote! { &M256D },
-            "__m256i" => quote! { &M256I },
-            "__m64" => quote! { &M64 },
-            "bool" => quote! { &BOOL },
-            "f32" => quote! { &F32 },
-            "f64" => quote! { &F64 },
-            "i16" => quote! { &I16 },
-            "i32" => quote! { &I32 },
-            "i64" => quote! { &I64 },
-            "i8" => quote! { &I8 },
-            "u16" => quote! { &U16 },
-            "u32" => quote! { &U32 },
-            "u64" => quote! { &U64 },
-            "u8" => quote! { &U8 },
-            "CpuidResult" => quote! { &CPUID },
-            s => panic!("unspported type: {}", s),
-        },
+        syn::Type::Path(ref p) => {
+            match extract_path_ident(&p.path).to_string().as_ref() {
+                "__m128" => quote! { &M128 },
+                "__m128d" => quote! { &M128D },
+                "__m128i" => quote! { &M128I },
+                "__m256" => quote! { &M256 },
+                "__m256d" => quote! { &M256D },
+                "__m256i" => quote! { &M256I },
+                "__m64" => quote! { &M64 },
+                "bool" => quote! { &BOOL },
+                "f32" => quote! { &F32 },
+                "f64" => quote! { &F64 },
+                "i16" => quote! { &I16 },
+                "i32" => quote! { &I32 },
+                "i64" => quote! { &I64 },
+                "i8" => quote! { &I8 },
+                "u16" => quote! { &U16 },
+                "u32" => quote! { &U32 },
+                "u64" => quote! { &U64 },
+                "u8" => quote! { &U8 },
+                "CpuidResult" => quote! { &CPUID },
+                s => panic!("unspported type: {}", s),
+            }
+        }
         syn::Type::Ptr(syn::TypePtr { ref elem, .. })
         | syn::Type::Reference(syn::TypeReference { ref elem, .. }) => {
             let tokens = to_type(&elem);

We should add to CI a check that after formatting cargo fmt --all -- --check should return true and if not fail the integration tests.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 15, 2018

cc @nrc

@nrc nrc added the bug Panic, non-idempotency, invalid code, etc. label Jun 18, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 19, 2018

update: the integration tests for cargo fmt --all -- --check have been merged in 75d46b1

Once this is fixed those tests should prevent it from becoming broken again.

@nrc nrc added this to the 1.0 (rc) milestone Aug 4, 2018
@nrc nrc added the p-high label Aug 4, 2018
@topecongiro
Copy link
Contributor

topecongiro commented Aug 4, 2018

This is still reproducible with the latest rustfmt (rustfmt 0.99.1-nightly (da17b689 2018-08-04)).

Why do cargo fmt --all or cargo fmt --all -- --check return 101 in the first place? AFAICT in the latest rustfmt there is no code path that returns 101 exit code...

@topecongiro
Copy link
Contributor

I figured that rustfmt silently crashes when formatting stdsimd/crates/src/lib.rs. In particular, this module declaration seems to break rustfmt.

@topecongiro
Copy link
Contributor

topecongiro commented Aug 4, 2018

EDIT: Ignore this comment, this is wrong 😞


Ok, so the actual file that breaks rustfmt is stdsimd/stdsimd/mod.rs, and the relevant code is this:

/// ```ignore
/// #[cfg(all(any(target_arch = "x86", target_arch = "x86_64"),
///       target_feature = "avx2"))]
/// fn foo() {
///     #[cfg(target_arch = "x86")]
///     use std::arch::x86::_mm256_add_epi64;
///     #[cfg(target_arch = "x86_64")]
///     use std::arch::x86_64::_mm256_add_epi64;
///
///     unsafe {
///         _mm256_add_epi64(...);
///     }
/// }
/// ```

Note that the third line should be prefixed with #. I have sent a PR that fixes this to stdsimd (rust-lang/stdarch#548).

@topecongiro
Copy link
Contributor

I am still unsure why rustfmt started to fail silently against invalid code, I guess there is something weird going on inside syntax::with_globals?

@topecongiro
Copy link
Contributor

#2787 (comment) was wrong 😞 It was a pure bug in the rustfmt side. #2895 should fix this.

@topecongiro
Copy link
Contributor

Closed via #2895.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 10, 2018

Did this land on the nightly preview yet? Or could you ping me when it does?

@nrc
Copy link
Member

nrc commented Aug 13, 2018

It hasn't because we've had some problems in the Rust repo. It is in the queue, so as soon as there is a new nightly, it should have this fix in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc. p-high
Projects
None yet
Development

No branches or pull requests

3 participants