Skip to content

Commit 69c65d2

Browse files
borsbors
committed
Auto merge of #42850 - estebank:unwanted-return-rotj, r=nikomatsakis
Detect missing `;` on methods with return type `()` - Point out the origin of a type requirement when it is the return type of a method - Point out possibly missing semicolon when the return type is `()` and the implicit return makes sense as a statement - Suggest changing the return type of methods with default return type - Don't suggest changing the return type on `fn main()` - Don't suggest changing the return type on impl fn - Suggest removal of semicolon (instead of being help)
2 parents 47faf1d + 7dad295 commit 69c65d2

38 files changed

+549
-77
lines changed

‎src/librustc/hir/map/mod.rs

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -594,8 +594,12 @@ impl<'hir> Map<'hir> {
594594
/// last good node id we found. Note that reaching the crate root (id == 0),
595595
/// is not an error, since items in the crate module have the crate root as
596596
/// parent.
597-
fn walk_parent_nodes<F>(&self, start_id: NodeId, found: F) -> Result<NodeId, NodeId>
598-
where F: Fn(&Node<'hir>) -> bool
597+
fn walk_parent_nodes<F, F2>(&self,
598+
start_id: NodeId,
599+
found: F,
600+
bail_early: F2)
601+
-> Result<NodeId, NodeId>
602+
where F: Fn(&Node<'hir>) -> bool, F2: Fn(&Node<'hir>) -> bool
599603
{
600604
let mut id = start_id;
601605
loop {
@@ -616,6 +620,8 @@ impl<'hir> Map<'hir> {
616620
Some(ref node) => {
617621
if found(node) {
618622
return Ok(parent_node);
623+
} else if bail_early(node) {
624+
return Err(parent_node);
619625
}
620626
}
621627
None => {
@@ -626,6 +632,56 @@ impl<'hir> Map<'hir> {
626632
}
627633
}
628634

635+
/// Retrieve the NodeId for `id`'s enclosing method, unless there's a
636+
/// `while` or `loop` before reacing it, as block tail returns are not
637+
/// available in them.
638+
///
639+
/// ```
640+
/// fn foo(x: usize) -> bool {
641+
/// if x == 1 {
642+
/// true // `get_return_block` gets passed the `id` corresponding
643+
/// } else { // to this, it will return `foo`'s `NodeId`.
644+
/// false
645+
/// }
646+
/// }
647+
/// ```
648+
///
649+
/// ```
650+
/// fn foo(x: usize) -> bool {
651+
/// loop {
652+
/// true // `get_return_block` gets passed the `id` corresponding
653+
/// } // to this, it will return `None`.
654+
/// false
655+
/// }
656+
/// ```
657+
pub fn get_return_block(&self, id: NodeId) -> Option<NodeId> {
658+
let match_fn = |node: &Node| {
659+
match *node {
660+
NodeItem(_) |
661+
NodeForeignItem(_) |
662+
NodeTraitItem(_) |
663+
NodeImplItem(_) => true,
664+
_ => false,
665+
}
666+
};
667+
let match_non_returning_block = |node: &Node| {
668+
match *node {
669+
NodeExpr(ref expr) => {
670+
match expr.node {
671+
ExprWhile(..) | ExprLoop(..) => true,
672+
_ => false,
673+
}
674+
}
675+
_ => false,
676+
}
677+
};
678+
679+
match self.walk_parent_nodes(id, match_fn, match_non_returning_block) {
680+
Ok(id) => Some(id),
681+
Err(_) => None,
682+
}
683+
}
684+
629685
/// Retrieve the NodeId for `id`'s parent item, or `id` itself if no
630686
/// parent item is in this map. The "parent item" is the closest parent node
631687
/// in the AST which is recorded by the map and is an item, either an item
@@ -637,7 +693,7 @@ impl<'hir> Map<'hir> {
637693
NodeTraitItem(_) |
638694
NodeImplItem(_) => true,
639695
_ => false,
640-
}) {
696+
}, |_| false) {
641697
Ok(id) => id,
642698
Err(id) => id,
643699
}
@@ -649,7 +705,7 @@ impl<'hir> Map<'hir> {
649705
let id = match self.walk_parent_nodes(id, |node| match *node {
650706
NodeItem(&Item { node: Item_::ItemMod(_), .. }) => true,
651707
_ => false,
652-
}) {
708+
}, |_| false) {
653709
Ok(id) => id,
654710
Err(id) => id,
655711
};
@@ -668,7 +724,7 @@ impl<'hir> Map<'hir> {
668724
NodeImplItem(_) |
669725
NodeBlock(_) => true,
670726
_ => false,
671-
}) {
727+
}, |_| false) {
672728
Ok(id) => Some(id),
673729
Err(_) => None,
674730
}

‎src/librustc/traits/error_reporting.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1088,7 +1088,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
10881088
ObligationCauseCode::VariableType(_) => {
10891089
err.note("all local variables must have a statically known size");
10901090
}
1091-
ObligationCauseCode::ReturnType => {
1091+
ObligationCauseCode::SizedReturnType => {
10921092
err.note("the return type of a function must have a \
10931093
statically known size");
10941094
}
@@ -1133,6 +1133,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
11331133
but not on the corresponding trait method",
11341134
predicate));
11351135
}
1136+
ObligationCauseCode::ReturnType(_) |
1137+
ObligationCauseCode::BlockTailExpression(_) => (),
11361138
}
11371139
}
11381140

‎src/librustc/traits/mod.rs

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -118,65 +118,76 @@ pub enum ObligationCauseCode<'tcx> {
118118
/// Obligation incurred due to an object cast.
119119
ObjectCastObligation(/* Object type */ Ty<'tcx>),
120120

121-
/// Various cases where expressions must be sized/copy/etc:
122-
AssignmentLhsSized, // L = X implies that L is Sized
123-
StructInitializerSized, // S { ... } must be Sized
124-
VariableType(ast::NodeId), // Type of each variable must be Sized
125-
ReturnType, // Return type must be Sized
126-
RepeatVec, // [T,..n] --> T must be Copy
127-
128-
// Types of fields (other than the last) in a struct must be sized.
121+
// Various cases where expressions must be sized/copy/etc:
122+
/// L = X implies that L is Sized
123+
AssignmentLhsSized,
124+
/// S { ... } must be Sized
125+
StructInitializerSized,
126+
/// Type of each variable must be Sized
127+
VariableType(ast::NodeId),
128+
/// Return type must be Sized
129+
SizedReturnType,
130+
/// [T,..n] --> T must be Copy
131+
RepeatVec,
132+
133+
/// Types of fields (other than the last) in a struct must be sized.
129134
FieldSized,
130135

131-
// Constant expressions must be sized.
136+
/// Constant expressions must be sized.
132137
ConstSized,
133138

134-
// static items must have `Sync` type
139+
/// static items must have `Sync` type
135140
SharedStatic,
136141

137142
BuiltinDerivedObligation(DerivedObligationCause<'tcx>),
138143

139144
ImplDerivedObligation(DerivedObligationCause<'tcx>),
140145

141-
// error derived when matching traits/impls; see ObligationCause for more details
146+
/// error derived when matching traits/impls; see ObligationCause for more details
142147
CompareImplMethodObligation {
143148
item_name: ast::Name,
144149
impl_item_def_id: DefId,
145150
trait_item_def_id: DefId,
146151
lint_id: Option<ast::NodeId>,
147152
},
148153

149-
// Checking that this expression can be assigned where it needs to be
154+
/// Checking that this expression can be assigned where it needs to be
150155
// FIXME(eddyb) #11161 is the original Expr required?
151156
ExprAssignable,
152157

153-
// Computing common supertype in the arms of a match expression
158+
/// Computing common supertype in the arms of a match expression
154159
MatchExpressionArm { arm_span: Span,
155160
source: hir::MatchSource },
156161

157-
// Computing common supertype in an if expression
162+
/// Computing common supertype in an if expression
158163
IfExpression,
159164

160-
// Computing common supertype of an if expression with no else counter-part
165+
/// Computing common supertype of an if expression with no else counter-part
161166
IfExpressionWithNoElse,
162167

163-
// `where a == b`
168+
/// `where a == b`
164169
EquatePredicate,
165170

166-
// `main` has wrong type
171+
/// `main` has wrong type
167172
MainFunctionType,
168173

169-
// `start` has wrong type
174+
/// `start` has wrong type
170175
StartFunctionType,
171176

172-
// intrinsic has wrong type
177+
/// intrinsic has wrong type
173178
IntrinsicType,
174179

175-
// method receiver
180+
/// method receiver
176181
MethodReceiver,
177182

178-
// `return` with no expression
183+
/// `return` with no expression
179184
ReturnNoExpression,
185+
186+
/// `return` with an expression
187+
ReturnType(ast::NodeId),
188+
189+
/// Block implicit return
190+
BlockTailExpression(ast::NodeId),
180191
}
181192

182193
#[derive(Clone, Debug, PartialEq, Eq)]

‎src/librustc/traits/structural_impls.rs

Lines changed: 17 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,8 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
191191
super::AssignmentLhsSized => Some(super::AssignmentLhsSized),
192192
super::StructInitializerSized => Some(super::StructInitializerSized),
193193
super::VariableType(id) => Some(super::VariableType(id)),
194-
super::ReturnType => Some(super::ReturnType),
194+
super::ReturnType(id) => Some(super::ReturnType(id)),
195+
super::SizedReturnType => Some(super::SizedReturnType),
195196
super::RepeatVec => Some(super::RepeatVec),
196197
super::FieldSized => Some(super::FieldSized),
197198
super::ConstSized => Some(super::ConstSized),
@@ -213,34 +214,19 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
213214
lint_id: lint_id,
214215
})
215216
}
216-
super::ExprAssignable => {
217-
Some(super::ExprAssignable)
218-
}
217+
super::ExprAssignable => Some(super::ExprAssignable),
219218
super::MatchExpressionArm { arm_span, source } => {
220219
Some(super::MatchExpressionArm { arm_span: arm_span,
221220
source: source })
222221
}
223-
super::IfExpression => {
224-
Some(super::IfExpression)
225-
}
226-
super::IfExpressionWithNoElse => {
227-
Some(super::IfExpressionWithNoElse)
228-
}
229-
super::EquatePredicate => {
230-
Some(super::EquatePredicate)
231-
}
232-
super::MainFunctionType => {
233-
Some(super::MainFunctionType)
234-
}
235-
super::StartFunctionType => {
236-
Some(super::StartFunctionType)
237-
}
238-
super::IntrinsicType => {
239-
Some(super::IntrinsicType)
240-
}
241-
super::MethodReceiver => {
242-
Some(super::MethodReceiver)
243-
}
222+
super::IfExpression => Some(super::IfExpression),
223+
super::IfExpressionWithNoElse => Some(super::IfExpressionWithNoElse),
224+
super::EquatePredicate => Some(super::EquatePredicate),
225+
super::MainFunctionType => Some(super::MainFunctionType),
226+
super::StartFunctionType => Some(super::StartFunctionType),
227+
super::IntrinsicType => Some(super::IntrinsicType),
228+
super::MethodReceiver => Some(super::MethodReceiver),
229+
super::BlockTailExpression(id) => Some(super::BlockTailExpression(id)),
244230
}
245231
}
246232
}
@@ -492,12 +478,14 @@ impl<'tcx> TypeFoldable<'tcx> for traits::ObligationCauseCode<'tcx> {
492478
super::AssignmentLhsSized |
493479
super::StructInitializerSized |
494480
super::VariableType(_) |
495-
super::ReturnType |
481+
super::ReturnType(_) |
482+
super::SizedReturnType |
496483
super::ReturnNoExpression |
497484
super::RepeatVec |
498485
super::FieldSized |
499486
super::ConstSized |
500487
super::SharedStatic |
488+
super::BlockTailExpression(_) |
501489
super::CompareImplMethodObligation { .. } => self.clone(),
502490

