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.

Sign up for GitHub

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

Suggest deref when coercing ty::Ref to ty::RawPtr with arbitrary mutability #71726

Merged
merged 4 commits into from
May 3, 2020
Merged

Suggest deref when coercing ty::Ref to ty::RawPtr with arbitrary mutability #71726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60d62be
Suggest deref when coercing `ty::Ref` to `ty::RawPtr` with arbitrary
ldm0 Apr 30, 2020
80d04cc
Add comments for deref_steps()
ldm0 May 1, 2020
089d4bb
Suggestion for immutable reference -> mutable pointer should be emitted
ldm0 May 1, 2020
9a212c1
Replace convenient function `remove_prefix()` with `replace_prefix()`
ldm0 May 1, 2020
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
18 changes: 15 additions & 3 deletions src/librustc_typeck/check/coercion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ use rustc_trait_selection::traits::{self, ObligationCause, ObligationCauseCode};
use smallvec::{smallvec, SmallVec};
use std::ops::Deref;

pub struct Coerce<'a, 'tcx> {
struct Coerce<'a, 'tcx> {
fcx: &'a FnCtxt<'a, 'tcx>,
cause: ObligationCause<'tcx>,
use_lub: bool,
Expand Down Expand Up @@ -126,15 +126,15 @@ fn success<'tcx>(
}

impl<'f, 'tcx> Coerce<'f, 'tcx> {
pub fn new(
fn new(
fcx: &'f FnCtxt<'f, 'tcx>,
cause: ObligationCause<'tcx>,
allow_two_phase: AllowTwoPhase,
) -> Self {
Coerce { fcx, cause, allow_two_phase, use_lub: false }
}

pub fn unify(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> InferResult<'tcx, Ty<'tcx>> {
fn unify(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> InferResult<'tcx, Ty<'tcx>> {
debug!("unify(a: {:?}, b: {:?}, use_lub: {})", a, b, self.use_lub);
self.commit_if_ok(|_| {
if self.use_lub {
Expand Down Expand Up @@ -831,6 +831,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.probe(|_| coerce.coerce(source, target)).is_ok()
}

/// Given a type and a target type, this function will calculate and return
/// how many dereference steps needed to achieve `expr_ty <: target`. If
/// it's not possible, return `None`.
pub fn deref_steps(&self, expr_ty: Ty<'tcx>, target: Ty<'tcx>) -> Option<usize> {
ldm0 marked this conversation as resolved.
Show resolved Hide resolved
let cause = self.cause(rustc_span::DUMMY_SP, ObligationCauseCode::ExprAssignable);
// We don't ever need two-phase here since we throw out the result of the coercion
let coerce = Coerce::new(self, cause, AllowTwoPhase::No);
coerce
.autoderef(rustc_span::DUMMY_SP, expr_ty)
.find_map(|(ty, steps)| coerce.unify(ty, target).ok().map(|_| steps))
}

/// Given some expressions, their known unified type and another expression,
/// tries to unify the types, potentially inserting coercions on any of the
/// provided expressions and returns their LUB (aka "common supertype").
Expand Down
136 changes: 96 additions & 40 deletions src/librustc_typeck/check/demand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::check::coercion::Coerce;
use crate::check::FnCtxt;
use rustc_infer::infer::InferOk;
use rustc_trait_selection::infer::InferCtxtExt as _;
Expand All @@ -9,7 +8,6 @@ use rustc_ast::util::parser::PREC_POSTFIX;
use rustc_errors::{Applicability, DiagnosticBuilder};
use rustc_hir as hir;
use rustc_hir::{is_range_literal, Node};
use rustc_middle::traits::ObligationCauseCode;
use rustc_middle::ty::adjustment::AllowTwoPhase;
use rustc_middle::ty::{self, AssocItem, Ty, TypeAndMut};
use rustc_span::symbol::sym;
Expand Down Expand Up @@ -355,6 +353,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
false
}

fn replace_prefix<A, B, C>(&self, s: A, old: B, new: C) -> Option<String>
where
A: AsRef<str>,
B: AsRef<str>,
C: AsRef<str>,
{
let s = s.as_ref();
let old = old.as_ref();
if s.starts_with(old) { Some(new.as_ref().to_owned() + &s[old.len()..]) } else { None }
}

/// This function is used to determine potential "simple" improvements or users' errors and
/// provide them useful help. For example:
///
Expand All @@ -376,7 +385,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expr: &hir::Expr<'_>,
checked_ty: Ty<'tcx>,
expected: Ty<'tcx>,
) -> Option<(Span, &'static str, String)> {
) -> Option<(Span, &'static str, String, Applicability)> {
let sm = self.sess().source_map();
let sp = expr.span;
if sm.is_imported(sp) {
Expand All @@ -400,11 +409,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
(&ty::Str, &ty::Array(arr, _) | &ty::Slice(arr)) if arr == self.tcx.types.u8 => {
if let hir::ExprKind::Lit(_) = expr.kind {
if let Ok(src) = sm.span_to_snippet(sp) {
if src.starts_with("b\"") {
if let Some(src) = self.replace_prefix(src, "b\"", "\"") {
return Some((
sp,
"consider removing the leading `b`",
src[1..].to_string(),
src,
Applicability::MachineApplicable,
));
}
}
Expand All @@ -413,11 +423,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
(&ty::Array(arr, _) | &ty::Slice(arr), &ty::Str) if arr == self.tcx.types.u8 => {
if let hir::ExprKind::Lit(_) = expr.kind {
if let Ok(src) = sm.span_to_snippet(sp) {
if src.starts_with('"') {
if let Some(src) = self.replace_prefix(src, "\"", "b\"") {
return Some((
sp,
"consider adding a leading `b`",
format!("b{}", src),
src,
Applicability::MachineApplicable,
));
}
}
Expand Down Expand Up @@ -470,7 +481,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let sugg_expr = if needs_parens { format!("({})", src) } else { src };

if let Some(sugg) = self.can_use_as_ref(expr) {
return Some(sugg);
return Some((
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're reaching the point where this function should probably return a struct with named fields ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I think for consistency that would requires a lot of irrelevant changes. If that's really need would changing them in a seperate PR be better? :-P

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yea, this was mainly a drive by comment, feel free to address it or not or in a different PR

sugg.0,
sugg.1,
sugg.2,
Applicability::MachineApplicable,
));
}
let field_name = if is_struct_pat_shorthand_field {
format!("{}: ", sugg_expr)
Expand All @@ -495,6 +511,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
"consider dereferencing here to assign to the mutable \
borrowed piece of memory",
format!("*{}", src),
Applicability::MachineApplicable,
));
}
}
Expand All @@ -505,11 +522,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
sp,
"consider mutably borrowing here",
format!("{}&mut {}", field_name, sugg_expr),
Applicability::MachineApplicable,
),
hir::Mutability::Not => (
sp,
"consider borrowing here",
format!("{}&{}", field_name, sugg_expr),
Applicability::MachineApplicable,
),
});
}
Expand All @@ -526,51 +545,88 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// We have `&T`, check if what was expected was `T`. If so,
// we may want to suggest removing a `&`.
if sm.is_imported(expr.span) {
if let Ok(code) = sm.span_to_snippet(sp) {
if code.starts_with('&') {
if let Ok(src) = sm.span_to_snippet(sp) {
if let Some(src) = self.replace_prefix(src, "&", "") {
return Some((
sp,
"consider removing the borrow",
code[1..].to_string(),
src,
Applicability::MachineApplicable,
));
}
}
return None;
}
if let Ok(code) = sm.span_to_snippet(expr.span) {
return Some((sp, "consider removing the borrow", code));
return Some((
sp,
"consider removing the borrow",
code,
Applicability::MachineApplicable,
));
}
}
(
_,
&ty::RawPtr(TypeAndMut { ty: _, mutbl: hir::Mutability::Not }),
&ty::Ref(_, _, hir::Mutability::Not),
&ty::RawPtr(TypeAndMut { ty: ty_b, mutbl: mutbl_b }),
&ty::Ref(_, ty_a, mutbl_a),
) => {
let cause = self.cause(rustc_span::DUMMY_SP, ObligationCauseCode::ExprAssignable);
// We don't ever need two-phase here since we throw out the result of the coercion
let coerce = Coerce::new(self, cause, AllowTwoPhase::No);

if let Some(steps) =
coerce.autoderef(sp, checked_ty).skip(1).find_map(|(referent_ty, steps)| {
coerce
.unify(
coerce.tcx.mk_ptr(ty::TypeAndMut {
mutbl: hir::Mutability::Not,
ty: referent_ty,
}),
expected,
)
.ok()
.map(|_| steps)
})
{
// The pointer type implements `Copy` trait so the suggestion is always valid.
if let Ok(code) = sm.span_to_snippet(sp) {
if code.starts_with('&') {
let derefs = "*".repeat(steps - 1);
let message = "consider dereferencing the reference";
let suggestion = format!("&{}{}", derefs, code[1..].to_string());
return Some((sp, message, suggestion));
if let Some(steps) = self.deref_steps(ty_a, ty_b) {
// Only suggest valid if dereferencing needed.
if steps > 0 {
// The pointer type implements `Copy` trait so the suggestion is always valid.
if let Ok(src) = sm.span_to_snippet(sp) {
let derefs = &"*".repeat(steps);
if let Some((src, applicability)) = match mutbl_b {
hir::Mutability::Mut => {
let new_prefix = "&mut ".to_owned() + derefs;
match mutbl_a {
hir::Mutability::Mut => {
if let Some(s) =
self.replace_prefix(src, "&mut ", new_prefix)
{
Some((s, Applicability::MachineApplicable))
} else {
None
}
}
hir::Mutability::Not => {
if let Some(s) =
self.replace_prefix(src, "&", new_prefix)
{
Some((s, Applicability::Unspecified))
} else {
None
}
}
}
}
hir::Mutability::Not => {
let new_prefix = "&".to_owned() + derefs;
match mutbl_a {
hir::Mutability::Mut => {
if let Some(s) =
self.replace_prefix(src, "&mut ", new_prefix)
{
Some((s, Applicability::MachineApplicable))
} else {
None
}
}
hir::Mutability::Not => {
if let Some(s) =
self.replace_prefix(src, "&", new_prefix)
{
Some((s, Applicability::MachineApplicable))
} else {
None
}
}
}
}
} {
return Some((sp, "consider dereferencing", src, applicability));
}
}
}
}
Expand Down Expand Up @@ -616,7 +672,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
} else {
format!("*{}", code)
};
return Some((sp, message, suggestion));
return Some((sp, message, suggestion, Applicability::MachineApplicable));
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_typeck/check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5036,8 +5036,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expected: Ty<'tcx>,
found: Ty<'tcx>,
) {
if let Some((sp, msg, suggestion)) = self.check_ref(expr, found, expected) {
err.span_suggestion(sp, msg, suggestion, Applicability::MachineApplicable);
if let Some((sp, msg, suggestion, applicability)) = self.check_ref(expr, found, expected) {
err.span_suggestion(sp, msg, suggestion, applicability);
} else if let (ty::FnDef(def_id, ..), true) =
(&found.kind, self.suggest_fn_call(err, expr, expected, found))
{
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-32122-1.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | let _: *const u8 = &a;
| --------- ^^
| | |
| | expected `u8`, found struct `Foo`
| | help: consider dereferencing the reference: `&*a`
| | help: consider dereferencing: `&*a`
| expected due to this
|
= note: expected raw pointer `*const u8`
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-32122-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | let _: *const u8 = &a;
| --------- ^^
| | |
| | expected `u8`, found struct `Emm`
| | help: consider dereferencing the reference: `&***a`
| | help: consider dereferencing: `&***a`
| expected due to this
|
= note: expected raw pointer `*const u8`
Expand Down
53 changes: 53 additions & 0 deletions src/test/ui/issues/issue-71676-1.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// run-rustfix
use std::ops::Deref;
use std::ops::DerefMut;
struct Bar(u8);
struct Foo(Bar);
struct Emm(Foo);
impl Deref for Bar{
type Target = u8;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl Deref for Foo {
type Target = Bar;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl Deref for Emm {
type Target = Foo;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl DerefMut for Bar{
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
impl DerefMut for Foo {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
impl DerefMut for Emm {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
fn main() {
// Suggest dereference with arbitrary mutability
let a = Emm(Foo(Bar(0)));
let _: *const u8 = &***a; //~ ERROR mismatched types

let mut a = Emm(Foo(Bar(0)));
let _: *mut u8 = &mut ***a; //~ ERROR mismatched types

let a = Emm(Foo(Bar(0)));
let _: *const u8 = &***a; //~ ERROR mismatched types

let mut a = Emm(Foo(Bar(0)));
let _: *mut u8 = &mut ***a; //~ ERROR mismatched types
}
Loading