Skip to content

Commit 73a3a90

Browse files
committed
Auto merge of #60920 - Manishearth:rollup-p4xp4gk, r=Manishearth
Rollup of 4 pull requests Successful merges: - #60791 (Update books) - #60891 (Allow claiming issues with triagebot) - #60901 (Handle more string addition cases with appropriate suggestions) - #60902 (Prevent Error::type_id overrides) Failed merges: r? @ghost
2 parents 823a75d + f48f37b commit 73a3a90

File tree

17 files changed

+253
-46
lines changed

17 files changed

+253
-46
lines changed

src/ci/docker/x86_64-gnu-tools/checktools.sh

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,9 @@ status_check() {
7474
check_dispatch $1 beta nomicon src/doc/nomicon
7575
check_dispatch $1 beta reference src/doc/reference
7676
check_dispatch $1 beta rust-by-example src/doc/rust-by-example
77-
check_dispatch $1 beta edition-guide src/doc/edition-guide
77+
# Temporarily disabled until
78+
# https://github.com/rust-lang/rust/issues/60459 is fixed.
79+
# check_dispatch $1 beta edition-guide src/doc/edition-guide
7880
check_dispatch $1 beta rls src/tools/rls
7981
check_dispatch $1 beta rustfmt src/tools/rustfmt
8082
check_dispatch $1 beta clippy-driver src/tools/clippy

src/doc/edition-guide

src/doc/nomicon

src/doc/reference

src/doc/rustc-guide

src/librustc_typeck/check/op.rs

Lines changed: 60 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -305,8 +305,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
305305
};
306306
if let Some(missing_trait) = missing_trait {
307307
if op.node == hir::BinOpKind::Add &&
308-
self.check_str_addition(expr, lhs_expr, rhs_expr, lhs_ty,
309-
rhs_ty, &mut err, true, op) {
308+
self.check_str_addition(
309+
lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, true, op) {
310310
// This has nothing here because it means we did string
311311
// concatenation (e.g., "Hello " += "World!"). This means
312312
// we don't want the note in the else clause to be emitted
@@ -400,8 +400,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
400400
};
401401
if let Some(missing_trait) = missing_trait {
402402
if op.node == hir::BinOpKind::Add &&
403-
self.check_str_addition(expr, lhs_expr, rhs_expr, lhs_ty,
404-
rhs_ty, &mut err, false, op) {
403+
self.check_str_addition(
404+
lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, false, op) {
405405
// This has nothing here because it means we did string
406406
// concatenation (e.g., "Hello " + "World!"). This means
407407
// we don't want the note in the else clause to be emitted
@@ -502,9 +502,13 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
502502
false
503503
}
504504

505+
/// Provide actionable suggestions when trying to add two strings with incorrect types,
506+
/// like `&str + &str`, `String + String` and `&str + &String`.
507+
///
508+
/// If this function returns `true` it means a note was printed, so we don't need
509+
/// to print the normal "implementation of `std::ops::Add` might be missing" note
505510
fn check_str_addition(
506511
&self,
507-
expr: &'gcx hir::Expr,
508512
lhs_expr: &'gcx hir::Expr,
509513
rhs_expr: &'gcx hir::Expr,
510514
lhs_ty: Ty<'tcx>,
@@ -514,45 +518,78 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
514518
op: hir::BinOp,
515519
) -> bool {
516520
let source_map = self.tcx.sess.source_map();
521+
let remove_borrow_msg = "String concatenation appends the string on the right to the \
522+
string on the left and may require reallocation. This \
523+
requires ownership of the string on the left";
524+
517525
let msg = "`to_owned()` can be used to create an owned `String` \
518526
from a string reference. String concatenation \
519527
appends the string on the right to the string \
520528
on the left and may require reallocation. This \
521529
requires ownership of the string on the left";
522-
// If this function returns true it means a note was printed, so we don't need
523-
// to print the normal "implementation of `std::ops::Add` might be missing" note
530+
531+
let is_std_string = |ty| &format!("{:?}", ty) == "std::string::String";
532+
524533
match (&lhs_ty.sty, &rhs_ty.sty) {
525-
(&Ref(_, l_ty, _), &Ref(_, r_ty, _))
526-
if l_ty.sty == Str && r_ty.sty == Str => {
527-
if !is_assign {
528-
err.span_label(op.span,
529-
"`+` can't be used to concatenate two `&str` strings");
534+
(&Ref(_, l_ty, _), &Ref(_, r_ty, _)) // &str or &String + &str, &String or &&str
535+
if (l_ty.sty == Str || is_std_string(l_ty)) && (
536+
r_ty.sty == Str || is_std_string(r_ty) ||
537+
&format!("{:?}", rhs_ty) == "&&str"
538+
) =>
539+
{
540+
if !is_assign { // Do not supply this message if `&str += &str`
541+
err.span_label(
542+
op.span,
543+
"`+` cannot be used to concatenate two `&str` strings",
544+
);
530545
match source_map.span_to_snippet(lhs_expr.span) {
531-
Ok(lstring) => err.span_suggestion(
532-
lhs_expr.span,
533-
msg,
534-
format!("{}.to_owned()", lstring),
535-
Applicability::MachineApplicable,
536-
),
546+
Ok(lstring) => {
547+
err.span_suggestion(
548+
lhs_expr.span,
549+
if lstring.starts_with("&") {
550+
remove_borrow_msg
551+
} else {
552+
msg
553+
},
554+
if lstring.starts_with("&") {
555+
// let a = String::new();
556+
// let _ = &a + "bar";
557+
format!("{}", &lstring[1..])
558+
} else {
559+
format!("{}.to_owned()", lstring)
560+
},
561+
Applicability::MachineApplicable,
562+
)
563+
}
537564
_ => err.help(msg),
538565
};
539566
}
540567
true
541568
}
542-
(&Ref(_, l_ty, _), &Adt(..))
543-
if l_ty.sty == Str && &format!("{:?}", rhs_ty) == "std::string::String" => {
544-
err.span_label(expr.span,
545-
"`+` can't be used to concatenate a `&str` with a `String`");
569+
(&Ref(_, l_ty, _), &Adt(..)) // Handle `&str` & `&String` + `String`
570+
if (l_ty.sty == Str || is_std_string(l_ty)) && is_std_string(rhs_ty) =>
571+
{
572+
err.span_label(
573+
op.span,
574+
"`+` cannot be used to concatenate a `&str` with a `String`",
575+
);
546576
match (
547577
source_map.span_to_snippet(lhs_expr.span),
548578
source_map.span_to_snippet(rhs_expr.span),
549579
is_assign,
550580
) {
551581
(Ok(l), Ok(r), false) => {
582+
let to_string = if l.starts_with("&") {
583+
// let a = String::new(); let b = String::new();
584+
// let _ = &a + b;
585+
format!("{}", &l[1..])
586+
} else {
587+
format!("{}.to_owned()", l)
588+
};
552589
err.multipart_suggestion(
553590
msg,
554591
vec![
555-
(lhs_expr.span, format!("{}.to_owned()", l)),
592+
(lhs_expr.span, to_string),
556593
(rhs_expr.span, format!("&{}", r)),
557594
],
558595
Applicability::MachineApplicable,

src/libstd/error.rs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -201,11 +201,19 @@ pub trait Error: Debug + Display {
201201
#[unstable(feature = "error_type_id",
202202
reason = "this is memory unsafe to override in user code",
203203
issue = "60784")]
204-
fn type_id(&self) -> TypeId where Self: 'static {
204+
fn type_id(&self, _: private::Internal) -> TypeId where Self: 'static {
205205
TypeId::of::<Self>()
206206
}
207207
}
208208

209+
mod private {
210+
// This is a hack to prevent `type_id` from being overridden by `Error`
211+
// implementations, since that can enable unsound downcasting.
212+
#[unstable(feature = "error_type_id", issue = "60784")]
213+
#[derive(Debug)]
214+
pub struct Internal;
215+
}
216+
209217
#[stable(feature = "rust1", since = "1.0.0")]
210218
impl<'a, E: Error + 'a> From<E> for Box<dyn Error + 'a> {
211219
/// Converts a type of [`Error`] into a box of dyn [`Error`].
@@ -575,7 +583,7 @@ impl dyn Error + 'static {
575583
let t = TypeId::of::<T>();
576584

577585
// Get TypeId of the type in the trait object
578-
let boxed = self.type_id();
586+
let boxed = self.type_id(private::Internal);
579587

580588
// Compare both TypeIds on equality
581589
t == boxed

0 commit comments

Comments
 (0)