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

Add check for [T;N]/usize mismatch in astconv #80538

Merged
merged 1 commit into from Jan 5, 2021

Conversation

JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Dec 30, 2020

Helps clarify the issue in #80506
by adding a specific check for mismatches between [T;N] and usize.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2020
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_typeck/src/astconv/generics.rs at line 7:
 use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticId, ErrorReported};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
-use rustc_hir::{GenericArg};
+use rustc_hir::GenericArg;
 use rustc_middle::ty::{
     self, subst, subst::SubstsRef, GenericParamDef, GenericParamDefKind, Ty, TyCtxt,
 };
Diff in /checkout/compiler/rustc_typeck/src/astconv/generics.rs at line 61:
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_typeck/src/astconv/generics.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
 
         // Specific suggestion set for diagnostics
         if let Some(param) = param {
-          match (arg, &param.kind) {
-              (
-                  GenericArg::Type(hir::Ty { kind: hir::TyKind::Path { .. }, .. }),
-                  GenericParamDefKind::Const { .. },
-              ) => {
-                  let suggestions = vec![
-                      (arg.span().shrink_to_lo(), String::from("{ ")),
-                      (arg.span().shrink_to_hi(), String::from(" }")),
-                  ];
-                  err.multipart_suggestion(
-                      "if this generic argument was intended as a const parameter, \
+            match (arg, &param.kind) {
+                (
+                    GenericArg::Type(hir::Ty { kind: hir::TyKind::Path { .. }, .. }),
+                    GenericParamDefKind::Const { .. },
+                ) => {
+                    let suggestions = vec![
+                        (arg.span().shrink_to_lo(), String::from("{ ")),
+                        (arg.span().shrink_to_hi(), String::from(" }")),
+                    ];
+                    err.multipart_suggestion(
+                        "if this generic argument was intended as a const parameter, \
                     try surrounding it with braces:",
-                      suggestions,
-                      Applicability::MaybeIncorrect,
-                  );
-              (
-              (
-                  GenericArg::Type(hir::Ty { kind: hir::TyKind::Array(_, len), .. }),
-                  GenericParamDefKind::Const { .. },
-              ) => if tcx.type_of(param.def_id).is_ptr_sized_integral() {
-                  let snippet = sess
-                      .source_map()
-                      .span_to_snippet(tcx.hir().span(len.hir_id))
-                      .unwrap_or(String::from("N"));
-                  err.span_suggestion(
-                      arg.span(),
-                      "`[T; N]` provided where `usize` was expected, try",
-                      format!("{}", snippet),
-                      Applicability::MaybeIncorrect,
-                  );
-              _ => {}
-          }
+                        suggestions,
+                        suggestions,
+                        Applicability::MaybeIncorrect,
+                    );
+                (
+                (
+                    GenericArg::Type(hir::Ty { kind: hir::TyKind::Array(_, len), .. }),
+                    GenericParamDefKind::Const { .. },
+                ) => {
+                    if tcx.type_of(param.def_id).is_ptr_sized_integral() {
+                        let snippet = sess
+                            .source_map()
+                            .span_to_snippet(tcx.hir().span(len.hir_id))
+                            .unwrap_or(String::from("N"));
+                        err.span_suggestion(
+                            arg.span(),
+                            "`[T; N]` provided where `usize` was expected, try",
+                            format!("{}", snippet),
+                            Applicability::MaybeIncorrect,
+                        );
+                }
+                _ => {}
+            }
         }
         }
 
         // This note is only true when generic parameters are strictly ordered by their kind.
Build completed unsuccessfully in 0:00:14

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

thanks ❤️

can you also add a test for [u8; 1 + 2]? We probably want to always recommend using braces for now as especially for complex expressions the error recovery when they are missing is fairly hard.

compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/astconv/generics.rs Outdated Show resolved Hide resolved
@JulianKnodt JulianKnodt force-pushed the err_usize branch 2 times, most recently from 4885728 to 029ff3f Compare January 4, 2021 01:14
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking rustc_attr v0.0.0 (/checkout/compiler/rustc_attr)
    Checking rustc_hir v0.0.0 (/checkout/compiler/rustc_hir)
    Checking rustc_parse v0.0.0 (/checkout/compiler/rustc_parse)
    Checking chalk-engine v0.36.0
error[E0599]: no method named `features` found for reference `&Session` in the current scope
   --> compiler/rustc_hir/src/hir.rs:299:60
    |
299 |                 ast::ParamKindOrd::Const { unordered: sess.features().const_generics }
    |                                                            ^^^^^^^^ private field, not a method
error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `rustc_hir`
error: could not compile `rustc_hir`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" " llvm max_level_info" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "-p" "rustc-main" "-p" "rustc_codegen_ssa" "-p" "rustc_session" "-p" "rustc_lint_defs" "-p" "rustc_feature" "-p" "rustc_data_structures" "-p" "rustc_graphviz" "-p" "rustc_target" "-p" "rustc_index" "-p" "rustc_span" "-p" "rustc_arena" "-p" "rustc_macros" "-p" "rustc_symbol_mangling" "-p" "rustc_middle" "-p" "rustc_query_system" "-p" "rustc_type_ir" "-p" "rustc_incremental" "-p" "rustc_apfloat" "-p" "rustc_hir" "-p" "rustc_ast" "-p" "rustc_lexer" "-p" "rustc_errors" "-p" "rustc_attr" "-p" "rustc_ast_pretty" "-p" "rustc_fs_util" "-p" "rustc_serialize" "-p" "rustc_driver" "-p" "rustc_parse" "-p" "rustc_error_codes" "-p" "rustc_lint" "-p" "rustc_trait_selection" "-p" "rustc_infer" "-p" "rustc_parse_format" "-p" "rustc_mir" "-p" "coverage_test_macros" "-p" "rustc_hir_pretty" "-p" "rustc_metadata" "-p" "rustc_expand" "-p" "rustc_ast_passes" "-p" "rustc_interface" "-p" "rustc_builtin_macros" "-p" "rustc_mir_build" "-p" "rustc_resolve" "-p" "rustc_codegen_llvm" "-p" "rustc_llvm" "-p" "rustc_ty_utils" "-p" "rustc_traits" "-p" "rustc_passes" "-p" "rustc_ast_lowering" "-p" "rustc_privacy" "-p" "rustc_typeck" "-p" "rustc_save_analysis" "-p" "rustc_plugin_impl" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:50

@JulianKnodt JulianKnodt force-pushed the err_usize branch 2 times, most recently from ad6ad28 to 7091cf7 Compare January 4, 2021 10:05
@lcnr
Copy link
Contributor

lcnr commented Jan 4, 2021

thanks 👍

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 4, 2021

📌 Commit 54883e0 has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2021
Rollup of 12 pull requests

Successful merges:

 - rust-lang#80442 (Mention Arc::make_mut and Rc::make_mut in the documentation of Cow)
 - rust-lang#80533 (bootstrap: clippy fixes)
 - rust-lang#80538 (Add check for `[T;N]`/`usize` mismatch in astconv)
 - rust-lang#80612 (Remove reverted change from relnotes)
 - rust-lang#80627 (Builder: Warn if test file does not exist)
 - rust-lang#80637 (Use Option::filter instead of open-coding it)
 - rust-lang#80643 (Move variable into the only branch where it is relevant)
 - rust-lang#80656 (Fixed documentation error for `std::hint::spin_loop`)
 - rust-lang#80666 (Fix missing link for "fully qualified syntax")
 - rust-lang#80672 (./x.py clippy: allow the most noisy lints)
 - rust-lang#80677 (doc -- list edit for consistency)
 - rust-lang#80696 (make sure that promoteds which fail to evaluate in dead const code behave correctly)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit be2a3f8 into rust-lang:master Jan 5, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 5, 2021
@JulianKnodt JulianKnodt deleted the err_usize branch January 5, 2021 06:05
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 7, 2021
@rylev
Copy link
Member

rylev commented Jan 8, 2021

@JulianKnodt @lcnr it seems this change was responsible for a moderate performance regression (and a minor perf gain). You can see the results in #80798 where we reverted this PR and ran the perf suite.

The regression was in the deeply-nested-async benchmark which does not contain any const code it seems. Any ideas?

@@ -203,7 +213,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
// We expected a lifetime argument, but got a type or const
// argument. That means we're inferring the lifetimes.
substs.push(ctx.inferred_kind(None, param, infer_args));
force_infer_lt = Some(arg);
force_infer_lt = Some((arg, param));
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that this loop is a lot hotter than I expected so storing arg with param here is quite costly?

Other than that i really don't know waht could be causing the perf regression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this would be the most likely cause, it could be that the type for param for deeply-nested-async stuff is huge and thus takes up a lot of space on the stack, so even if it's not hot it might just be causing a lot more cache misses

edit: these are refs, so the size shouldn't matter, ignore above

Would it be good to change this to an index instead? I don't remember where the param was coming from at the moment

Copy link
Member

Choose a reason for hiding this comment

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

It seems possible that this increases the size of force_infer_lt enough that it becomes stored on the stack instead of a register, I guess, but it's pretty odd that it would cause any problems (still just two pointers vs one). An index would still be a usize so should be equally costly I'd expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a print to the inner body, and during compilation it appears that the body is hit at least 1.5M times, so I guess that's fairly hot?

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean during the compilation of the compiler itself? so during ./x.py build --stage 2 or just while building std?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was running ./x.py test src/test/ui, but it got to compiling stage 1 artifacts and I quit out

@lcnr lcnr added the A-const-generics Area: const generics (parameters and arguments) label Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants