Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimal implementation of implicit deref patterns for Strings #98914

Merged
merged 6 commits into from
Nov 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions compiler/rustc_feature/src/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,8 @@ declare_features! (
(active, stmt_expr_attributes, "1.6.0", Some(15701), None),
/// Allows lints part of the strict provenance effort.
(active, strict_provenance, "1.61.0", Some(95228), None),
/// Allows string patterns to dereference values to match them.
(active, string_deref_patterns, "CURRENT_RUSTC_VERSION", Some(87121), None),
/// Allows the use of `#[target_feature]` on safe functions.
(active, target_feature_11, "1.45.0", Some(69098), None),
/// Allows using `#[thread_local]` on `static` items.
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_hir/src/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ language_item_table! {
Range, sym::Range, range_struct, Target::Struct, GenericRequirement::None;
RangeToInclusive, sym::RangeToInclusive, range_to_inclusive_struct, Target::Struct, GenericRequirement::None;
RangeTo, sym::RangeTo, range_to_struct, Target::Struct, GenericRequirement::None;

String, sym::String, string, Target::Struct, GenericRequirement::None;
}

pub enum GenericRequirement {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ref_cnt += 1;
}
if let ty::Adt(adt, _) = peeled.kind()
&& self.tcx.is_diagnostic_item(sym::String, adt.did())
&& Some(adt.did()) == self.tcx.lang_items().string()
{
err.span_suggestion_verbose(
expr.span.shrink_to_hi(),
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_typeck/src/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -817,10 +817,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty.is_str()
|| matches!(
ty.kind(),
ty::Adt(adt, _) if self.tcx.is_diagnostic_item(sym::String, adt.did())
ty::Adt(adt, _) if Some(adt.did()) == self.tcx.lang_items().string()
)
}
ty::Adt(adt, _) => self.tcx.is_diagnostic_item(sym::String, adt.did()),
ty::Adt(adt, _) => Some(adt.did()) == self.tcx.lang_items().string(),
_ => false,
};
if is_string_or_ref_str && item_name.name == sym::iter {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_typeck/src/op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -556,9 +556,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let rm_borrow_msg = "remove the borrow to obtain an owned `String`";
let to_owned_msg = "create an owned `String` from a string reference";

let string_type = self.tcx.lang_items().string();
let is_std_string = |ty: Ty<'tcx>| {
ty.ty_adt_def()
.map_or(false, |ty_def| self.tcx.is_diagnostic_item(sym::String, ty_def.did()))
ty.ty_adt_def().map_or(false, |ty_def| Some(ty_def.did()) == string_type)
};

match (lhs_ty.kind(), rhs_ty.kind()) {
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_hir_typeck/src/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

if self.tcx.features().string_deref_patterns && let hir::ExprKind::Lit(Spanned { node: ast::LitKind::Str(..), .. }) = lt.kind {
let tcx = self.tcx;
let expected = self.resolve_vars_if_possible(expected);
pat_ty = match expected.kind() {
ty::Adt(def, _) if Some(def.did()) == tcx.lang_items().string() => expected,
ty::Str => tcx.mk_static_str(),
_ => pat_ty,
};
}

// Somewhat surprising: in this case, the subtyping relation goes the
// opposite way as the other cases. Actually what we really want is not
// a subtyping relation at all but rather that there exists a LUB
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/non_fmt_panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc
ty::Ref(_, r, _) if *r.kind() == ty::Str,
) || matches!(
ty.ty_adt_def(),
Some(ty_def) if cx.tcx.is_diagnostic_item(sym::String, ty_def.did()),
Some(ty_def) if Some(ty_def.did()) == cx.tcx.lang_items().string(),
);

let infcx = cx.tcx.infer_ctxt().build();
Expand Down
33 changes: 33 additions & 0 deletions compiler/rustc_mir_build/src/build/matches/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,39 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

TestKind::Eq { value, ty } => {
let tcx = self.tcx;
if let ty::Adt(def, _) = ty.kind() && Some(def.did()) == tcx.lang_items().string() {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to check the feature flag here?

Copy link
Member Author

Choose a reason for hiding this comment

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

if it falls through to the statements below it would ICE anyways. I will add a bug! statement just to make sure that the feature flag is enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess here is what you would have to look for all Deref impls of def and look for any where the types match (on both sides). What would be missing from this code is having a check, somewhere to deduplicate the resulting .deref() call for cases where the impl is not idempotent (and verify that the derefed value is dropped in the match arm and not at the end of the match block).

Copy link
Contributor

Choose a reason for hiding this comment

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

(Note to self: this would also be a place to provide suggestions if no impl matches, but there are some, potentially filtered using some heuristic, maybe limited only to std.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like you're missing a check here, even for this minimal case: you should be looking at value.ty() to see if it is actually &str, otherwise you would continue through this with, for example, a numeric literal and I don't know what the error would be in that case. Could you try out match String::new() { 42 => {} _ => {}} and tell us what the failure is?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is previous typeck code that prevents this.

Here's the output

error[E0308]: mismatched types
 --> a.rs:2:27
  |
2 |     match String::new() { 42 => {} _ => {}}
  |           -------------   ^^ expected struct `String`, found integer
  |           |
  |           this expression has type `String`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes me preemptively sad, knowing that we'll have to have deref checks in E0308 both ways to account for deref in patterns. :(

Copy link
Member Author

Choose a reason for hiding this comment

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

It could just be a TestKind variant encoding the necessary deref steps so we don't have to check again

if !tcx.features().string_deref_patterns {
bug!("matching on `String` went through without enabling string_deref_patterns");
}
let re_erased = tcx.lifetimes.re_erased;
let ref_string = self.temp(tcx.mk_imm_ref(re_erased, ty), test.span);
let ref_str_ty = tcx.mk_imm_ref(re_erased, tcx.types.str_);
let ref_str = self.temp(ref_str_ty, test.span);
let deref = tcx.require_lang_item(LangItem::Deref, None);
let method = trait_method(tcx, deref, sym::deref, ty, &[]);
let eq_block = self.cfg.start_new_block();
self.cfg.push_assign(block, source_info, ref_string, Rvalue::Ref(re_erased, BorrowKind::Shared, place));
self.cfg.terminate(
block,
source_info,
TerminatorKind::Call {
func: Operand::Constant(Box::new(Constant {
span: test.span,
user_ty: None,
literal: method,
})),
args: vec![Operand::Move(ref_string)],
destination: ref_str,
target: Some(eq_block),
cleanup: None,
from_hir_call: false,
fn_span: source_info.span
}
);
self.non_scalar_compare(eq_block, make_target_blocks, source_info, value, ref_str, ref_str_ty);
return;
}
if !ty.is_scalar() {
// Use `PartialEq::eq` instead of `BinOp::Eq`
// (the binop can only handle primitives)
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,7 @@ symbols! {
str_trim_end,
str_trim_start,
strict_provenance,
string_deref_patterns,
stringify,
struct_field_attributes,
struct_inherit,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1705,7 +1705,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
ty::Bool => Some(0),
ty::Char => Some(1),
ty::Str => Some(2),
ty::Adt(def, _) if tcx.is_diagnostic_item(sym::String, def.did()) => Some(2),
ty::Adt(def, _) if Some(def.did()) == tcx.lang_items().string() => Some(2),
ty::Int(..)
| ty::Uint(..)
| ty::Float(..)
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,8 @@ use crate::vec::Vec;
/// [`Deref`]: core::ops::Deref "ops::Deref"
/// [`as_str()`]: String::as_str
#[derive(PartialOrd, Eq, Ord)]
#[cfg_attr(not(test), rustc_diagnostic_item = "String")]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(all(not(bootstrap), not(test)), lang = "String")]
pub struct String {
vec: Vec<u8>,
}
Expand Down
74 changes: 74 additions & 0 deletions src/test/mir-opt/deref-patterns/string.foo.PreCodegen.after.mir
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// MIR for `foo` after PreCodegen

fn foo(_1: Option<String>) -> i32 {
debug s => _1; // in scope 0 at $DIR/string.rs:+0:12: +0:13
let mut _0: i32; // return place in scope 0 at $DIR/string.rs:+0:34: +0:37
let mut _2: &std::string::String; // in scope 0 at $DIR/string.rs:+2:14: +2:17
let mut _3: &str; // in scope 0 at $DIR/string.rs:+2:14: +2:17
let mut _4: bool; // in scope 0 at $DIR/string.rs:+2:14: +2:17
let mut _5: isize; // in scope 0 at $DIR/string.rs:+2:9: +2:18
let _6: std::option::Option<std::string::String>; // in scope 0 at $DIR/string.rs:+3:9: +3:10
let mut _7: bool; // in scope 0 at $DIR/string.rs:+5:1: +5:2
scope 1 {
debug s => _6; // in scope 1 at $DIR/string.rs:+3:9: +3:10
}

bb0: {
_7 = const false; // scope 0 at $DIR/string.rs:+1:11: +1:12
_7 = const true; // scope 0 at $DIR/string.rs:+1:11: +1:12
_5 = discriminant(_1); // scope 0 at $DIR/string.rs:+1:11: +1:12
switchInt(move _5) -> [1_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/string.rs:+1:5: +1:12
}

bb1: {
StorageLive(_6); // scope 0 at $DIR/string.rs:+3:9: +3:10
_7 = const false; // scope 0 at $DIR/string.rs:+3:9: +3:10
_6 = move _1; // scope 0 at $DIR/string.rs:+3:9: +3:10
_0 = const 4321_i32; // scope 1 at $DIR/string.rs:+3:14: +3:18
drop(_6) -> bb6; // scope 0 at $DIR/string.rs:+3:17: +3:18
}

bb2: {
_2 = &((_1 as Some).0: std::string::String); // scope 0 at $DIR/string.rs:+2:14: +2:17
_3 = <String as Deref>::deref(move _2) -> bb3; // scope 0 at $DIR/string.rs:+2:14: +2:17
// mir::Constant
// + span: $DIR/string.rs:9:14: 9:17
// + literal: Const { ty: for<'a> fn(&'a String) -> &'a <String as Deref>::Target {<String as Deref>::deref}, val: Value(<ZST>) }
}

bb3: {
_4 = <str as PartialEq>::eq(_3, const "a") -> bb4; // scope 0 at $DIR/string.rs:+2:14: +2:17
// mir::Constant
// + span: $DIR/string.rs:9:14: 9:17
// + literal: Const { ty: for<'a, 'b> fn(&'a str, &'b str) -> bool {<str as PartialEq>::eq}, val: Value(<ZST>) }
// mir::Constant
// + span: $DIR/string.rs:9:14: 9:17
// + literal: Const { ty: &str, val: Value(Slice(..)) }
}

bb4: {
switchInt(move _4) -> [false: bb1, otherwise: bb5]; // scope 0 at $DIR/string.rs:+2:14: +2:17
}

bb5: {
_0 = const 1234_i32; // scope 0 at $DIR/string.rs:+2:22: +2:26
goto -> bb9; // scope 0 at $DIR/string.rs:+2:22: +2:26
}

bb6: {
StorageDead(_6); // scope 0 at $DIR/string.rs:+3:17: +3:18
goto -> bb9; // scope 0 at $DIR/string.rs:+3:17: +3:18
}

bb7: {
return; // scope 0 at $DIR/string.rs:+5:2: +5:2
}

bb8: {
drop(_1) -> bb7; // scope 0 at $DIR/string.rs:+5:1: +5:2
}

bb9: {
switchInt(_7) -> [false: bb7, otherwise: bb8]; // scope 0 at $DIR/string.rs:+5:1: +5:2
}
}
12 changes: 12 additions & 0 deletions src/test/mir-opt/deref-patterns/string.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// compile-flags: -Z mir-opt-level=0 -C panic=abort

#![feature(string_deref_patterns)]
#![crate_type = "lib"]

// EMIT_MIR string.foo.PreCodegen.after.mir
pub fn foo(s: Option<String>) -> i32 {
match s {
Some("a") => 1234,
s => 4321,
}
}
17 changes: 17 additions & 0 deletions src/test/ui/deref-patterns/basic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// run-pass
// check-run-results
#![feature(string_deref_patterns)]

fn main() {
test(Some(String::from("42")));
test(Some(String::new()));
test(None);
}

fn test(o: Option<String>) {
match o {
Some("42") => println!("the answer"),
Some(_) => println!("something else?"),
None => println!("nil"),
}
}
3 changes: 3 additions & 0 deletions src/test/ui/deref-patterns/basic.run.stdout
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
the answer
something else?
nil
9 changes: 9 additions & 0 deletions src/test/ui/deref-patterns/default-infer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// check-pass
#![feature(string_deref_patterns)]

fn main() {
match <_ as Default>::default() {
"" => (),
_ => unreachable!(),
}
}
7 changes: 7 additions & 0 deletions src/test/ui/deref-patterns/gate.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// gate-test-string_deref_patterns
fn main() {
match String::new() {
"" | _ => {}
//~^ mismatched types
}
}
11 changes: 11 additions & 0 deletions src/test/ui/deref-patterns/gate.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0308]: mismatched types
--> $DIR/gate.rs:4:9
|
LL | match String::new() {
| ------------- this expression has type `String`
LL | "" | _ => {}
| ^^ expected struct `String`, found `&str`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
18 changes: 18 additions & 0 deletions src/test/ui/deref-patterns/refs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// check-pass
#![feature(string_deref_patterns)]

fn foo(s: &String) -> i32 {
match *s {
"a" => 42,
_ => -1,
}
}

fn bar(s: Option<&&&&String>) -> i32 {
match s {
Some(&&&&"&&&&") => 1,
_ => -1,
}
}

fn main() {}
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessFormat {
if format_args.format_string.parts == [kw::Empty];
if arg.format.is_default();
if match cx.typeck_results().expr_ty(value).peel_refs().kind() {
ty::Adt(adt, _) => cx.tcx.is_diagnostic_item(sym::String, adt.did()),
ty::Adt(adt, _) => Some(adt.did()) == cx.tcx.lang_items().string(),
ty::Str => true,
_ => false,
};
Expand Down
6 changes: 3 additions & 3 deletions src/tools/clippy/clippy_lints/src/format_push_string.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::ty::is_type_lang_item;
use clippy_utils::{match_def_path, paths, peel_hir_expr_refs};
use rustc_hir::{BinOpKind, Expr, ExprKind};
use rustc_hir::{BinOpKind, Expr, ExprKind, LangItem};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;
Expand Down Expand Up @@ -41,7 +41,7 @@ declare_clippy_lint! {
declare_lint_pass!(FormatPushString => [FORMAT_PUSH_STRING]);

fn is_string(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(e).peel_refs(), sym::String)
is_type_lang_item(cx, cx.typeck_results().expr_ty(e).peel_refs(), LangItem::String)
}
fn is_format(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
if let Some(macro_def_id) = e.span.ctxt().outer_expn_data().macro_def_id {
Expand Down
6 changes: 3 additions & 3 deletions src/tools/clippy/clippy_lints/src/from_str_radix_10.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::is_integer_literal;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{def, Expr, ExprKind, PrimTy, QPath, TyKind};
use rustc_hir::{def, Expr, ExprKind, LangItem, PrimTy, QPath, TyKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::Ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
Expand Down Expand Up @@ -98,5 +98,5 @@ impl<'tcx> LateLintPass<'tcx> for FromStrRadix10 {

/// Checks if a Ty is `String` or `&str`
fn is_ty_stringish(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
is_type_diagnostic_item(cx, ty, sym::String) || is_type_diagnostic_item(cx, ty, sym::str)
is_type_lang_item(cx, ty, LangItem::String) || is_type_diagnostic_item(cx, ty, sym::str)
}
6 changes: 3 additions & 3 deletions src/tools/clippy/clippy_lints/src/inherent_to_string.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::ty::{implements_trait, is_type_diagnostic_item};
use clippy_utils::ty::{implements_trait, is_type_lang_item};
use clippy_utils::{return_ty, trait_ref_of_method};
use if_chain::if_chain;
use rustc_hir::{GenericParamKind, ImplItem, ImplItemKind};
use rustc_hir::{GenericParamKind, ImplItem, ImplItemKind, LangItem};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;
Expand Down Expand Up @@ -105,7 +105,7 @@ impl<'tcx> LateLintPass<'tcx> for InherentToString {
if impl_item.generics.params.iter().all(|p| matches!(p.kind, GenericParamKind::Lifetime { .. }));

// Check if return type is String
if is_type_diagnostic_item(cx, return_ty(cx, impl_item.hir_id()), sym::String);
if is_type_lang_item(cx, return_ty(cx, impl_item.hir_id()), LangItem::String);

// Filters instances of to_string which are required by a trait
if trait_ref_of_method(cx, impl_item.owner_id.def_id).is_none();
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/clippy_lints/src/manual_retain.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};
use clippy_utils::{get_parent_expr, match_def_path, paths, SpanlessEq};
use clippy_utils::{meets_msrv, msrvs};
use rustc_errors::Applicability;
Expand Down Expand Up @@ -140,7 +140,7 @@ fn check_to_owned(
&& let Some(chars_expr_def_id) = cx.typeck_results().type_dependent_def_id(chars_expr.hir_id)
&& match_def_path(cx, chars_expr_def_id, &paths::STR_CHARS)
&& let ty = cx.typeck_results().expr_ty(str_expr).peel_refs()
&& is_type_diagnostic_item(cx, ty, sym::String)
&& is_type_lang_item(cx, ty, hir::LangItem::String)
&& SpanlessEq::new(cx).eq_expr(left_expr, str_expr) {
suggest(cx, parent_expr, left_expr, filter_expr);
}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/manual_string_new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl LateLintPass<'_> for ManualStringNew {
let ty = cx.typeck_results().expr_ty(expr);
match ty.kind() {
ty::Adt(adt_def, _) if adt_def.is_struct() => {
if !cx.tcx.is_diagnostic_item(sym::String, adt_def.did()) {
if cx.tcx.lang_items().string() != Some(adt_def.did()) {
return;
}
},
Expand Down
Loading