503491
super::ProjectionWf(proj) => super::ProjectionWf(proj.fold_with(folder)),
@@ -537,12 +525,14 @@ impl<'tcx> TypeFoldable<'tcx> for traits::ObligationCauseCode<'tcx> {
537525
super::AssignmentLhsSized |
538526
super::StructInitializerSized |
539527
super::VariableType(_) |
540-
super::ReturnType |
528+
super::ReturnType(_) |
529+
super::SizedReturnType |
541530
super::ReturnNoExpression |
542531
super::RepeatVec |
543532
super::FieldSized |
544533
super::ConstSized |
545534
super::SharedStatic |
535+
super::BlockTailExpression(_) |
546536
super::CompareImplMethodObligation { .. } => false,
547537

548538
super::ProjectionWf(proj) => proj.visit_with(visitor),

‎src/librustc/ty/mod.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,18 @@ impl<'tcx> TyS<'tcx> {
481481
_ => false,
482482
}
483483
}
484+
485+
pub fn is_suggestable(&self) -> bool {
486+
match self.sty {
487+
TypeVariants::TyAnon(..) |
488+
TypeVariants::TyFnDef(..) |
489+
TypeVariants::TyFnPtr(..) |
490+
TypeVariants::TyDynamic(..) |
491+
TypeVariants::TyClosure(..) |
492+
TypeVariants::TyProjection(..) => false,
493+
_ => true,
494+
}
495+
}
484496
}
485497

