From 7e7d0074671a4b1c4940defbabb30c67c4369fda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Sun, 3 Jul 2022 00:00:00 +0000 Subject: [PATCH 01/13] Add `SourceScope::inlined_instance` --- .../rustc_codegen_ssa/src/mir/coverageinfo.rs | 7 ++----- compiler/rustc_middle/src/mir/mod.rs | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs index a283bf1de763a..f1fe495282abc 100644 --- a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs @@ -9,11 +9,8 @@ use super::FunctionCx; impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { pub fn codegen_coverage(&self, bx: &mut Bx, coverage: Coverage, scope: SourceScope) { // Determine the instance that coverage data was originally generated for. - let scope_data = &self.mir.source_scopes[scope]; - let instance = if let Some((inlined_instance, _)) = scope_data.inlined { - self.monomorphize(inlined_instance) - } else if let Some(inlined_scope) = scope_data.inlined_parent_scope { - self.monomorphize(self.mir.source_scopes[inlined_scope].inlined.unwrap().0) + let instance = if let Some(inlined) = scope.inlined_instance(&self.mir.source_scopes) { + self.monomorphize(inlined) } else { self.instance }; diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 9368612810198..743e02ad3e19e 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1650,6 +1650,22 @@ impl SourceScope { ClearCrossCrate::Clear => None, } } + + /// The instance this source scope was inlined from, if any. + #[inline] + pub fn inlined_instance<'tcx>( + self, + source_scopes: &IndexVec>, + ) -> Option> { + let scope_data = &source_scopes[self]; + if let Some((inlined_instance, _)) = scope_data.inlined { + Some(inlined_instance) + } else if let Some(inlined_scope) = scope_data.inlined_parent_scope { + Some(source_scopes[inlined_scope].inlined.unwrap().0) + } else { + None + } + } } #[derive(Clone, Debug, TyEncodable, TyDecodable, HashStable, TypeFoldable, TypeVisitable)] From fadae872fabd317020eeefbb118d8e07e5e43994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Sun, 3 Jul 2022 00:00:00 +0000 Subject: [PATCH 02/13] Use extend instead of repeatedly pushing into a vec --- compiler/rustc_mir_transform/src/simplify.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir_transform/src/simplify.rs b/compiler/rustc_mir_transform/src/simplify.rs index 980af98436281..ec0eebe5611e8 100644 --- a/compiler/rustc_mir_transform/src/simplify.rs +++ b/compiler/rustc_mir_transform/src/simplify.rs @@ -352,15 +352,15 @@ fn save_unreachable_coverage( } let start_block = &mut basic_blocks[START_BLOCK]; - for (source_info, code_region) in dropped_coverage { - start_block.statements.push(Statement { + start_block.statements.extend(dropped_coverage.into_iter().map( + |(source_info, code_region)| Statement { source_info, kind: StatementKind::Coverage(Box::new(Coverage { kind: CoverageKind::Unreachable, code_region: Some(code_region), })), - }) - } + }, + )); } pub struct SimplifyLocals; From 62ab4b61609e6f1329c3f386e545412e8c7e58d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Sun, 3 Jul 2022 00:00:00 +0000 Subject: [PATCH 03/13] Fix unreachable coverage generation for inlined functions To generate a function coverage we need at least one coverage counter, so a coverage from unreachable blocks is retained only when some live counters remain. The previous implementation incorrectly retained unreachable coverage, because it didn't account for the fact that those live counters can belong to another function due to inlining. --- compiler/rustc_mir_transform/src/simplify.rs | 72 ++++++++++--------- .../src/partitioning/mod.rs | 12 +++- .../expected_show_coverage.inline-dead.txt | 21 ++++++ .../run-make-fulldeps/coverage/inline-dead.rs | 20 ++++++ 4 files changed, 90 insertions(+), 35 deletions(-) create mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline-dead.txt create mode 100644 src/test/run-make-fulldeps/coverage/inline-dead.rs diff --git a/compiler/rustc_mir_transform/src/simplify.rs b/compiler/rustc_mir_transform/src/simplify.rs index ec0eebe5611e8..59c38b87b5cb2 100644 --- a/compiler/rustc_mir_transform/src/simplify.rs +++ b/compiler/rustc_mir_transform/src/simplify.rs @@ -28,6 +28,7 @@ //! return. use crate::MirPass; +use rustc_data_structures::stable_set::FxHashSet; use rustc_index::vec::{Idx, IndexVec}; use rustc_middle::mir::coverage::*; use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor}; @@ -267,7 +268,8 @@ pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { return; } - let basic_blocks = body.basic_blocks_mut(); + let basic_blocks = body.basic_blocks.as_mut(); + let source_scopes = &body.source_scopes; let mut replacements: Vec<_> = (0..num_blocks).map(BasicBlock::new).collect(); let mut used_blocks = 0; for alive_index in reachable.iter() { @@ -282,7 +284,7 @@ pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { } if tcx.sess.instrument_coverage() { - save_unreachable_coverage(basic_blocks, used_blocks); + save_unreachable_coverage(basic_blocks, source_scopes, used_blocks); } basic_blocks.raw.truncate(used_blocks); @@ -311,48 +313,54 @@ pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { /// `Unreachable` coverage statements. These are non-executable statements whose /// code regions are still recorded in the coverage map, representing regions /// with `0` executions. +/// +/// If there are no live `Counter` `Coverage` statements remaining, we remove +/// dead `Coverage` statements along with the dead blocks. Since at least one +/// counter per function is required by LLVM (and necessary, to add the +/// `function_hash` to the counter's call to the LLVM intrinsic +/// `instrprof.increment()`). +/// +/// The `generator::StateTransform` MIR pass and MIR inlining can create +/// atypical conditions, where all live `Counter`s are dropped from the MIR. +/// +/// With MIR inlining we can have coverage counters belonging to different +/// instances in a single body, so the strategy described above is applied to +/// coverage counters from each instance individually. fn save_unreachable_coverage( basic_blocks: &mut IndexVec>, + source_scopes: &IndexVec>, first_dead_block: usize, ) { - let has_live_counters = basic_blocks.raw[0..first_dead_block].iter().any(|live_block| { - live_block.statements.iter().any(|statement| { - if let StatementKind::Coverage(coverage) = &statement.kind { - matches!(coverage.kind, CoverageKind::Counter { .. }) - } else { - false - } - }) - }); - if !has_live_counters { - // If there are no live `Counter` `Coverage` statements anymore, don't - // move dead coverage to the `START_BLOCK`. Just allow the dead - // `Coverage` statements to be dropped with the dead blocks. - // - // The `generator::StateTransform` MIR pass can create atypical - // conditions, where all live `Counter`s are dropped from the MIR. - // - // At least one Counter per function is required by LLVM (and necessary, - // to add the `function_hash` to the counter's call to the LLVM - // intrinsic `instrprof.increment()`). + // Identify instances that still have some live coverage counters left. + let mut live = FxHashSet::default(); + for basic_block in &basic_blocks.raw[0..first_dead_block] { + for statement in &basic_block.statements { + let StatementKind::Coverage(coverage) = &statement.kind else { continue }; + let CoverageKind::Counter { .. } = coverage.kind else { continue }; + let instance = statement.source_info.scope.inlined_instance(source_scopes); + live.insert(instance); + } + } + + if live.is_empty() { return; } - // Retain coverage info for dead blocks, so coverage reports will still - // report `0` executions for the uncovered code regions. - let mut dropped_coverage = Vec::new(); - for dead_block in basic_blocks.raw[first_dead_block..].iter() { - for statement in dead_block.statements.iter() { - if let StatementKind::Coverage(coverage) = &statement.kind { - if let Some(code_region) = &coverage.code_region { - dropped_coverage.push((statement.source_info, code_region.clone())); - } + // Retain coverage for instances that still have some live counters left. + let mut retained_coverage = Vec::new(); + for dead_block in &basic_blocks.raw[first_dead_block..] { + for statement in &dead_block.statements { + let StatementKind::Coverage(coverage) = &statement.kind else { continue }; + let Some(code_region) = &coverage.code_region else { continue }; + let instance = statement.source_info.scope.inlined_instance(source_scopes); + if live.contains(&instance) { + retained_coverage.push((statement.source_info, code_region.clone())); } } } let start_block = &mut basic_blocks[START_BLOCK]; - start_block.statements.extend(dropped_coverage.into_iter().map( + start_block.statements.extend(retained_coverage.into_iter().map( |(source_info, code_region)| Statement { source_info, kind: StatementKind::Coverage(Box::new(Coverage { diff --git a/compiler/rustc_monomorphize/src/partitioning/mod.rs b/compiler/rustc_monomorphize/src/partitioning/mod.rs index c1992137575bc..a1061dbf67fbd 100644 --- a/compiler/rustc_monomorphize/src/partitioning/mod.rs +++ b/compiler/rustc_monomorphize/src/partitioning/mod.rs @@ -98,6 +98,7 @@ mod merging; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::sync; use rustc_hir::def_id::DefIdSet; +use rustc_middle::mir; use rustc_middle::mir::mono::MonoItem; use rustc_middle::mir::mono::{CodegenUnit, Linkage}; use rustc_middle::ty::print::with_no_trimmed_paths; @@ -479,9 +480,14 @@ fn codegened_and_inlined_items<'tcx>(tcx: TyCtxt<'tcx>, (): ()) -> &'tcx DefIdSe if !visited.insert(did) { continue; } - for scope in &tcx.instance_mir(instance.def).source_scopes { - if let Some((ref inlined, _)) = scope.inlined { - result.insert(inlined.def_id()); + let body = tcx.instance_mir(instance.def); + for block in body.basic_blocks() { + for statement in &block.statements { + let mir::StatementKind::Coverage(_) = statement.kind else { continue }; + let scope = statement.source_info.scope; + if let Some(inlined) = scope.inlined_instance(&body.source_scopes) { + result.insert(inlined.def_id()); + } } } } diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline-dead.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline-dead.txt new file mode 100644 index 0000000000000..d102d9ecf7d14 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inline-dead.txt @@ -0,0 +1,21 @@ + 1| |// Regression test for issue #98833. + 2| |// compile-flags: -Zinline-mir + 3| | + 4| 1|fn main() { + 5| 1| println!("{}", live::()); + 6| 1|} + 7| | + 8| |#[inline] + 9| 1|fn live() -> u32 { + 10| 1| if B { + 11| 0| dead() + 12| | } else { + 13| 1| 0 + 14| | } + 15| 1|} + 16| | + 17| |#[inline] + 18| 0|fn dead() -> u32 { + 19| 0| 42 + 20| 0|} + diff --git a/src/test/run-make-fulldeps/coverage/inline-dead.rs b/src/test/run-make-fulldeps/coverage/inline-dead.rs new file mode 100644 index 0000000000000..cd1ae911a5f7e --- /dev/null +++ b/src/test/run-make-fulldeps/coverage/inline-dead.rs @@ -0,0 +1,20 @@ +// Regression test for issue #98833. +// compile-flags: -Zinline-mir + +fn main() { + println!("{}", live::()); +} + +#[inline] +fn live() -> u32 { + if B { + dead() + } else { + 0 + } +} + +#[inline] +fn dead() -> u32 { + 42 +} From c8165c5775966ba639e1a062bb807917e3fa3631 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 15 Jul 2022 05:37:32 +0000 Subject: [PATCH 04/13] Do not resolve associated const when there is no provided value --- .../src/traits/const_evaluatable.rs | 15 ++++++++++++--- compiler/rustc_ty_utils/src/instance.rs | 5 +++++ .../ui/const-generics/issues/issue-86530.rs | 1 - .../const-generics/issues/issue-86530.stderr | 18 +----------------- .../ui/const-generics/issues/issue-98629.rs | 15 +++++++++++++++ .../const-generics/issues/issue-98629.stderr | 12 ++++++++++++ src/test/ui/issues/issue-77919.rs | 1 - src/test/ui/issues/issue-77919.stderr | 16 ++++------------ 8 files changed, 49 insertions(+), 34 deletions(-) create mode 100644 src/test/ui/const-generics/issues/issue-98629.rs create mode 100644 src/test/ui/const-generics/issues/issue-98629.stderr diff --git a/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs b/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs index e6284b1c4ace0..decbf0133114f 100644 --- a/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs +++ b/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs @@ -185,14 +185,20 @@ pub fn is_const_evaluatable<'cx, 'tcx>( } let concrete = infcx.const_eval_resolve(param_env, uv.expand(), Some(span)); match concrete { - Err(ErrorHandled::TooGeneric) => Err(if !uv.has_infer_types_or_consts() { + Err(ErrorHandled::TooGeneric) => Err(if uv.has_infer_types_or_consts() { + NotConstEvaluatable::MentionsInfer + } else if uv.has_param_types_or_consts() { infcx .tcx .sess .delay_span_bug(span, &format!("unexpected `TooGeneric` for {:?}", uv)); NotConstEvaluatable::MentionsParam } else { - NotConstEvaluatable::MentionsInfer + let guar = infcx.tcx.sess.delay_span_bug( + span, + format!("Missing value for constant, but no error reported?"), + ); + NotConstEvaluatable::Error(guar) }), Err(ErrorHandled::Linted) => { let reported = infcx @@ -240,8 +246,11 @@ pub fn is_const_evaluatable<'cx, 'tcx>( Err(ErrorHandled::TooGeneric) => Err(if uv.has_infer_types_or_consts() { NotConstEvaluatable::MentionsInfer - } else { + } else if uv.has_param_types_or_consts() { NotConstEvaluatable::MentionsParam + } else { + let guar = infcx.tcx.sess.delay_span_bug(span, format!("Missing value for constant, but no error reported?")); + NotConstEvaluatable::Error(guar) }), Err(ErrorHandled::Linted) => { let reported = diff --git a/compiler/rustc_ty_utils/src/instance.rs b/compiler/rustc_ty_utils/src/instance.rs index 5e58f2379827e..979e997f24491 100644 --- a/compiler/rustc_ty_utils/src/instance.rs +++ b/compiler/rustc_ty_utils/src/instance.rs @@ -280,6 +280,11 @@ fn resolve_associated_item<'tcx>( return Ok(None); } + // If the item does not have a value, then we cannot return an instance. + if !leaf_def.item.defaultness.has_value() { + return Ok(None); + } + let substs = tcx.erase_regions(substs); // Check if we just resolved an associated `const` declaration from diff --git a/src/test/ui/const-generics/issues/issue-86530.rs b/src/test/ui/const-generics/issues/issue-86530.rs index 4a6ffd1f3008e..b024decd4e11c 100644 --- a/src/test/ui/const-generics/issues/issue-86530.rs +++ b/src/test/ui/const-generics/issues/issue-86530.rs @@ -15,7 +15,6 @@ where fn unit_literals() { z(" "); //~^ ERROR: the trait bound `&str: X` is not satisfied - //~| ERROR: unconstrained generic constant } fn main() {} diff --git a/src/test/ui/const-generics/issues/issue-86530.stderr b/src/test/ui/const-generics/issues/issue-86530.stderr index c688f838dab47..c63857b2314e9 100644 --- a/src/test/ui/const-generics/issues/issue-86530.stderr +++ b/src/test/ui/const-generics/issues/issue-86530.stderr @@ -15,22 +15,6 @@ LL | where LL | T: X, | ^ required by this bound in `z` -error: unconstrained generic constant - --> $DIR/issue-86530.rs:16:5 - | -LL | z(" "); - | ^ - | - = help: try adding a `where` bound using this expression: `where [(); T::Y]:` -note: required by a bound in `z` - --> $DIR/issue-86530.rs:11:10 - | -LL | fn z(t: T) - | - required by a bound in this -... -LL | [(); T::Y]: , - | ^^^^ required by this bound in `z` - -error: aborting due to 2 previous errors +error: aborting due to previous error For more information about this error, try `rustc --explain E0277`. diff --git a/src/test/ui/const-generics/issues/issue-98629.rs b/src/test/ui/const-generics/issues/issue-98629.rs new file mode 100644 index 0000000000000..fc8666bbcdb79 --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-98629.rs @@ -0,0 +1,15 @@ +#![feature(const_trait_impl)] + +trait Trait { + const N: usize; +} + +impl const Trait for i32 {} +//~^ ERROR not all trait items implemented, missing: `N` + +fn f() +where + [(); ::N]:, +{} + +fn main() {} diff --git a/src/test/ui/const-generics/issues/issue-98629.stderr b/src/test/ui/const-generics/issues/issue-98629.stderr new file mode 100644 index 0000000000000..53570220882c3 --- /dev/null +++ b/src/test/ui/const-generics/issues/issue-98629.stderr @@ -0,0 +1,12 @@ +error[E0046]: not all trait items implemented, missing: `N` + --> $DIR/issue-98629.rs:7:1 + | +LL | const N: usize; + | -------------- `N` from trait +... +LL | impl const Trait for i32 {} + | ^^^^^^^^^^^^^^^^^^^^^^^^ missing `N` in implementation + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0046`. diff --git a/src/test/ui/issues/issue-77919.rs b/src/test/ui/issues/issue-77919.rs index 1d5d593073117..966d76d148af3 100644 --- a/src/test/ui/issues/issue-77919.rs +++ b/src/test/ui/issues/issue-77919.rs @@ -1,6 +1,5 @@ fn main() { [1; >::VAL]; - //~^ ERROR: constant expression depends on a generic parameter } trait TypeVal { const VAL: T; diff --git a/src/test/ui/issues/issue-77919.stderr b/src/test/ui/issues/issue-77919.stderr index b4c877a2d74a4..ca256847b1f3b 100644 --- a/src/test/ui/issues/issue-77919.stderr +++ b/src/test/ui/issues/issue-77919.stderr @@ -1,5 +1,5 @@ error[E0412]: cannot find type `PhantomData` in this scope - --> $DIR/issue-77919.rs:10:9 + --> $DIR/issue-77919.rs:9:9 | LL | _n: PhantomData, | ^^^^^^^^^^^ not found in this scope @@ -10,7 +10,7 @@ LL | use std::marker::PhantomData; | error[E0412]: cannot find type `VAL` in this scope - --> $DIR/issue-77919.rs:12:63 + --> $DIR/issue-77919.rs:11:63 | LL | impl TypeVal for Multiply where N: TypeVal {} | - ^^^ not found in this scope @@ -18,7 +18,7 @@ LL | impl TypeVal for Multiply where N: TypeVal {} | help: you might be missing a type parameter: `, VAL` error[E0046]: not all trait items implemented, missing: `VAL` - --> $DIR/issue-77919.rs:12:1 + --> $DIR/issue-77919.rs:11:1 | LL | const VAL: T; | ------------ `VAL` from trait @@ -26,15 +26,7 @@ LL | const VAL: T; LL | impl TypeVal for Multiply where N: TypeVal {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `VAL` in implementation -error: constant expression depends on a generic parameter - --> $DIR/issue-77919.rs:2:9 - | -LL | [1; >::VAL]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: this may fail depending on what value the parameter takes - -error: aborting due to 4 previous errors +error: aborting due to 3 previous errors Some errors have detailed explanations: E0046, E0412. For more information about an error, try `rustc --explain E0046`. From 0e146148c44abc9afa20331b837358b73b8ed415 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 18 Jul 2022 17:45:54 +0200 Subject: [PATCH 05/13] Group CSS font rule --- src/librustdoc/html/static/css/rustdoc.css | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/librustdoc/html/static/css/rustdoc.css b/src/librustdoc/html/static/css/rustdoc.css index c6933a8254bc2..f0bbf2e664d66 100644 --- a/src/librustdoc/html/static/css/rustdoc.css +++ b/src/librustdoc/html/static/css/rustdoc.css @@ -216,6 +216,15 @@ details.rustdoc-toggle > summary::before, div.impl-items > div:not(.docblock):not(.item-info), .content ul.crate a.crate, a.srclink, +#main-content > .since, +#help-button > button, +details.rustdoc-toggle.top-doc > summary, +details.rustdoc-toggle.top-doc > summary::before, +details.rustdoc-toggle.non-exhaustive > summary, +details.rustdoc-toggle.non-exhaustive > summary::before, +.scraped-example-title, +.more-examples-toggle summary, .more-examples-toggle .hide-more, +.example-links a, /* This selector is for the items listed in the "all items" page. */ #main-content > ul.docblock > li > a { font-family: "Fira Sans", Arial, NanumBarunGothic, sans-serif; @@ -702,7 +711,6 @@ pre, .rustdoc.source .example-wrap { } #main-content > .since { top: inherit; - font-family: "Fira Sans", Arial, sans-serif; } .content table:not(.table-display) { @@ -1521,7 +1529,6 @@ input:checked + .slider { } #help-button > button { - font-family: "Fira Sans", Arial, sans-serif; text-align: center; /* Rare exception to specifying font sizes in rem. Since this is acting as an icon, it's okay to specify their sizes in pixels. */ @@ -1693,7 +1700,6 @@ details.rustdoc-toggle.top-doc > summary, details.rustdoc-toggle.top-doc > summary::before, details.rustdoc-toggle.non-exhaustive > summary, details.rustdoc-toggle.non-exhaustive > summary::before { - font-family: 'Fira Sans'; font-size: 1rem; } @@ -2179,10 +2185,6 @@ in storage.js plus the media query with (min-width: 701px) border-radius: 50px; } -.scraped-example-title { - font-family: 'Fira Sans'; -} - .scraped-example .code-wrapper { position: relative; display: flex; @@ -2286,10 +2288,6 @@ in storage.js plus the media query with (min-width: 701px) cursor: pointer; } -.more-examples-toggle summary, .more-examples-toggle .hide-more { - font-family: 'Fira Sans'; -} - .more-scraped-examples { margin-left: 5px; display: flex; @@ -2324,7 +2322,6 @@ in storage.js plus the media query with (min-width: 701px) .example-links a { margin-top: 20px; - font-family: 'Fira Sans'; } .example-links ul { From 3d9dd681f520d1d59f38aed0056cf9474894cc74 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 20 Jul 2022 23:42:24 +0000 Subject: [PATCH 06/13] Resolve vars in same_type_modulo_infer --- .../src/infer/error_reporting/mod.rs | 146 ++++++++++-------- .../src/traits/error_reporting/mod.rs | 3 +- 2 files changed, 79 insertions(+), 70 deletions(-) diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs index d7505717bf3d2..6c57e7f4347f2 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/mod.rs @@ -316,37 +316,6 @@ pub fn unexpected_hidden_region_diagnostic<'tcx>( err } -/// Structurally compares two types, modulo any inference variables. -/// -/// Returns `true` if two types are equal, or if one type is an inference variable compatible -/// with the other type. A TyVar inference type is compatible with any type, and an IntVar or -/// FloatVar inference type are compatible with themselves or their concrete types (Int and -/// Float types, respectively). When comparing two ADTs, these rules apply recursively. -pub fn same_type_modulo_infer<'tcx>(a: Ty<'tcx>, b: Ty<'tcx>) -> bool { - match (&a.kind(), &b.kind()) { - (&ty::Adt(did_a, substs_a), &ty::Adt(did_b, substs_b)) => { - if did_a != did_b { - return false; - } - - substs_a.types().zip(substs_b.types()).all(|(a, b)| same_type_modulo_infer(a, b)) - } - (&ty::Int(_), &ty::Infer(ty::InferTy::IntVar(_))) - | (&ty::Infer(ty::InferTy::IntVar(_)), &ty::Int(_) | &ty::Infer(ty::InferTy::IntVar(_))) - | (&ty::Float(_), &ty::Infer(ty::InferTy::FloatVar(_))) - | ( - &ty::Infer(ty::InferTy::FloatVar(_)), - &ty::Float(_) | &ty::Infer(ty::InferTy::FloatVar(_)), - ) - | (&ty::Infer(ty::InferTy::TyVar(_)), _) - | (_, &ty::Infer(ty::InferTy::TyVar(_))) => true, - (&ty::Ref(_, ty_a, mut_a), &ty::Ref(_, ty_b, mut_b)) => { - mut_a == mut_b && same_type_modulo_infer(*ty_a, *ty_b) - } - _ => a == b, - } -} - impl<'a, 'tcx> InferCtxt<'a, 'tcx> { pub fn report_region_errors( &self, @@ -1723,15 +1692,14 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { }; debug!("exp_found {:?} terr {:?} cause.code {:?}", exp_found, terr, cause.code()); if let Some(exp_found) = exp_found { - let should_suggest_fixes = if let ObligationCauseCode::Pattern { root_ty, .. } = - cause.code() - { - // Skip if the root_ty of the pattern is not the same as the expected_ty. - // If these types aren't equal then we've probably peeled off a layer of arrays. - same_type_modulo_infer(self.resolve_vars_if_possible(*root_ty), exp_found.expected) - } else { - true - }; + let should_suggest_fixes = + if let ObligationCauseCode::Pattern { root_ty, .. } = cause.code() { + // Skip if the root_ty of the pattern is not the same as the expected_ty. + // If these types aren't equal then we've probably peeled off a layer of arrays. + self.same_type_modulo_infer(*root_ty, exp_found.expected) + } else { + true + }; if should_suggest_fixes { self.suggest_tuple_pattern(cause, &exp_found, diag); @@ -1786,7 +1754,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { .filter_map(|variant| { let sole_field = &variant.fields[0]; let sole_field_ty = sole_field.ty(self.tcx, substs); - if same_type_modulo_infer(sole_field_ty, exp_found.found) { + if self.same_type_modulo_infer(sole_field_ty, exp_found.found) { let variant_path = with_no_trimmed_paths!(self.tcx.def_path_str(variant.def_id)); // FIXME #56861: DRYer prelude filtering @@ -1902,39 +1870,41 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { self.get_impl_future_output_ty(exp_found.expected).map(Binder::skip_binder), self.get_impl_future_output_ty(exp_found.found).map(Binder::skip_binder), ) { - (Some(exp), Some(found)) if same_type_modulo_infer(exp, found) => match cause.code() { - ObligationCauseCode::IfExpression(box IfExpressionCause { then, .. }) => { - diag.multipart_suggestion( - "consider `await`ing on both `Future`s", - vec![ - (then.shrink_to_hi(), ".await".to_string()), - (exp_span.shrink_to_hi(), ".await".to_string()), - ], - Applicability::MaybeIncorrect, - ); - } - ObligationCauseCode::MatchExpressionArm(box MatchExpressionArmCause { - prior_arms, - .. - }) => { - if let [.., arm_span] = &prior_arms[..] { + (Some(exp), Some(found)) if self.same_type_modulo_infer(exp, found) => { + match cause.code() { + ObligationCauseCode::IfExpression(box IfExpressionCause { then, .. }) => { diag.multipart_suggestion( "consider `await`ing on both `Future`s", vec![ - (arm_span.shrink_to_hi(), ".await".to_string()), + (then.shrink_to_hi(), ".await".to_string()), (exp_span.shrink_to_hi(), ".await".to_string()), ], Applicability::MaybeIncorrect, ); - } else { + } + ObligationCauseCode::MatchExpressionArm(box MatchExpressionArmCause { + prior_arms, + .. + }) => { + if let [.., arm_span] = &prior_arms[..] { + diag.multipart_suggestion( + "consider `await`ing on both `Future`s", + vec![ + (arm_span.shrink_to_hi(), ".await".to_string()), + (exp_span.shrink_to_hi(), ".await".to_string()), + ], + Applicability::MaybeIncorrect, + ); + } else { + diag.help("consider `await`ing on both `Future`s"); + } + } + _ => { diag.help("consider `await`ing on both `Future`s"); } } - _ => { - diag.help("consider `await`ing on both `Future`s"); - } - }, - (_, Some(ty)) if same_type_modulo_infer(exp_found.expected, ty) => { + } + (_, Some(ty)) if self.same_type_modulo_infer(exp_found.expected, ty) => { diag.span_suggestion_verbose( exp_span.shrink_to_hi(), "consider `await`ing on the `Future`", @@ -1942,7 +1912,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { Applicability::MaybeIncorrect, ); } - (Some(ty), _) if same_type_modulo_infer(ty, exp_found.found) => match cause.code() { + (Some(ty), _) if self.same_type_modulo_infer(ty, exp_found.found) => match cause.code() + { ObligationCauseCode::Pattern { span: Some(span), .. } | ObligationCauseCode::IfExpression(box IfExpressionCause { then: span, .. }) => { diag.span_suggestion_verbose( @@ -1992,7 +1963,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { .iter() .filter(|field| field.vis.is_accessible_from(field.did, self.tcx)) .map(|field| (field.name, field.ty(self.tcx, expected_substs))) - .find(|(_, ty)| same_type_modulo_infer(*ty, exp_found.found)) + .find(|(_, ty)| self.same_type_modulo_infer(*ty, exp_found.found)) { if let ObligationCauseCode::Pattern { span: Some(span), .. } = *cause.code() { if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) { @@ -2057,7 +2028,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { | (_, ty::Infer(_)) | (ty::Param(_), _) | (ty::Infer(_), _) => {} - _ if same_type_modulo_infer(exp_ty, found_ty) => {} + _ if self.same_type_modulo_infer(exp_ty, found_ty) => {} _ => show_suggestion = false, }; } @@ -2179,7 +2150,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { ) { let [expected_tup_elem] = expected_fields[..] else { return }; - if !same_type_modulo_infer(expected_tup_elem, found) { + if !self.same_type_modulo_infer(expected_tup_elem, found) { return; } @@ -2647,6 +2618,45 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { span.is_desugaring(DesugaringKind::QuestionMark) && self.tcx.is_diagnostic_item(sym::From, trait_def_id) } + + /// Structurally compares two types, modulo any inference variables. + /// + /// Returns `true` if two types are equal, or if one type is an inference variable compatible + /// with the other type. A TyVar inference type is compatible with any type, and an IntVar or + /// FloatVar inference type are compatible with themselves or their concrete types (Int and + /// Float types, respectively). When comparing two ADTs, these rules apply recursively. + pub fn same_type_modulo_infer(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> bool { + let (a, b) = self.resolve_vars_if_possible((a, b)); + match (&a.kind(), &b.kind()) { + (&ty::Adt(did_a, substs_a), &ty::Adt(did_b, substs_b)) => { + if did_a != did_b { + return false; + } + + substs_a + .types() + .zip(substs_b.types()) + .all(|(a, b)| self.same_type_modulo_infer(a, b)) + } + (&ty::Int(_) | &ty::Uint(_), &ty::Infer(ty::InferTy::IntVar(_))) + | ( + &ty::Infer(ty::InferTy::IntVar(_)), + &ty::Int(_) | &ty::Uint(_) | &ty::Infer(ty::InferTy::IntVar(_)), + ) + | (&ty::Float(_), &ty::Infer(ty::InferTy::FloatVar(_))) + | ( + &ty::Infer(ty::InferTy::FloatVar(_)), + &ty::Float(_) | &ty::Infer(ty::InferTy::FloatVar(_)), + ) + | (&ty::Infer(ty::InferTy::TyVar(_)), _) + | (_, &ty::Infer(ty::InferTy::TyVar(_))) => true, + (&ty::Ref(_, ty_a, mut_a), &ty::Ref(_, ty_b, mut_b)) => { + mut_a == mut_b && self.same_type_modulo_infer(*ty_a, *ty_b) + } + // FIXME(compiler-errors): This needs to be generalized more + _ => a == b, + } + } } impl<'a, 'tcx> InferCtxt<'a, 'tcx> { diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs index 29df771b95780..1fbc904eb48e8 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -22,7 +22,6 @@ use rustc_hir::intravisit::Visitor; use rustc_hir::GenericParam; use rustc_hir::Item; use rustc_hir::Node; -use rustc_infer::infer::error_reporting::same_type_modulo_infer; use rustc_infer::traits::TraitEngine; use rustc_middle::traits::select::OverflowError; use rustc_middle::ty::abstract_const::NotConstEvaluatable; @@ -640,7 +639,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { if expected.len() == 1 { "" } else { "s" }, ) ); - } else if !same_type_modulo_infer(given_ty, expected_ty) { + } else if !self.same_type_modulo_infer(given_ty, expected_ty) { // Print type mismatch let (expected_args, given_args) = self.cmp(given_ty, expected_ty); From 99c32570bbe20645fa953b0618cec9970c9750fd Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 21 Jul 2022 00:03:02 +0000 Subject: [PATCH 07/13] Do if-expression obligation stuff less eagerly --- compiler/rustc_hir/src/hir.rs | 10 + .../src/infer/error_reporting/mod.rs | 335 ++++++++++++++++-- compiler/rustc_middle/src/traits/mod.rs | 16 +- .../src/traits/structural_impls.rs | 1 - compiler/rustc_typeck/src/check/_match.rs | 124 +++---- .../rustc_typeck/src/check/fn_ctxt/_impl.rs | 84 +---- .../src/check/fn_ctxt/suggestions.rs | 116 +----- ...block-control-flow-static-semantics.stderr | 18 +- src/test/ui/suggestions/return-bindings.fixed | 23 -- src/test/ui/suggestions/return-bindings.rs | 10 +- .../ui/suggestions/return-bindings.stderr | 24 +- 11 files changed, 411 insertions(+), 350 deletions(-) delete mode 100644 src/test/ui/suggestions/return-bindings.fixed diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 48a41c8bd245a..d6e183dd5a3bb 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -954,6 +954,16 @@ pub struct Block<'hir> { pub targeted_by_break: bool, } +impl<'hir> Block<'hir> { + pub fn peel_blocks(&self) -> &Block<'hir> { + let mut block = self; + while let Some(Expr { kind: ExprKind::Block(inner_block, _), .. }) = block.expr { + block = inner_block; + } + block + } +} + #[derive(Debug, HashStable_Generic)] pub struct Pat<'hir> { #[stable_hasher(ignore)] diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs index 6c57e7f4347f2..a8fc306b28677 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/mod.rs @@ -721,25 +721,39 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { } }, ObligationCauseCode::IfExpression(box IfExpressionCause { - then, - else_sp, - outer, - semicolon, + then_id, + else_id, + then_ty, + else_ty, + outer_span, opt_suggest_box_span, }) => { - err.span_label(then, "expected because of this"); - if let Some(sp) = outer { + let then_span = self.find_block_span_from_hir_id(then_id); + let else_span = self.find_block_span_from_hir_id(then_id); + err.span_label(then_span, "expected because of this"); + if let Some(sp) = outer_span { err.span_label(sp, "`if` and `else` have incompatible types"); } + let semicolon = if let hir::Node::Block(blk) = self.tcx.hir().get(then_id) + && let Some(remove_semicolon) = self.could_remove_semicolon(blk, else_ty) + { + Some(remove_semicolon) + } else if let hir::Node::Block(blk) = self.tcx.hir().get(else_id) + && let Some(remove_semicolon) = self.could_remove_semicolon(blk, then_ty) + { + Some(remove_semicolon) + } else { + None + }; if let Some((sp, boxed)) = semicolon { if matches!(boxed, StatementAsExpression::NeedsBoxing) { err.multipart_suggestion( "consider removing this semicolon and boxing the expression", vec![ - (then.shrink_to_lo(), "Box::new(".to_string()), - (then.shrink_to_hi(), ")".to_string()), - (else_sp.shrink_to_lo(), "Box::new(".to_string()), - (else_sp.shrink_to_hi(), ")".to_string()), + (then_span.shrink_to_lo(), "Box::new(".to_string()), + (then_span.shrink_to_hi(), ")".to_string()), + (else_span.shrink_to_lo(), "Box::new(".to_string()), + (else_span.shrink_to_hi(), ")".to_string()), (sp, String::new()), ], Applicability::MachineApplicable, @@ -752,12 +766,21 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { Applicability::MachineApplicable, ); } + } else { + let suggested = if let hir::Node::Block(blk) = self.tcx.hir().get(then_id) { + self.consider_returning_binding(blk, else_ty, err) + } else { + false + }; + if !suggested && let hir::Node::Block(blk) = self.tcx.hir().get(else_id) { + self.consider_returning_binding(blk, then_ty, err); + } } if let Some(ret_sp) = opt_suggest_box_span { self.suggest_boxing_for_return_impl_trait( err, ret_sp, - [then, else_sp].into_iter(), + [then_span, else_span].into_iter(), ); } } @@ -1870,40 +1893,41 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { self.get_impl_future_output_ty(exp_found.expected).map(Binder::skip_binder), self.get_impl_future_output_ty(exp_found.found).map(Binder::skip_binder), ) { - (Some(exp), Some(found)) if self.same_type_modulo_infer(exp, found) => { - match cause.code() { - ObligationCauseCode::IfExpression(box IfExpressionCause { then, .. }) => { + (Some(exp), Some(found)) if self.same_type_modulo_infer(exp, found) => match cause + .code() + { + ObligationCauseCode::IfExpression(box IfExpressionCause { then_id, .. }) => { + let then_span = self.find_block_span_from_hir_id(*then_id); + diag.multipart_suggestion( + "consider `await`ing on both `Future`s", + vec![ + (then_span.shrink_to_hi(), ".await".to_string()), + (exp_span.shrink_to_hi(), ".await".to_string()), + ], + Applicability::MaybeIncorrect, + ); + } + ObligationCauseCode::MatchExpressionArm(box MatchExpressionArmCause { + prior_arms, + .. + }) => { + if let [.., arm_span] = &prior_arms[..] { diag.multipart_suggestion( "consider `await`ing on both `Future`s", vec![ - (then.shrink_to_hi(), ".await".to_string()), + (arm_span.shrink_to_hi(), ".await".to_string()), (exp_span.shrink_to_hi(), ".await".to_string()), ], Applicability::MaybeIncorrect, ); - } - ObligationCauseCode::MatchExpressionArm(box MatchExpressionArmCause { - prior_arms, - .. - }) => { - if let [.., arm_span] = &prior_arms[..] { - diag.multipart_suggestion( - "consider `await`ing on both `Future`s", - vec![ - (arm_span.shrink_to_hi(), ".await".to_string()), - (exp_span.shrink_to_hi(), ".await".to_string()), - ], - Applicability::MaybeIncorrect, - ); - } else { - diag.help("consider `await`ing on both `Future`s"); - } - } - _ => { + } else { diag.help("consider `await`ing on both `Future`s"); } } - } + _ => { + diag.help("consider `await`ing on both `Future`s"); + } + }, (_, Some(ty)) if self.same_type_modulo_infer(exp_found.expected, ty) => { diag.span_suggestion_verbose( exp_span.shrink_to_hi(), @@ -1914,10 +1938,18 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { } (Some(ty), _) if self.same_type_modulo_infer(ty, exp_found.found) => match cause.code() { - ObligationCauseCode::Pattern { span: Some(span), .. } - | ObligationCauseCode::IfExpression(box IfExpressionCause { then: span, .. }) => { + ObligationCauseCode::Pattern { span: Some(then_span), .. } => { + diag.span_suggestion_verbose( + then_span.shrink_to_hi(), + "consider `await`ing on the `Future`", + ".await", + Applicability::MaybeIncorrect, + ); + } + ObligationCauseCode::IfExpression(box IfExpressionCause { then_id, .. }) => { + let then_span = self.find_block_span_from_hir_id(*then_id); diag.span_suggestion_verbose( - span.shrink_to_hi(), + then_span.shrink_to_hi(), "consider `await`ing on the `Future`", ".await", Applicability::MaybeIncorrect, @@ -2808,3 +2840,230 @@ impl TyCategory { } } } + +impl<'tcx> InferCtxt<'_, 'tcx> { + pub fn find_block_span(&self, block: &'tcx hir::Block<'tcx>) -> Span { + let block = block.peel_blocks(); + if let Some(expr) = &block.expr { + expr.span + } else if let Some(stmt) = block.stmts.last() { + // possibly incorrect trailing `;` in the else arm + stmt.span + } else { + // empty block; point at its entirety + block.span + } + } + + pub fn find_block_span_from_hir_id(&self, hir_id: hir::HirId) -> Span { + match self.tcx.hir().get(hir_id) { + hir::Node::Block(blk) => self.find_block_span(blk), + // The parser was in a weird state if either of these happen... + hir::Node::Expr(e) => e.span, + _ => rustc_span::DUMMY_SP, + } + } + + pub fn could_remove_semicolon( + &self, + blk: &'tcx hir::Block<'tcx>, + expected_ty: Ty<'tcx>, + ) -> Option<(Span, StatementAsExpression)> { + let blk = blk.peel_blocks(); + // Do not suggest if we have a tail expr. + if blk.expr.is_some() { + return None; + } + // Be helpful when the user wrote `{... expr;}` and + // taking the `;` off is enough to fix the error. + let last_stmt = blk.stmts.last()?; + let hir::StmtKind::Semi(ref last_expr) = last_stmt.kind else { + return None; + }; + let last_expr_ty = self.in_progress_typeck_results?.borrow().expr_ty_opt(*last_expr)?; + let needs_box = match (last_expr_ty.kind(), expected_ty.kind()) { + _ if last_expr_ty.references_error() => return None, + _ if self.same_type_modulo_infer(last_expr_ty, expected_ty) => { + StatementAsExpression::CorrectType + } + (ty::Opaque(last_def_id, _), ty::Opaque(exp_def_id, _)) + if last_def_id == exp_def_id => + { + StatementAsExpression::CorrectType + } + (ty::Opaque(last_def_id, last_bounds), ty::Opaque(exp_def_id, exp_bounds)) => { + debug!( + "both opaque, likely future {:?} {:?} {:?} {:?}", + last_def_id, last_bounds, exp_def_id, exp_bounds + ); + + let last_local_id = last_def_id.as_local()?; + let exp_local_id = exp_def_id.as_local()?; + + match ( + &self.tcx.hir().expect_item(last_local_id).kind, + &self.tcx.hir().expect_item(exp_local_id).kind, + ) { + ( + hir::ItemKind::OpaqueTy(hir::OpaqueTy { bounds: last_bounds, .. }), + hir::ItemKind::OpaqueTy(hir::OpaqueTy { bounds: exp_bounds, .. }), + ) if iter::zip(*last_bounds, *exp_bounds).all(|(left, right)| { + match (left, right) { + ( + hir::GenericBound::Trait(tl, ml), + hir::GenericBound::Trait(tr, mr), + ) if tl.trait_ref.trait_def_id() == tr.trait_ref.trait_def_id() + && ml == mr => + { + true + } + ( + hir::GenericBound::LangItemTrait(langl, _, _, argsl), + hir::GenericBound::LangItemTrait(langr, _, _, argsr), + ) if langl == langr => { + // FIXME: consider the bounds! + debug!("{:?} {:?}", argsl, argsr); + true + } + _ => false, + } + }) => + { + StatementAsExpression::NeedsBoxing + } + _ => StatementAsExpression::CorrectType, + } + } + _ => return None, + }; + let span = if last_stmt.span.from_expansion() { + let mac_call = rustc_span::source_map::original_sp(last_stmt.span, blk.span); + self.tcx.sess.source_map().mac_call_stmt_semi_span(mac_call)? + } else { + last_stmt.span.with_lo(last_stmt.span.hi() - BytePos(1)) + }; + Some((span, needs_box)) + } + + pub fn consider_returning_binding( + &self, + blk: &'tcx hir::Block<'tcx>, + expected_ty: Ty<'tcx>, + err: &mut Diagnostic, + ) -> bool { + let blk = blk.peel_blocks(); + // Do not suggest if we have a tail expr. + if blk.expr.is_some() { + return false; + } + let mut shadowed = FxHashSet::default(); + let mut candidate_idents = vec![]; + let mut find_compatible_candidates = |pat: &hir::Pat<'_>| { + if let hir::PatKind::Binding(_, hir_id, ident, _) = &pat.kind + && let Some(pat_ty) = self + .in_progress_typeck_results + .and_then(|typeck_results| typeck_results.borrow().node_type_opt(*hir_id)) + { + let pat_ty = self.resolve_vars_if_possible(pat_ty); + if self.same_type_modulo_infer(pat_ty, expected_ty) + && !(pat_ty, expected_ty).references_error() + && shadowed.insert(ident.name) + { + candidate_idents.push((*ident, pat_ty)); + } + } + true + }; + + let hir = self.tcx.hir(); + for stmt in blk.stmts.iter().rev() { + let hir::StmtKind::Local(local) = &stmt.kind else { continue; }; + local.pat.walk(&mut find_compatible_candidates); + } + match hir.find(hir.get_parent_node(blk.hir_id)) { + Some(hir::Node::Expr(hir::Expr { hir_id, .. })) => { + match hir.find(hir.get_parent_node(*hir_id)) { + Some(hir::Node::Arm(hir::Arm { pat, .. })) => { + pat.walk(&mut find_compatible_candidates); + } + Some( + hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(_, _, body), .. }) + | hir::Node::ImplItem(hir::ImplItem { + kind: hir::ImplItemKind::Fn(_, body), + .. + }) + | hir::Node::TraitItem(hir::TraitItem { + kind: hir::TraitItemKind::Fn(_, hir::TraitFn::Provided(body)), + .. + }) + | hir::Node::Expr(hir::Expr { + kind: hir::ExprKind::Closure(hir::Closure { body, .. }), + .. + }), + ) => { + for param in hir.body(*body).params { + param.pat.walk(&mut find_compatible_candidates); + } + } + Some(hir::Node::Expr(hir::Expr { + kind: + hir::ExprKind::If( + hir::Expr { kind: hir::ExprKind::Let(let_), .. }, + then_block, + _, + ), + .. + })) if then_block.hir_id == *hir_id => { + let_.pat.walk(&mut find_compatible_candidates); + } + _ => {} + } + } + _ => {} + } + + match &candidate_idents[..] { + [(ident, _ty)] => { + let sm = self.tcx.sess.source_map(); + if let Some(stmt) = blk.stmts.last() { + let stmt_span = sm.stmt_span(stmt.span, blk.span); + let sugg = if sm.is_multiline(blk.span) + && let Some(spacing) = sm.indentation_before(stmt_span) + { + format!("\n{spacing}{ident}") + } else { + format!(" {ident}") + }; + err.span_suggestion_verbose( + stmt_span.shrink_to_hi(), + format!("consider returning the local binding `{ident}`"), + sugg, + Applicability::MaybeIncorrect, + ); + } else { + let sugg = if sm.is_multiline(blk.span) + && let Some(spacing) = sm.indentation_before(blk.span.shrink_to_lo()) + { + format!("\n{spacing} {ident}\n{spacing}") + } else { + format!(" {ident} ") + }; + let left_span = sm.span_through_char(blk.span, '{').shrink_to_hi(); + err.span_suggestion_verbose( + sm.span_extend_while(left_span, |c| c.is_whitespace()).unwrap_or(left_span), + format!("consider returning the local binding `{ident}`"), + sugg, + Applicability::MaybeIncorrect, + ); + } + true + } + values if (1..3).contains(&values.len()) => { + let spans = values.iter().map(|(ident, _)| ident.span).collect::>(); + err.span_note(spans, "consider returning one of these bindings"); + true + } + _ => false, + } + } +} diff --git a/compiler/rustc_middle/src/traits/mod.rs b/compiler/rustc_middle/src/traits/mod.rs index 75559d4f8b843..7a38c800ccf5f 100644 --- a/compiler/rustc_middle/src/traits/mod.rs +++ b/compiler/rustc_middle/src/traits/mod.rs @@ -351,7 +351,7 @@ pub enum ObligationCauseCode<'tcx> { ConstPatternStructural, /// Computing common supertype in an if expression - IfExpression(Box), + IfExpression(Box>), /// Computing common supertype of an if expression with no else counter-part IfExpressionWithNoElse, @@ -498,12 +498,14 @@ pub struct MatchExpressionArmCause<'tcx> { pub opt_suggest_box_span: Option, } -#[derive(Clone, Debug, PartialEq, Eq, Hash)] -pub struct IfExpressionCause { - pub then: Span, - pub else_sp: Span, - pub outer: Option, - pub semicolon: Option<(Span, StatementAsExpression)>, +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Lift, TypeFoldable, TypeVisitable)] +pub struct IfExpressionCause<'tcx> { + pub then_id: hir::HirId, + pub else_id: hir::HirId, + pub then_ty: Ty<'tcx>, + pub else_ty: Ty<'tcx>, + pub outer_span: Option, pub opt_suggest_box_span: Option, } diff --git a/compiler/rustc_middle/src/traits/structural_impls.rs b/compiler/rustc_middle/src/traits/structural_impls.rs index 8f1a1564fc8e8..7fbd57ac7354a 100644 --- a/compiler/rustc_middle/src/traits/structural_impls.rs +++ b/compiler/rustc_middle/src/traits/structural_impls.rs @@ -130,7 +130,6 @@ impl fmt::Debug for traits::ImplSourceConstDestructData { // Lift implementations TrivialTypeTraversalAndLiftImpls! { - super::IfExpressionCause, super::ImplSourceDiscriminantKindData, super::ImplSourcePointeeData, } diff --git a/compiler/rustc_typeck/src/check/_match.rs b/compiler/rustc_typeck/src/check/_match.rs index c733f0d3c86d0..00547a6d8278f 100644 --- a/compiler/rustc_typeck/src/check/_match.rs +++ b/compiler/rustc_typeck/src/check/_match.rs @@ -216,13 +216,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ) -> (Span, Option<(Span, StatementAsExpression)>) { let arm = &arms[i]; let (arm_span, mut semi_span) = if let hir::ExprKind::Block(blk, _) = &arm.body.kind { - self.find_block_span(blk, prior_arm_ty) + ( + self.find_block_span(blk), + prior_arm_ty + .and_then(|prior_arm_ty| self.could_remove_semicolon(blk, prior_arm_ty)), + ) } else { (arm.body.span, None) }; if semi_span.is_none() && i > 0 { if let hir::ExprKind::Block(blk, _) = &arms[i - 1].body.kind { - let (_, semi_span_prev) = self.find_block_span(blk, Some(arm_ty)); + let semi_span_prev = self.could_remove_semicolon(blk, arm_ty); semi_span = semi_span_prev; } } @@ -313,7 +317,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { else_ty: Ty<'tcx>, opt_suggest_box_span: Option, ) -> ObligationCause<'tcx> { - let mut outer_sp = if self.tcx.sess.source_map().is_multiline(span) { + let mut outer_span = if self.tcx.sess.source_map().is_multiline(span) { // The `if`/`else` isn't in one line in the output, include some context to make it // clear it is an if/else expression: // ``` @@ -339,69 +343,67 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { None }; - let mut remove_semicolon = None; - let error_sp = if let ExprKind::Block(block, _) = &else_expr.kind { - let (error_sp, semi_sp) = self.find_block_span(block, Some(then_ty)); - remove_semicolon = semi_sp; - if block.expr.is_none() && block.stmts.is_empty() { - // Avoid overlapping spans that aren't as readable: - // ``` - // 2 | let x = if true { - // | _____________- - // 3 | | 3 - // | | - expected because of this - // 4 | | } else { - // | |____________^ - // 5 | || - // 6 | || }; - // | || ^ - // | ||_____| - // | |______if and else have incompatible types - // | expected integer, found `()` - // ``` - // by not pointing at the entire expression: - // ``` - // 2 | let x = if true { - // | ------- `if` and `else` have incompatible types - // 3 | 3 - // | - expected because of this - // 4 | } else { - // | ____________^ - // 5 | | - // 6 | | }; - // | |_____^ expected integer, found `()` - // ``` - if outer_sp.is_some() { - outer_sp = Some(self.tcx.sess.source_map().guess_head_span(span)); - } + let (error_sp, else_id) = if let ExprKind::Block(block, _) = &else_expr.kind { + let block = block.peel_blocks(); + + // Avoid overlapping spans that aren't as readable: + // ``` + // 2 | let x = if true { + // | _____________- + // 3 | | 3 + // | | - expected because of this + // 4 | | } else { + // | |____________^ + // 5 | || + // 6 | || }; + // | || ^ + // | ||_____| + // | |______if and else have incompatible types + // | expected integer, found `()` + // ``` + // by not pointing at the entire expression: + // ``` + // 2 | let x = if true { + // | ------- `if` and `else` have incompatible types + // 3 | 3 + // | - expected because of this + // 4 | } else { + // | ____________^ + // 5 | | + // 6 | | }; + // | |_____^ expected integer, found `()` + // ``` + if block.expr.is_none() && block.stmts.is_empty() + && let Some(outer_span) = &mut outer_span + { + *outer_span = self.tcx.sess.source_map().guess_head_span(*outer_span); } - error_sp + + (self.find_block_span(block), block.hir_id) } else { - // shouldn't happen unless the parser has done something weird - else_expr.span + (else_expr.span, else_expr.hir_id) }; - // Compute `Span` of `then` part of `if`-expression. - let then_sp = if let ExprKind::Block(block, _) = &then_expr.kind { - let (then_sp, semi_sp) = self.find_block_span(block, Some(else_ty)); - remove_semicolon = remove_semicolon.or(semi_sp); + let then_id = if let ExprKind::Block(block, _) = &then_expr.kind { + let block = block.peel_blocks(); + // Exclude overlapping spans if block.expr.is_none() && block.stmts.is_empty() { - outer_sp = None; // same as in `error_sp`; cleanup output + outer_span = None; } - then_sp + block.hir_id } else { - // shouldn't happen unless the parser has done something weird - then_expr.span + then_expr.hir_id }; // Finally construct the cause: self.cause( error_sp, ObligationCauseCode::IfExpression(Box::new(IfExpressionCause { - then: then_sp, - else_sp: error_sp, - outer: outer_sp, - semicolon: remove_semicolon, + else_id, + then_id, + then_ty, + else_ty, + outer_span, opt_suggest_box_span, })), ) @@ -482,22 +484,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } - fn find_block_span( - &self, - block: &'tcx hir::Block<'tcx>, - expected_ty: Option>, - ) -> (Span, Option<(Span, StatementAsExpression)>) { - if let Some(expr) = &block.expr { - (expr.span, None) - } else if let Some(stmt) = block.stmts.last() { - // possibly incorrect trailing `;` in the else arm - (stmt.span, expected_ty.and_then(|ty| self.could_remove_semicolon(block, ty))) - } else { - // empty block; point at its entirety - (block.span, None) - } - } - // When we have a `match` as a tail expression in a `fn` with a returned `impl Trait` // we check if the different arms would work with boxed trait objects instead and // provide a structured suggestion in that case. diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs b/compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs index d079aeb4801ca..21b3c9063a78a 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs @@ -30,17 +30,15 @@ use rustc_middle::ty::{ }; use rustc_session::lint; use rustc_span::hygiene::DesugaringKind; -use rustc_span::source_map::{original_sp, DUMMY_SP}; use rustc_span::symbol::{kw, sym, Ident}; -use rustc_span::{self, BytePos, Span}; +use rustc_span::{Span, DUMMY_SP}; use rustc_trait_selection::infer::InferCtxtExt as _; use rustc_trait_selection::traits::error_reporting::InferCtxtExt as _; use rustc_trait_selection::traits::{ - self, ObligationCause, ObligationCauseCode, StatementAsExpression, TraitEngine, TraitEngineExt, + self, ObligationCause, ObligationCauseCode, TraitEngine, TraitEngineExt, }; use std::collections::hash_map::Entry; -use std::iter; use std::slice; impl<'a, 'tcx> FnCtxt<'a, 'tcx> { @@ -1059,84 +1057,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { )); } - pub(in super::super) fn could_remove_semicolon( - &self, - blk: &'tcx hir::Block<'tcx>, - expected_ty: Ty<'tcx>, - ) -> Option<(Span, StatementAsExpression)> { - // Be helpful when the user wrote `{... expr;}` and - // taking the `;` off is enough to fix the error. - let last_stmt = blk.stmts.last()?; - let hir::StmtKind::Semi(ref last_expr) = last_stmt.kind else { - return None; - }; - let last_expr_ty = self.node_ty(last_expr.hir_id); - let needs_box = match (last_expr_ty.kind(), expected_ty.kind()) { - (ty::Opaque(last_def_id, _), ty::Opaque(exp_def_id, _)) - if last_def_id == exp_def_id => - { - StatementAsExpression::CorrectType - } - (ty::Opaque(last_def_id, last_bounds), ty::Opaque(exp_def_id, exp_bounds)) => { - debug!( - "both opaque, likely future {:?} {:?} {:?} {:?}", - last_def_id, last_bounds, exp_def_id, exp_bounds - ); - - let last_local_id = last_def_id.as_local()?; - let exp_local_id = exp_def_id.as_local()?; - - match ( - &self.tcx.hir().expect_item(last_local_id).kind, - &self.tcx.hir().expect_item(exp_local_id).kind, - ) { - ( - hir::ItemKind::OpaqueTy(hir::OpaqueTy { bounds: last_bounds, .. }), - hir::ItemKind::OpaqueTy(hir::OpaqueTy { bounds: exp_bounds, .. }), - ) if iter::zip(*last_bounds, *exp_bounds).all(|(left, right)| { - match (left, right) { - ( - hir::GenericBound::Trait(tl, ml), - hir::GenericBound::Trait(tr, mr), - ) if tl.trait_ref.trait_def_id() == tr.trait_ref.trait_def_id() - && ml == mr => - { - true - } - ( - hir::GenericBound::LangItemTrait(langl, _, _, argsl), - hir::GenericBound::LangItemTrait(langr, _, _, argsr), - ) if langl == langr => { - // FIXME: consider the bounds! - debug!("{:?} {:?}", argsl, argsr); - true - } - _ => false, - } - }) => - { - StatementAsExpression::NeedsBoxing - } - _ => StatementAsExpression::CorrectType, - } - } - _ => StatementAsExpression::CorrectType, - }; - if (matches!(last_expr_ty.kind(), ty::Error(_)) - || self.can_sub(self.param_env, last_expr_ty, expected_ty).is_err()) - && matches!(needs_box, StatementAsExpression::CorrectType) - { - return None; - } - let span = if last_stmt.span.from_expansion() { - let mac_call = original_sp(last_stmt.span, blk.span); - self.tcx.sess.source_map().mac_call_stmt_semi_span(mac_call)? - } else { - last_stmt.span.with_lo(last_stmt.span.hi() - BytePos(1)) - }; - Some((span, needs_box)) - } - // Instantiates the given path, which must refer to an item with the given // number of type parameters and type. #[instrument(skip(self, span), level = "debug")] diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs b/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs index 871fc4a21f2a7..3e6ff72204f4c 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs @@ -3,7 +3,6 @@ use crate::astconv::AstConv; use crate::errors::{AddReturnTypeSuggestion, ExpectedReturnTypeLabel}; use rustc_ast::util::parser::ExprPrecedence; -use rustc_data_structures::fx::FxHashSet; use rustc_errors::{Applicability, Diagnostic, MultiSpan}; use rustc_hir as hir; use rustc_hir::def::{CtorOf, DefKind}; @@ -14,7 +13,7 @@ use rustc_hir::{ use rustc_infer::infer::{self, TyCtxtInferExt}; use rustc_infer::traits::{self, StatementAsExpression}; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{self, Binder, IsSuggestable, Subst, ToPredicate, Ty, TypeVisitable}; +use rustc_middle::ty::{self, Binder, IsSuggestable, Subst, ToPredicate, Ty}; use rustc_span::symbol::sym; use rustc_span::Span; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _; @@ -904,117 +903,4 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { false } } - - pub(crate) fn consider_returning_binding( - &self, - blk: &'tcx hir::Block<'tcx>, - expected_ty: Ty<'tcx>, - err: &mut Diagnostic, - ) { - let mut shadowed = FxHashSet::default(); - let mut candidate_idents = vec![]; - let mut find_compatible_candidates = |pat: &hir::Pat<'_>| { - if let hir::PatKind::Binding(_, hir_id, ident, _) = &pat.kind - && let Some(pat_ty) = self.typeck_results.borrow().node_type_opt(*hir_id) - { - let pat_ty = self.resolve_vars_if_possible(pat_ty); - if self.can_coerce(pat_ty, expected_ty) - && !(pat_ty, expected_ty).references_error() - && shadowed.insert(ident.name) - { - candidate_idents.push((*ident, pat_ty)); - } - } - true - }; - - let hir = self.tcx.hir(); - for stmt in blk.stmts.iter().rev() { - let StmtKind::Local(local) = &stmt.kind else { continue; }; - local.pat.walk(&mut find_compatible_candidates); - } - match hir.find(hir.get_parent_node(blk.hir_id)) { - Some(hir::Node::Expr(hir::Expr { hir_id, .. })) => { - match hir.find(hir.get_parent_node(*hir_id)) { - Some(hir::Node::Arm(hir::Arm { pat, .. })) => { - pat.walk(&mut find_compatible_candidates); - } - Some( - hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(_, _, body), .. }) - | hir::Node::ImplItem(hir::ImplItem { - kind: hir::ImplItemKind::Fn(_, body), - .. - }) - | hir::Node::TraitItem(hir::TraitItem { - kind: hir::TraitItemKind::Fn(_, hir::TraitFn::Provided(body)), - .. - }) - | hir::Node::Expr(hir::Expr { - kind: hir::ExprKind::Closure(hir::Closure { body, .. }), - .. - }), - ) => { - for param in hir.body(*body).params { - param.pat.walk(&mut find_compatible_candidates); - } - } - Some(hir::Node::Expr(hir::Expr { - kind: - hir::ExprKind::If( - hir::Expr { kind: hir::ExprKind::Let(let_), .. }, - then_block, - _, - ), - .. - })) if then_block.hir_id == *hir_id => { - let_.pat.walk(&mut find_compatible_candidates); - } - _ => {} - } - } - _ => {} - } - - match &candidate_idents[..] { - [(ident, _ty)] => { - let sm = self.tcx.sess.source_map(); - if let Some(stmt) = blk.stmts.last() { - let stmt_span = sm.stmt_span(stmt.span, blk.span); - let sugg = if sm.is_multiline(blk.span) - && let Some(spacing) = sm.indentation_before(stmt_span) - { - format!("\n{spacing}{ident}") - } else { - format!(" {ident}") - }; - err.span_suggestion_verbose( - stmt_span.shrink_to_hi(), - format!("consider returning the local binding `{ident}`"), - sugg, - Applicability::MachineApplicable, - ); - } else { - let sugg = if sm.is_multiline(blk.span) - && let Some(spacing) = sm.indentation_before(blk.span.shrink_to_lo()) - { - format!("\n{spacing} {ident}\n{spacing}") - } else { - format!(" {ident} ") - }; - let left_span = sm.span_through_char(blk.span, '{').shrink_to_hi(); - err.span_suggestion_verbose( - sm.span_extend_while(left_span, |c| c.is_whitespace()).unwrap_or(left_span), - format!("consider returning the local binding `{ident}`"), - sugg, - Applicability::MachineApplicable, - ); - } - } - values if (1..3).contains(&values.len()) => { - let spans = values.iter().map(|(ident, _)| ident.span).collect::>(); - err.span_note(spans, "consider returning one of these bindings"); - } - _ => {} - } - } } diff --git a/src/test/ui/async-await/async-block-control-flow-static-semantics.stderr b/src/test/ui/async-await/async-block-control-flow-static-semantics.stderr index ada6e357aea5e..e5887689690e7 100644 --- a/src/test/ui/async-await/async-block-control-flow-static-semantics.stderr +++ b/src/test/ui/async-await/async-block-control-flow-static-semantics.stderr @@ -18,14 +18,6 @@ LL | | break 0u8; LL | | }; | |_________- enclosing `async` block -error[E0271]: type mismatch resolving ` as Future>::Output == ()` - --> $DIR/async-block-control-flow-static-semantics.rs:26:39 - | -LL | let _: &dyn Future = █ - | ^^^^^^ expected `()`, found `u8` - | - = note: required for the cast from `impl Future` to the object type `dyn Future` - error[E0308]: mismatched types --> $DIR/async-block-control-flow-static-semantics.rs:21:58 | @@ -40,7 +32,7 @@ LL | | } | |_^ expected `u8`, found `()` error[E0271]: type mismatch resolving ` as Future>::Output == ()` - --> $DIR/async-block-control-flow-static-semantics.rs:17:39 + --> $DIR/async-block-control-flow-static-semantics.rs:26:39 | LL | let _: &dyn Future = █ | ^^^^^^ expected `()`, found `u8` @@ -55,6 +47,14 @@ LL | fn return_targets_async_block_not_fn() -> u8 { | | | implicitly returns `()` as its body has no tail or `return` expression +error[E0271]: type mismatch resolving ` as Future>::Output == ()` + --> $DIR/async-block-control-flow-static-semantics.rs:17:39 + | +LL | let _: &dyn Future = █ + | ^^^^^^ expected `()`, found `u8` + | + = note: required for the cast from `impl Future` to the object type `dyn Future` + error[E0308]: mismatched types --> $DIR/async-block-control-flow-static-semantics.rs:47:44 | diff --git a/src/test/ui/suggestions/return-bindings.fixed b/src/test/ui/suggestions/return-bindings.fixed deleted file mode 100644 index 4fabc411abcbe..0000000000000 --- a/src/test/ui/suggestions/return-bindings.fixed +++ /dev/null @@ -1,23 +0,0 @@ -// run-rustfix - -#![allow(unused)] - -fn a(i: i32) -> i32 { i } -//~^ ERROR mismatched types - -fn b(opt_str: Option) { - let s: String = if let Some(s) = opt_str { - s - //~^ ERROR mismatched types - } else { - String::new() - }; -} - -fn c() -> Option { - //~^ ERROR mismatched types - let x = Some(1); - x -} - -fn main() {} diff --git a/src/test/ui/suggestions/return-bindings.rs b/src/test/ui/suggestions/return-bindings.rs index d05b4ba27d6e8..80c83a70d50c1 100644 --- a/src/test/ui/suggestions/return-bindings.rs +++ b/src/test/ui/suggestions/return-bindings.rs @@ -1,5 +1,3 @@ -// run-rustfix - #![allow(unused)] fn a(i: i32) -> i32 {} @@ -18,4 +16,12 @@ fn c() -> Option { let x = Some(1); } +fn d(opt_str: Option) { + let s: String = if let Some(s) = opt_str { + //~^ ERROR mismatched types + } else { + String::new() + }; +} + fn main() {} diff --git a/src/test/ui/suggestions/return-bindings.stderr b/src/test/ui/suggestions/return-bindings.stderr index e5d4925500556..53ef7106fa808 100644 --- a/src/test/ui/suggestions/return-bindings.stderr +++ b/src/test/ui/suggestions/return-bindings.stderr @@ -1,5 +1,5 @@ error[E0308]: mismatched types - --> $DIR/return-bindings.rs:5:17 + --> $DIR/return-bindings.rs:3:17 | LL | fn a(i: i32) -> i32 {} | - ^^^ expected `i32`, found `()` @@ -12,7 +12,7 @@ LL | fn a(i: i32) -> i32 { i } | + error[E0308]: mismatched types - --> $DIR/return-bindings.rs:9:46 + --> $DIR/return-bindings.rs:7:46 | LL | let s: String = if let Some(s) = opt_str { | ______________________________________________^ @@ -28,7 +28,7 @@ LL ~ | error[E0308]: mismatched types - --> $DIR/return-bindings.rs:16:11 + --> $DIR/return-bindings.rs:14:11 | LL | fn c() -> Option { | - ^^^^^^^^^^^ expected enum `Option`, found `()` @@ -43,6 +43,22 @@ LL ~ let x = Some(1); LL + x | -error: aborting due to 3 previous errors +error[E0308]: mismatched types + --> $DIR/return-bindings.rs:20:46 + | +LL | let s: String = if let Some(s) = opt_str { + | ______________________________________________^ +LL | | +LL | | } else { + | |_____^ expected struct `String`, found `()` + | +help: consider returning the local binding `s` + | +LL ~ let s: String = if let Some(s) = opt_str { +LL + s +LL ~ + | + +error: aborting due to 4 previous errors For more information about this error, try `rustc --explain E0308`. From 8926dac54911e9382763876dc3a8e7a4ae50e270 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 21 Jul 2022 01:26:00 +0000 Subject: [PATCH 08/13] And for patterns too --- .../src/infer/error_reporting/mod.rs | 167 ++++++++++-------- compiler/rustc_middle/src/traits/mod.rs | 7 +- compiler/rustc_typeck/src/check/_match.rs | 57 ++---- src/test/ui/suggestions/return-bindings.rs | 24 +++ .../ui/suggestions/return-bindings.stderr | 48 ++++- 5 files changed, 184 insertions(+), 119 deletions(-) diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs index a8fc306b28677..5c1ab2bf5888d 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/mod.rs @@ -614,13 +614,16 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { err.span_label(span, "expected due to this"); } ObligationCauseCode::MatchExpressionArm(box MatchExpressionArmCause { - semi_span, + arm_block_id, + arm_span, + arm_ty, + prior_arm_block_id, + prior_arm_span, + prior_arm_ty, source, ref prior_arms, - last_ty, scrut_hir_id, opt_suggest_box_span, - arm_span, scrut_span, .. }) => match source { @@ -651,10 +654,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { } } _ => { - // `last_ty` can be `!`, `expected` will have better info when present. + // `prior_arm_ty` can be `!`, `expected` will have better info when present. let t = self.resolve_vars_if_possible(match exp_found { Some(ty::error::ExpectedFound { expected, .. }) => expected, - _ => last_ty, + _ => prior_arm_ty, }); let source_map = self.tcx.sess.source_map(); let mut any_multiline_arm = source_map.is_multiline(arm_span); @@ -679,37 +682,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { }; let msg = "`match` arms have incompatible types"; err.span_label(outer_error_span, msg); - if let Some((sp, boxed)) = semi_span { - if let (StatementAsExpression::NeedsBoxing, [.., prior_arm]) = - (boxed, &prior_arms[..]) - { - err.multipart_suggestion( - "consider removing this semicolon and boxing the expressions", - vec![ - (prior_arm.shrink_to_lo(), "Box::new(".to_string()), - (prior_arm.shrink_to_hi(), ")".to_string()), - (arm_span.shrink_to_lo(), "Box::new(".to_string()), - (arm_span.shrink_to_hi(), ")".to_string()), - (sp, String::new()), - ], - Applicability::HasPlaceholders, - ); - } else if matches!(boxed, StatementAsExpression::NeedsBoxing) { - err.span_suggestion_short( - sp, - "consider removing this semicolon and boxing the expressions", - "", - Applicability::MachineApplicable, - ); - } else { - err.span_suggestion_short( - sp, - "consider removing this semicolon", - "", - Applicability::MachineApplicable, - ); - } - } + self.suggest_remove_semi_or_return_binding( + err, + prior_arm_block_id, + prior_arm_ty, + prior_arm_span, + arm_block_id, + arm_ty, + arm_span, + ); if let Some(ret_sp) = opt_suggest_box_span { // Get return type span and point to it. self.suggest_boxing_for_return_impl_trait( @@ -734,48 +715,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { if let Some(sp) = outer_span { err.span_label(sp, "`if` and `else` have incompatible types"); } - let semicolon = if let hir::Node::Block(blk) = self.tcx.hir().get(then_id) - && let Some(remove_semicolon) = self.could_remove_semicolon(blk, else_ty) - { - Some(remove_semicolon) - } else if let hir::Node::Block(blk) = self.tcx.hir().get(else_id) - && let Some(remove_semicolon) = self.could_remove_semicolon(blk, then_ty) - { - Some(remove_semicolon) - } else { - None - }; - if let Some((sp, boxed)) = semicolon { - if matches!(boxed, StatementAsExpression::NeedsBoxing) { - err.multipart_suggestion( - "consider removing this semicolon and boxing the expression", - vec![ - (then_span.shrink_to_lo(), "Box::new(".to_string()), - (then_span.shrink_to_hi(), ")".to_string()), - (else_span.shrink_to_lo(), "Box::new(".to_string()), - (else_span.shrink_to_hi(), ")".to_string()), - (sp, String::new()), - ], - Applicability::MachineApplicable, - ); - } else { - err.span_suggestion_short( - sp, - "consider removing this semicolon", - "", - Applicability::MachineApplicable, - ); - } - } else { - let suggested = if let hir::Node::Block(blk) = self.tcx.hir().get(then_id) { - self.consider_returning_binding(blk, else_ty, err) - } else { - false - }; - if !suggested && let hir::Node::Block(blk) = self.tcx.hir().get(else_id) { - self.consider_returning_binding(blk, then_ty, err); - } - } + self.suggest_remove_semi_or_return_binding( + err, + Some(then_id), + then_ty, + then_span, + Some(else_id), + else_ty, + else_span, + ); if let Some(ret_sp) = opt_suggest_box_span { self.suggest_boxing_for_return_impl_trait( err, @@ -800,6 +748,69 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { } } + fn suggest_remove_semi_or_return_binding( + &self, + err: &mut Diagnostic, + first_id: Option, + first_ty: Ty<'tcx>, + first_span: Span, + second_id: Option, + second_ty: Ty<'tcx>, + second_span: Span, + ) { + let semicolon = + if let Some(first_id) = first_id + && let hir::Node::Block(blk) = self.tcx.hir().get(first_id) + && let Some(remove_semicolon) = self.could_remove_semicolon(blk, second_ty) + { + Some(remove_semicolon) + } else if let Some(second_id) = second_id + && let hir::Node::Block(blk) = self.tcx.hir().get(second_id) + && let Some(remove_semicolon) = self.could_remove_semicolon(blk, first_ty) + { + Some(remove_semicolon) + } else { + None + }; + if let Some((sp, boxed)) = semicolon { + if matches!(boxed, StatementAsExpression::NeedsBoxing) { + err.multipart_suggestion( + "consider removing this semicolon and boxing the expressions", + vec![ + (first_span.shrink_to_lo(), "Box::new(".to_string()), + (first_span.shrink_to_hi(), ")".to_string()), + (second_span.shrink_to_lo(), "Box::new(".to_string()), + (second_span.shrink_to_hi(), ")".to_string()), + (sp, String::new()), + ], + Applicability::MachineApplicable, + ); + } else { + err.span_suggestion_short( + sp, + "consider removing this semicolon", + "", + Applicability::MachineApplicable, + ); + } + } else { + let suggested = + if let Some(first_id) = first_id + && let hir::Node::Block(blk) = self.tcx.hir().get(first_id) + { + self.consider_returning_binding(blk, second_ty, err) + } else { + false + }; + if !suggested + && let Some(second_id) = second_id + && let hir::Node::Block(blk) = self.tcx.hir().get(second_id) + { + self.consider_returning_binding(blk, first_ty, err); + } + } + } + fn suggest_boxing_for_return_impl_trait( &self, err: &mut Diagnostic, diff --git a/compiler/rustc_middle/src/traits/mod.rs b/compiler/rustc_middle/src/traits/mod.rs index 7a38c800ccf5f..c55971557fac1 100644 --- a/compiler/rustc_middle/src/traits/mod.rs +++ b/compiler/rustc_middle/src/traits/mod.rs @@ -488,12 +488,15 @@ impl<'tcx> ty::Lift<'tcx> for StatementAsExpression { #[derive(Clone, Debug, PartialEq, Eq, Hash, Lift)] pub struct MatchExpressionArmCause<'tcx> { + pub arm_block_id: Option, + pub arm_ty: Ty<'tcx>, pub arm_span: Span, + pub prior_arm_block_id: Option, + pub prior_arm_ty: Ty<'tcx>, + pub prior_arm_span: Span, pub scrut_span: Span, - pub semi_span: Option<(Span, StatementAsExpression)>, pub source: hir::MatchSource, pub prior_arms: Vec, - pub last_ty: Ty<'tcx>, pub scrut_hir_id: hir::HirId, pub opt_suggest_box_span: Option, } diff --git a/compiler/rustc_typeck/src/check/_match.rs b/compiler/rustc_typeck/src/check/_match.rs index 00547a6d8278f..2671f2f4f89ca 100644 --- a/compiler/rustc_typeck/src/check/_match.rs +++ b/compiler/rustc_typeck/src/check/_match.rs @@ -9,7 +9,6 @@ use rustc_span::Span; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt; use rustc_trait_selection::traits::{ IfExpressionCause, MatchExpressionArmCause, ObligationCause, ObligationCauseCode, - StatementAsExpression, }; impl<'a, 'tcx> FnCtxt<'a, 'tcx> { @@ -75,8 +74,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }; let mut other_arms = vec![]; // Used only for diagnostics. - let mut prior_arm_ty = None; - for (i, arm) in arms.iter().enumerate() { + let mut prior_arm = None; + for arm in arms { if let Some(g) = &arm.guard { self.diverges.set(Diverges::Maybe); match g { @@ -96,21 +95,28 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let opt_suggest_box_span = self.opt_suggest_box_span(arm_ty, orig_expected); - let (arm_span, semi_span) = - self.get_appropriate_arm_semicolon_removal_span(&arms, i, prior_arm_ty, arm_ty); - let (span, code) = match i { + let (arm_block_id, arm_span) = if let hir::ExprKind::Block(blk, _) = arm.body.kind { + (Some(blk.hir_id), self.find_block_span(blk)) + } else { + (None, arm.body.span) + }; + + let (span, code) = match prior_arm { // The reason for the first arm to fail is not that the match arms diverge, // but rather that there's a prior obligation that doesn't hold. - 0 => (arm_span, ObligationCauseCode::BlockTailExpression(arm.body.hir_id)), - _ => ( + None => (arm_span, ObligationCauseCode::BlockTailExpression(arm.body.hir_id)), + Some((prior_arm_block_id, prior_arm_ty, prior_arm_span)) => ( expr.span, ObligationCauseCode::MatchExpressionArm(Box::new(MatchExpressionArmCause { + arm_block_id, arm_span, + arm_ty, + prior_arm_block_id, + prior_arm_ty, + prior_arm_span, scrut_span: scrut.span, - semi_span, source: match_src, prior_arms: other_arms.clone(), - last_ty: prior_arm_ty.unwrap(), scrut_hir_id: scrut.hir_id, opt_suggest_box_span, })), @@ -139,7 +145,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let ret_ty = ret_coercion.borrow().expected_ty(); let ret_ty = self.inh.infcx.shallow_resolve(ret_ty); self.can_coerce(arm_ty, ret_ty) - && prior_arm_ty.map_or(true, |t| self.can_coerce(t, ret_ty)) + && prior_arm.map_or(true, |(_, t, _)| self.can_coerce(t, ret_ty)) // The match arms need to unify for the case of `impl Trait`. && !matches!(ret_ty.kind(), ty::Opaque(..)) } @@ -181,7 +187,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if other_arms.len() > 5 { other_arms.remove(0); } - prior_arm_ty = Some(arm_ty); + + prior_arm = Some((arm_block_id, arm_ty, arm_span)); } // If all of the arms in the `match` diverge, @@ -207,32 +214,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { match_ty } - fn get_appropriate_arm_semicolon_removal_span( - &self, - arms: &'tcx [hir::Arm<'tcx>], - i: usize, - prior_arm_ty: Option>, - arm_ty: Ty<'tcx>, - ) -> (Span, Option<(Span, StatementAsExpression)>) { - let arm = &arms[i]; - let (arm_span, mut semi_span) = if let hir::ExprKind::Block(blk, _) = &arm.body.kind { - ( - self.find_block_span(blk), - prior_arm_ty - .and_then(|prior_arm_ty| self.could_remove_semicolon(blk, prior_arm_ty)), - ) - } else { - (arm.body.span, None) - }; - if semi_span.is_none() && i > 0 { - if let hir::ExprKind::Block(blk, _) = &arms[i - 1].body.kind { - let semi_span_prev = self.could_remove_semicolon(blk, arm_ty); - semi_span = semi_span_prev; - } - } - (arm_span, semi_span) - } - /// When the previously checked expression (the scrutinee) diverges, /// warn the user about the match arms being unreachable. fn warn_arms_when_scrutinee_diverges(&self, arms: &'tcx [hir::Arm<'tcx>]) { diff --git a/src/test/ui/suggestions/return-bindings.rs b/src/test/ui/suggestions/return-bindings.rs index 80c83a70d50c1..fa1bad37699f3 100644 --- a/src/test/ui/suggestions/return-bindings.rs +++ b/src/test/ui/suggestions/return-bindings.rs @@ -24,4 +24,28 @@ fn d(opt_str: Option) { }; } +fn d2(opt_str: Option) { + let s = if let Some(s) = opt_str { + } else { + String::new() + //~^ ERROR `if` and `else` have incompatible types + }; +} + +fn e(opt_str: Option) { + let s: String = match opt_str { + Some(s) => {} + //~^ ERROR mismatched types + None => String::new(), + }; +} + +fn e2(opt_str: Option) { + let s = match opt_str { + Some(s) => {} + None => String::new(), + //~^ ERROR `match` arms have incompatible types + }; +} + fn main() {} diff --git a/src/test/ui/suggestions/return-bindings.stderr b/src/test/ui/suggestions/return-bindings.stderr index 53ef7106fa808..c14fb336773d1 100644 --- a/src/test/ui/suggestions/return-bindings.stderr +++ b/src/test/ui/suggestions/return-bindings.stderr @@ -59,6 +59,52 @@ LL + s LL ~ | -error: aborting due to 4 previous errors +error[E0308]: `if` and `else` have incompatible types + --> $DIR/return-bindings.rs:30:9 + | +LL | let s = if let Some(s) = opt_str { + | ______________________________________- +LL | | } else { + | |_____- expected because of this +LL | String::new() + | ^^^^^^^^^^^^^ expected `()`, found struct `String` + | +help: consider returning the local binding `s` + | +LL ~ let s = if let Some(s) = opt_str { +LL + s +LL ~ } else { + | + +error[E0308]: mismatched types + --> $DIR/return-bindings.rs:37:20 + | +LL | Some(s) => {} + | ^^ expected struct `String`, found `()` + | +help: consider returning the local binding `s` + | +LL | Some(s) => { s } + | + + +error[E0308]: `match` arms have incompatible types + --> $DIR/return-bindings.rs:46:17 + | +LL | let s = match opt_str { + | _____________- +LL | | Some(s) => {} + | | -- this is found to be of type `()` +LL | | None => String::new(), + | | ^^^^^^^^^^^^^ expected `()`, found struct `String` +LL | | +LL | | }; + | |_____- `match` arms have incompatible types + | +help: consider returning the local binding `s` + | +LL | Some(s) => { s } + | + + +error: aborting due to 7 previous errors For more information about this error, try `rustc --explain E0308`. From cd89978d869587fc025286b0326116819d5b78e4 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 21 Jul 2022 15:45:35 +0000 Subject: [PATCH 09/13] Generalize same_type_modulo_infer --- .../src/infer/error_reporting/mod.rs | 37 +++++++++++++++++-- src/test/ui/issues/issue-59494.stderr | 2 - 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs index 5c1ab2bf5888d..93f94bd497a55 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/mod.rs @@ -2670,8 +2670,18 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { /// Float types, respectively). When comparing two ADTs, these rules apply recursively. pub fn same_type_modulo_infer(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> bool { let (a, b) = self.resolve_vars_if_possible((a, b)); - match (&a.kind(), &b.kind()) { - (&ty::Adt(did_a, substs_a), &ty::Adt(did_b, substs_b)) => { + match (a.kind(), b.kind()) { + (&ty::Adt(def_a, substs_a), &ty::Adt(def_b, substs_b)) => { + if def_a != def_b { + return false; + } + + substs_a + .types() + .zip(substs_b.types()) + .all(|(a, b)| self.same_type_modulo_infer(a, b)) + } + (&ty::FnDef(did_a, substs_a), &ty::FnDef(did_b, substs_b)) => { if did_a != did_b { return false; } @@ -2694,7 +2704,28 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { | (&ty::Infer(ty::InferTy::TyVar(_)), _) | (_, &ty::Infer(ty::InferTy::TyVar(_))) => true, (&ty::Ref(_, ty_a, mut_a), &ty::Ref(_, ty_b, mut_b)) => { - mut_a == mut_b && self.same_type_modulo_infer(*ty_a, *ty_b) + mut_a == mut_b && self.same_type_modulo_infer(ty_a, ty_b) + } + (&ty::RawPtr(a), &ty::RawPtr(b)) => { + a.mutbl == b.mutbl && self.same_type_modulo_infer(a.ty, b.ty) + } + (&ty::Slice(a), &ty::Slice(b)) => self.same_type_modulo_infer(a, b), + (&ty::Array(a_ty, a_ct), &ty::Array(b_ty, b_ct)) => { + self.same_type_modulo_infer(a_ty, b_ty) && a_ct == b_ct + } + (&ty::Tuple(a), &ty::Tuple(b)) => { + if a.len() != b.len() { + return false; + } + std::iter::zip(a.iter(), b.iter()).all(|(a, b)| self.same_type_modulo_infer(a, b)) + } + (&ty::FnPtr(a), &ty::FnPtr(b)) => { + let a = a.skip_binder().inputs_and_output; + let b = b.skip_binder().inputs_and_output; + if a.len() != b.len() { + return false; + } + std::iter::zip(a.iter(), b.iter()).all(|(a, b)| self.same_type_modulo_infer(a, b)) } // FIXME(compiler-errors): This needs to be generalized more _ => a == b, diff --git a/src/test/ui/issues/issue-59494.stderr b/src/test/ui/issues/issue-59494.stderr index 8b542bb69de2e..a9284535e4dc4 100644 --- a/src/test/ui/issues/issue-59494.stderr +++ b/src/test/ui/issues/issue-59494.stderr @@ -7,8 +7,6 @@ LL | let t8 = t8n(t7, t7p(f, g)); | required by a bound introduced by this call | = help: the trait `Fn<(_,)>` is not implemented for `impl Fn(((_, _), _))` - = note: expected a closure with arguments `(((_, _), _),)` - found a closure with arguments `(_,)` note: required by a bound in `t8n` --> $DIR/issue-59494.rs:5:45 | From 947314125370164f52feb221ef2f1aac0e420309 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Thu, 21 Jul 2022 11:51:40 -0400 Subject: [PATCH 10/13] Update compiler/rustc_mir_transform/src/simplify.rs --- compiler/rustc_mir_transform/src/simplify.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_mir_transform/src/simplify.rs b/compiler/rustc_mir_transform/src/simplify.rs index 59c38b87b5cb2..d305960b4856d 100644 --- a/compiler/rustc_mir_transform/src/simplify.rs +++ b/compiler/rustc_mir_transform/src/simplify.rs @@ -28,7 +28,7 @@ //! return. use crate::MirPass; -use rustc_data_structures::stable_set::FxHashSet; +use rustc_data_structures::fx::FxHashSet; use rustc_index::vec::{Idx, IndexVec}; use rustc_middle::mir::coverage::*; use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor}; From eb5acc9b9b520092cc667a810a18498b28d052ab Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Thu, 16 Jun 2022 19:47:24 +0200 Subject: [PATCH 11/13] Rename `<*{mut,const} T>::as_{const,mut}` to `cast_` This renames the methods to use the `cast_` prefix instead of `as_` to make it more readable and avoid confusion with `<*mut T>::as_mut()` which is `unsafe` and returns a reference. See #92675 --- library/core/src/ptr/const_ptr.rs | 2 +- library/core/src/ptr/mut_ptr.rs | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/library/core/src/ptr/const_ptr.rs b/library/core/src/ptr/const_ptr.rs index 74aa0d9c7bcb2..55f781ce0222a 100644 --- a/library/core/src/ptr/const_ptr.rs +++ b/library/core/src/ptr/const_ptr.rs @@ -97,7 +97,7 @@ impl *const T { /// refactored. #[unstable(feature = "ptr_const_cast", issue = "92675")] #[rustc_const_unstable(feature = "ptr_const_cast", issue = "92675")] - pub const fn as_mut(self) -> *mut T { + pub const fn cast_mut(self) -> *mut T { self as _ } diff --git a/library/core/src/ptr/mut_ptr.rs b/library/core/src/ptr/mut_ptr.rs index b988090f4bc4c..27e8b20b3c52d 100644 --- a/library/core/src/ptr/mut_ptr.rs +++ b/library/core/src/ptr/mut_ptr.rs @@ -96,11 +96,13 @@ impl *mut T { /// refactored. /// /// While not strictly required (`*mut T` coerces to `*const T`), this is provided for symmetry - /// with `as_mut()` on `*const T` and may have documentation value if used instead of implicit + /// with [`cast_mut`] on `*const T` and may have documentation value if used instead of implicit /// coercion. + /// + /// [`cast_mut`]: #method.cast_mut #[unstable(feature = "ptr_const_cast", issue = "92675")] #[rustc_const_unstable(feature = "ptr_const_cast", issue = "92675")] - pub const fn as_const(self) -> *const T { + pub const fn cast_const(self) -> *const T { self as _ } @@ -289,7 +291,7 @@ impl *mut T { /// For the mutable counterpart see [`as_mut`]. /// /// [`as_uninit_ref`]: #method.as_uninit_ref-1 - /// [`as_mut`]: #method.as_mut-1 + /// [`as_mut`]: #method.as_mut /// /// # Safety /// From 3eef023da04cd732ee6656867e2161b7dbfee821 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 21 Jul 2022 16:12:57 +0000 Subject: [PATCH 12/13] Address more nits --- compiler/rustc_hir/src/hir.rs | 2 +- .../src/infer/error_reporting/mod.rs | 68 +++++++++---------- compiler/rustc_typeck/src/check/_match.rs | 4 +- 3 files changed, 34 insertions(+), 40 deletions(-) diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index d6e183dd5a3bb..18ffc227fed86 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -955,7 +955,7 @@ pub struct Block<'hir> { } impl<'hir> Block<'hir> { - pub fn peel_blocks(&self) -> &Block<'hir> { + pub fn innermost_block(&self) -> &Block<'hir> { let mut block = self; while let Some(Expr { kind: ExprKind::Block(inner_block, _), .. }) = block.expr { block = inner_block; diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs index 93f94bd497a55..4e87ec86658f8 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/mod.rs @@ -758,22 +758,13 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { second_ty: Ty<'tcx>, second_span: Span, ) { - let semicolon = - if let Some(first_id) = first_id - && let hir::Node::Block(blk) = self.tcx.hir().get(first_id) - && let Some(remove_semicolon) = self.could_remove_semicolon(blk, second_ty) - { - Some(remove_semicolon) - } else if let Some(second_id) = second_id - && let hir::Node::Block(blk) = self.tcx.hir().get(second_id) - && let Some(remove_semicolon) = self.could_remove_semicolon(blk, first_ty) - { - Some(remove_semicolon) - } else { - None - }; - if let Some((sp, boxed)) = semicolon { - if matches!(boxed, StatementAsExpression::NeedsBoxing) { + let remove_semicolon = + [(first_id, second_ty), (second_id, first_ty)].into_iter().find_map(|(id, ty)| { + let hir::Node::Block(blk) = self.tcx.hir().get(id?) else { return None }; + self.could_remove_semicolon(blk, ty) + }); + match remove_semicolon { + Some((sp, StatementAsExpression::NeedsBoxing)) => { err.multipart_suggestion( "consider removing this semicolon and boxing the expressions", vec![ @@ -785,7 +776,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { ], Applicability::MachineApplicable, ); - } else { + } + Some((sp, StatementAsExpression::CorrectType)) => { err.span_suggestion_short( sp, "consider removing this semicolon", @@ -793,20 +785,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { Applicability::MachineApplicable, ); } - } else { - let suggested = - if let Some(first_id) = first_id - && let hir::Node::Block(blk) = self.tcx.hir().get(first_id) - { - self.consider_returning_binding(blk, second_ty, err) - } else { - false - }; - if !suggested - && let Some(second_id) = second_id - && let hir::Node::Block(blk) = self.tcx.hir().get(second_id) - { - self.consider_returning_binding(blk, first_ty, err); + None => { + for (id, ty) in [(first_id, second_ty), (second_id, first_ty)] { + if let Some(id) = id + && let hir::Node::Block(blk) = self.tcx.hir().get(id) + && self.consider_returning_binding(blk, ty, err) + { + break; + } + } } } } @@ -2884,8 +2871,10 @@ impl TyCategory { } impl<'tcx> InferCtxt<'_, 'tcx> { + /// Given a [`hir::Block`], get the span of its last expression or + /// statement, peeling off any inner blocks. pub fn find_block_span(&self, block: &'tcx hir::Block<'tcx>) -> Span { - let block = block.peel_blocks(); + let block = block.innermost_block(); if let Some(expr) = &block.expr { expr.span } else if let Some(stmt) = block.stmts.last() { @@ -2897,27 +2886,30 @@ impl<'tcx> InferCtxt<'_, 'tcx> { } } + /// Given a [`hir::HirId`] for a block, get the span of its last expression + /// or statement, peeling off any inner blocks. pub fn find_block_span_from_hir_id(&self, hir_id: hir::HirId) -> Span { match self.tcx.hir().get(hir_id) { hir::Node::Block(blk) => self.find_block_span(blk), - // The parser was in a weird state if either of these happen... + // The parser was in a weird state if either of these happen, but + // it's better not to panic. hir::Node::Expr(e) => e.span, _ => rustc_span::DUMMY_SP, } } + /// Be helpful when the user wrote `{... expr; }` and taking the `;` off + /// is enough to fix the error. pub fn could_remove_semicolon( &self, blk: &'tcx hir::Block<'tcx>, expected_ty: Ty<'tcx>, ) -> Option<(Span, StatementAsExpression)> { - let blk = blk.peel_blocks(); + let blk = blk.innermost_block(); // Do not suggest if we have a tail expr. if blk.expr.is_some() { return None; } - // Be helpful when the user wrote `{... expr;}` and - // taking the `;` off is enough to fix the error. let last_stmt = blk.stmts.last()?; let hir::StmtKind::Semi(ref last_expr) = last_stmt.kind else { return None; @@ -2987,13 +2979,15 @@ impl<'tcx> InferCtxt<'_, 'tcx> { Some((span, needs_box)) } + /// Suggest returning a local binding with a compatible type if the block + /// has no return expression. pub fn consider_returning_binding( &self, blk: &'tcx hir::Block<'tcx>, expected_ty: Ty<'tcx>, err: &mut Diagnostic, ) -> bool { - let blk = blk.peel_blocks(); + let blk = blk.innermost_block(); // Do not suggest if we have a tail expr. if blk.expr.is_some() { return false; diff --git a/compiler/rustc_typeck/src/check/_match.rs b/compiler/rustc_typeck/src/check/_match.rs index 2671f2f4f89ca..f629f6a0099d7 100644 --- a/compiler/rustc_typeck/src/check/_match.rs +++ b/compiler/rustc_typeck/src/check/_match.rs @@ -325,7 +325,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }; let (error_sp, else_id) = if let ExprKind::Block(block, _) = &else_expr.kind { - let block = block.peel_blocks(); + let block = block.innermost_block(); // Avoid overlapping spans that aren't as readable: // ``` @@ -366,7 +366,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }; let then_id = if let ExprKind::Block(block, _) = &then_expr.kind { - let block = block.peel_blocks(); + let block = block.innermost_block(); // Exclude overlapping spans if block.expr.is_none() && block.stmts.is_empty() { outer_span = None; From 7ba0be832a0f9c7a22f4b9e2cb76b653d4010b30 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 21 Jul 2022 18:15:24 -0600 Subject: [PATCH 13/13] add same warning to Result::expect as Result::unwrap --- library/core/src/result.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/library/core/src/result.rs b/library/core/src/result.rs index 8a68cdf7d651b..45b052c824d31 100644 --- a/library/core/src/result.rs +++ b/library/core/src/result.rs @@ -1009,6 +1009,15 @@ impl Result { /// Returns the contained [`Ok`] value, consuming the `self` value. /// + /// Because this function may panic, its use is generally discouraged. + /// Instead, prefer to use pattern matching and handle the [`Err`] + /// case explicitly, or call [`unwrap_or`], [`unwrap_or_else`], or + /// [`unwrap_or_default`]. + /// + /// [`unwrap_or`]: Result::unwrap_or + /// [`unwrap_or_else`]: Result::unwrap_or_else + /// [`unwrap_or_default`]: Result::unwrap_or_default + /// /// # Panics /// /// Panics if the value is an [`Err`], with a panic message including the