486498
impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for ty::TyS<'tcx> {

‎src/librustc_errors/emitter.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,12 @@ impl Emitter for EmitterWriter {
4747
// don't display multiline suggestions as labels
4848
sugg.substitution_parts[0].substitutions[0].find('\n').is_none() {
4949
let substitution = &sugg.substitution_parts[0].substitutions[0];
50-
let msg = format!("help: {} `{}`", sugg.msg, substitution);
50+
let msg = if substitution.len() == 0 {
51+
// This substitution is only removal, don't show it
52+
format!("help: {}", sugg.msg)
53+
} else {
54+
format!("help: {} `{}`", sugg.msg, substitution)
55+
};
5156
primary_span.push_span_label(sugg.substitution_spans().next().unwrap(), msg);
5257
} else {
5358
// if there are multiple suggestions, print them all in full

‎src/librustc_typeck/check/coercion.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1168,6 +1168,18 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E>
11681168
"`return;` in a function whose return type is not `()`");
11691169
db.span_label(cause.span, "return type is not ()");
11701170
}
1171+
ObligationCauseCode::BlockTailExpression(blk_id) => {
1172+
db = fcx.report_mismatched_types(cause, expected, found, err);
1173+
1174+
let expr = expression.unwrap_or_else(|| {
1175+
span_bug!(cause.span,
1176+
"supposed to be part of a block tail expression, but the \
1177+
expression is empty");
1178+
});
1179+
fcx.suggest_mismatched_types_on_tail(&mut db, expr,
1180+
expected, found,
1181+
cause.span, blk_id);
1182+
}
11711183
_ => {
11721184
db = fcx.report_mismatched_types(cause, expected, found, err);
11731185
}

‎src/librustc_typeck/check/demand.rs

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -73,15 +73,21 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
7373
}
7474
}
7575

76+
pub fn demand_coerce(&self, expr: &hir::Expr, checked_ty: Ty<'tcx>, expected: Ty<'tcx>) {
77+
if let Some(mut err) = self.demand_coerce_diag(expr, checked_ty, expected) {
78+
err.emit();
79+
}
80+
}
81+
7682
// Checks that the type of `expr` can be coerced to `expected`.
7783
//
7884
// NB: This code relies on `self.diverges` to be accurate. In
7985
// particular, assignments to `!` will be permitted if the
8086
// diverges flag is currently "always".
81-
pub fn demand_coerce(&self,
82-
expr: &hir::Expr,
83-
checked_ty: Ty<'tcx>,
84-
expected: Ty<'tcx>) {
87+
pub fn demand_coerce_diag(&self,
88+
expr: &hir::Expr,
89+
checked_ty: Ty<'tcx>,
90+
expected: Ty<'tcx>) -> Option<DiagnosticBuilder<'tcx>> {
8591
let expected = self.resolve_type_vars_with_obligations(expected);
8692

8793
if let Err(e) = self.try_coerce(expr, checked_ty, self.diverges.get(), expected) {
@@ -105,8 +111,9 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
105111
self.get_best_match(&suggestions).join("\n")));
106112
}
107113
}
108-
err.emit();
114+
return Some(err);
109115
}
116+
None
110117
}
111118

112119
fn format_method_suggestion(&self, method: &AssociatedItem) -> String {

0 commit comments

Comments
 (0)