From 23bc6418e844cbb9170ecf37f87df20742fcf7a2 Mon Sep 17 00:00:00 2001 From: Isaac van Bakel Date: Mon, 1 Jan 2018 22:47:50 +0000 Subject: [PATCH 1/7] Moved builtin indexing checks to writeback stage Use of builtin indexing is now only checked after types are fully resolved. Types are not correctly checked in case of autoderefs. --- src/librustc_typeck/check/mod.rs | 12 ---- src/librustc_typeck/check/writeback.rs | 84 +++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 14 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 6d68824980b6a..7d7aa80519dbf 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -2217,18 +2217,6 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { adjusted_ty, index_ty); - // First, try built-in indexing. - match (adjusted_ty.builtin_index(), &index_ty.sty) { - (Some(ty), &ty::TyUint(ast::UintTy::Usize)) | - (Some(ty), &ty::TyInfer(ty::IntVar(_))) => { - debug!("try_index_step: success, using built-in indexing"); - let adjustments = autoderef.adjust_steps(lvalue_pref); - self.apply_adjustments(base_expr, adjustments); - return Some((self.tcx.types.usize, ty)); - } - _ => {} - } - for &unsize in &[false, true] { let mut self_ty = adjusted_ty; if unsize { diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 29dc983ab560b..7f966cae24900 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -12,12 +12,13 @@ // unresolved type variables and replaces "ty_var" types with their // substitutions. -use check::FnCtxt; +use check::{FnCtxt, LvalueOp}; use rustc::hir; use rustc::hir::def_id::{DefId, DefIndex}; use rustc::hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc::infer::InferCtxt; -use rustc::ty::{self, Ty, TyCtxt}; +use rustc::ty::{self, LvaluePreference, Ty, TyCtxt}; +use rustc::ty::adjustment::{Adjust, Adjustment}; use rustc::ty::fold::{TypeFoldable, TypeFolder}; use rustc::util::nodemap::DefIdSet; use syntax::ast; @@ -159,8 +160,86 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { _ => {} } } + + // Similar to operators, indexing is always assumed to be overloaded + // Here, correct cases where an indexing expression can be simplified + // to use builtin indexing because the index type is known to be + // usize-ish + fn fix_index_builtin_expr(&mut self, e: &hir::Expr) { + if let hir::ExprIndex(ref base, ref index) = e.node { + let base_ty = self.fcx.node_ty(base.hir_id); + let base_ty = self.fcx.resolve_type_vars_if_possible(&base_ty); + let index_ty = self.fcx.node_ty(index.hir_id); + let index_ty = self.fcx.resolve_type_vars_if_possible(&index_ty); + + if index_ty.is_uint() { + // HACK: the *actual* type being indexed is not stored anywhere + // so we try to find it again here by derefs + let mut autoderef = self.fcx.autoderef(e.span, base_ty); + let builtin_ty : Option<_> = { + loop { + // This is essentially a duplicate of the index discovery + // logic in typechecking code + // Find the first type dereffable to which has builtin + // indexing - this + if let Some(_) = autoderef.next() { + let current_ty = autoderef.unambiguous_final_ty(); + + if current_ty.builtin_index().is_some() { + // If there is a builtin index, use it + break Some(current_ty); + } else { + // If there's an overloaded index which happens + // to take a uint, stop looking - otherwise we + // might incorrectly deref further + let overloaded_method = + self.fcx.try_overloaded_lvalue_op( + e.span, + base_ty, + &[index_ty], + LvaluePreference::NoPreference, + LvalueOp::Index + ); + + if overloaded_method.is_some() { + break None; + } + } + } else { + break None; + } + } + }; + + if builtin_ty.is_some() { + let mut tables = self.fcx.tables.borrow_mut(); + + // Remove the method call record, which blocks use in + // constant or static cases + tables.type_dependent_defs_mut().remove(e.hir_id); + tables.node_substs_mut().remove(e.hir_id); + + tables.adjustments_mut().get_mut(base.hir_id).map(|a| { + // Discard the need for a mutable borrow + match a.pop() { + // Extra adjustment made when indexing causes a drop + // of size information - we need to get rid of it + // Since this is "after" the other adjustment to be + // discarded, we do an extra `pop()` + Some(Adjustment { kind: Adjust::Unsize, .. }) => { + // So the borrow discard actually happens here + a.pop(); + }, + _ => {} + } + }); + } + } + } + } } + /////////////////////////////////////////////////////////////////////////// // Impl of Visitor for Resolver // @@ -176,6 +255,7 @@ impl<'cx, 'gcx, 'tcx> Visitor<'gcx> for WritebackCx<'cx, 'gcx, 'tcx> { fn visit_expr(&mut self, e: &'gcx hir::Expr) { self.fix_scalar_builtin_expr(e); + self.fix_index_builtin_expr(e); self.visit_node_id(e.span, e.hir_id); From bf199d49652df2561459f39ff4f6f9ea02989999 Mon Sep 17 00:00:00 2001 From: Isaac van Bakel Date: Mon, 1 Jan 2018 22:49:01 +0000 Subject: [PATCH 2/7] Added tests for non-obvious builtin indexing Tests match issues opened on Github. Tests would not previously compile and pass. --- src/test/run-pass/issue-33903.rs | 19 ++++++++++++++++ src/test/run-pass/issue-46095.rs | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 src/test/run-pass/issue-33903.rs create mode 100644 src/test/run-pass/issue-46095.rs diff --git a/src/test/run-pass/issue-33903.rs b/src/test/run-pass/issue-33903.rs new file mode 100644 index 0000000000000..ab368537e21c0 --- /dev/null +++ b/src/test/run-pass/issue-33903.rs @@ -0,0 +1,19 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Issue 33903: +// Built-in indexing should be used even when the index is not +// trivially an integer +// Only built-in indexing can be used in constant expresssions + +const FOO: i32 = [12, 34][0 + 1]; + +fn main() {} + diff --git a/src/test/run-pass/issue-46095.rs b/src/test/run-pass/issue-46095.rs new file mode 100644 index 0000000000000..35e51ebe70b89 --- /dev/null +++ b/src/test/run-pass/issue-46095.rs @@ -0,0 +1,39 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +struct A; + +impl A { + fn take_mutably(&mut self) {} +} + +fn identity(t: T) -> T { + t +} + +// Issue 46095 +// Built-in indexing should be used even when the index is not +// trivially an integer +// Overloaded indexing would cause wrapped to be borrowed mutably + +fn main() { + let mut a1 = A; + let mut a2 = A; + + let wrapped = [&mut a1, &mut a2]; + + { + wrapped[0 + 1 - 1].take_mutably(); + } + + { + wrapped[identity(0)].take_mutably(); + } +} From 8ec3696b37ef6508650f6df727f6d92b7bef06c4 Mon Sep 17 00:00:00 2001 From: Isaac van Bakel Date: Mon, 1 Jan 2018 23:36:49 +0000 Subject: [PATCH 3/7] Made eddyb's suggested change The adjusted type is now used instead in cases of autoderefs. --- src/librustc_typeck/check/writeback.rs | 91 ++++++++------------------ 1 file changed, 26 insertions(+), 65 deletions(-) diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 7f966cae24900..93f9590a83e43 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -12,12 +12,12 @@ // unresolved type variables and replaces "ty_var" types with their // substitutions. -use check::{FnCtxt, LvalueOp}; +use check::FnCtxt; use rustc::hir; use rustc::hir::def_id::{DefId, DefIndex}; use rustc::hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc::infer::InferCtxt; -use rustc::ty::{self, LvaluePreference, Ty, TyCtxt}; +use rustc::ty::{self, Ty, TyCtxt}; use rustc::ty::adjustment::{Adjust, Adjustment}; use rustc::ty::fold::{TypeFoldable, TypeFolder}; use rustc::util::nodemap::DefIdSet; @@ -167,73 +167,34 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { // usize-ish fn fix_index_builtin_expr(&mut self, e: &hir::Expr) { if let hir::ExprIndex(ref base, ref index) = e.node { - let base_ty = self.fcx.node_ty(base.hir_id); + let mut tables = self.fcx.tables.borrow_mut(); + + let base_ty = tables.expr_ty_adjusted(&base); let base_ty = self.fcx.resolve_type_vars_if_possible(&base_ty); - let index_ty = self.fcx.node_ty(index.hir_id); + let index_ty = tables.expr_ty_adjusted(&index); let index_ty = self.fcx.resolve_type_vars_if_possible(&index_ty); - if index_ty.is_uint() { - // HACK: the *actual* type being indexed is not stored anywhere - // so we try to find it again here by derefs - let mut autoderef = self.fcx.autoderef(e.span, base_ty); - let builtin_ty : Option<_> = { - loop { - // This is essentially a duplicate of the index discovery - // logic in typechecking code - // Find the first type dereffable to which has builtin - // indexing - this - if let Some(_) = autoderef.next() { - let current_ty = autoderef.unambiguous_final_ty(); - - if current_ty.builtin_index().is_some() { - // If there is a builtin index, use it - break Some(current_ty); - } else { - // If there's an overloaded index which happens - // to take a uint, stop looking - otherwise we - // might incorrectly deref further - let overloaded_method = - self.fcx.try_overloaded_lvalue_op( - e.span, - base_ty, - &[index_ty], - LvaluePreference::NoPreference, - LvalueOp::Index - ); - - if overloaded_method.is_some() { - break None; - } - } - } else { - break None; - } + if base_ty.builtin_index().is_some() && index_ty.is_uint() { + + // Remove the method call record, which blocks use in + // constant or static cases + tables.type_dependent_defs_mut().remove(e.hir_id); + tables.node_substs_mut().remove(e.hir_id); + + tables.adjustments_mut().get_mut(base.hir_id).map(|a| { + // Discard the need for a mutable borrow + match a.pop() { + // Extra adjustment made when indexing causes a drop + // of size information - we need to get rid of it + // Since this is "after" the other adjustment to be + // discarded, we do an extra `pop()` + Some(Adjustment { kind: Adjust::Unsize, .. }) => { + // So the borrow discard actually happens here + a.pop(); + }, + _ => {} } - }; - - if builtin_ty.is_some() { - let mut tables = self.fcx.tables.borrow_mut(); - - // Remove the method call record, which blocks use in - // constant or static cases - tables.type_dependent_defs_mut().remove(e.hir_id); - tables.node_substs_mut().remove(e.hir_id); - - tables.adjustments_mut().get_mut(base.hir_id).map(|a| { - // Discard the need for a mutable borrow - match a.pop() { - // Extra adjustment made when indexing causes a drop - // of size information - we need to get rid of it - // Since this is "after" the other adjustment to be - // discarded, we do an extra `pop()` - Some(Adjustment { kind: Adjust::Unsize, .. }) => { - // So the borrow discard actually happens here - a.pop(); - }, - _ => {} - } - }); - } + }); } } } From 21ddc181f9318d9358806b6dd8f3c615c979b0bc Mon Sep 17 00:00:00 2001 From: Isaac van Bakel Date: Wed, 3 Jan 2018 23:24:06 +0000 Subject: [PATCH 4/7] Fixed unsize changing adjusted type If the final type coming out of an Adjust is a ref, it is deconstructed. Also made some formatting changes. --- src/librustc_typeck/check/writeback.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 93f9590a83e43..ae8daa86a45ff 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -17,7 +17,7 @@ use rustc::hir; use rustc::hir::def_id::{DefId, DefIndex}; use rustc::hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc::infer::InferCtxt; -use rustc::ty::{self, Ty, TyCtxt}; +use rustc::ty::{self, Ty, TyCtxt, TypeVariants}; use rustc::ty::adjustment::{Adjust, Adjustment}; use rustc::ty::fold::{TypeFoldable, TypeFolder}; use rustc::util::nodemap::DefIdSet; @@ -168,14 +168,23 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { fn fix_index_builtin_expr(&mut self, e: &hir::Expr) { if let hir::ExprIndex(ref base, ref index) = e.node { let mut tables = self.fcx.tables.borrow_mut(); - - let base_ty = tables.expr_ty_adjusted(&base); - let base_ty = self.fcx.resolve_type_vars_if_possible(&base_ty); + + let base_ty = { + let base_ty = tables.expr_ty_adjusted(&base); + // When unsizing, the final type of the expression is taken + // from the first argument of the indexing operator, which + // is a &self, and has to be deconstructed + if let TypeVariants::TyRef(_, ref ref_to) = base_ty.sty { + ref_to.ty + } else { + base_ty + } + }; + let index_ty = tables.expr_ty_adjusted(&index); let index_ty = self.fcx.resolve_type_vars_if_possible(&index_ty); if base_ty.builtin_index().is_some() && index_ty.is_uint() { - // Remove the method call record, which blocks use in // constant or static cases tables.type_dependent_defs_mut().remove(e.hir_id); From 137a63c6a2cdc3ccee8911f4ee954f811ff3a591 Mon Sep 17 00:00:00 2001 From: Isaac van Bakel Date: Thu, 4 Jan 2018 00:12:29 +0000 Subject: [PATCH 5/7] Made requested changes --- src/librustc_typeck/check/writeback.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index ae8daa86a45ff..352bebf2ea358 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -17,7 +17,7 @@ use rustc::hir; use rustc::hir::def_id::{DefId, DefIndex}; use rustc::hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc::infer::InferCtxt; -use rustc::ty::{self, Ty, TyCtxt, TypeVariants}; +use rustc::ty::{self, Ty, TyCtxt}; use rustc::ty::adjustment::{Adjust, Adjustment}; use rustc::ty::fold::{TypeFoldable, TypeFolder}; use rustc::util::nodemap::DefIdSet; @@ -174,7 +174,7 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { // When unsizing, the final type of the expression is taken // from the first argument of the indexing operator, which // is a &self, and has to be deconstructed - if let TypeVariants::TyRef(_, ref ref_to) = base_ty.sty { + if let ty::TyRef(_, ref ref_to) = base_ty.sty { ref_to.ty } else { base_ty @@ -184,9 +184,9 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { let index_ty = tables.expr_ty_adjusted(&index); let index_ty = self.fcx.resolve_type_vars_if_possible(&index_ty); - if base_ty.builtin_index().is_some() && index_ty.is_uint() { - // Remove the method call record, which blocks use in - // constant or static cases + if base_ty.builtin_index().is_some() + && index_ty == self.fcx.tcx.types.usize { + // Remove the method call record tables.type_dependent_defs_mut().remove(e.hir_id); tables.node_substs_mut().remove(e.hir_id); From 2133ace20746d579db2527b94f7d2756690852c1 Mon Sep 17 00:00:00 2001 From: Isaac van Bakel Date: Thu, 4 Jan 2018 00:30:28 +0000 Subject: [PATCH 6/7] Removed trailing whitespace --- src/librustc_typeck/check/writeback.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 352bebf2ea358..077ef5e35ac73 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -184,7 +184,7 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { let index_ty = tables.expr_ty_adjusted(&index); let index_ty = self.fcx.resolve_type_vars_if_possible(&index_ty); - if base_ty.builtin_index().is_some() + if base_ty.builtin_index().is_some() && index_ty == self.fcx.tcx.types.usize { // Remove the method call record tables.type_dependent_defs_mut().remove(e.hir_id); From 6132806d361dce863a220e278c568f83b72a3c8a Mon Sep 17 00:00:00 2001 From: Isaac van Bakel Date: Sat, 6 Jan 2018 19:04:09 +0000 Subject: [PATCH 7/7] Correct reasoning behind writeback ref removal The ref was actually always necessary. This clarifies some incorrect thinking in the original code that might have led to errors in the future to do with unsizing. --- src/librustc_typeck/check/writeback.rs | 64 ++++++++++++-------------- 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 077ef5e35ac73..5e102c7a44516 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -169,41 +169,37 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> { if let hir::ExprIndex(ref base, ref index) = e.node { let mut tables = self.fcx.tables.borrow_mut(); - let base_ty = { - let base_ty = tables.expr_ty_adjusted(&base); - // When unsizing, the final type of the expression is taken - // from the first argument of the indexing operator, which - // is a &self, and has to be deconstructed - if let ty::TyRef(_, ref ref_to) = base_ty.sty { - ref_to.ty - } else { - base_ty - } - }; - - let index_ty = tables.expr_ty_adjusted(&index); - let index_ty = self.fcx.resolve_type_vars_if_possible(&index_ty); - - if base_ty.builtin_index().is_some() - && index_ty == self.fcx.tcx.types.usize { - // Remove the method call record - tables.type_dependent_defs_mut().remove(e.hir_id); - tables.node_substs_mut().remove(e.hir_id); - - tables.adjustments_mut().get_mut(base.hir_id).map(|a| { - // Discard the need for a mutable borrow - match a.pop() { - // Extra adjustment made when indexing causes a drop - // of size information - we need to get rid of it - // Since this is "after" the other adjustment to be - // discarded, we do an extra `pop()` - Some(Adjustment { kind: Adjust::Unsize, .. }) => { - // So the borrow discard actually happens here - a.pop(); - }, - _ => {} + match tables.expr_ty_adjusted(&base).sty { + // All valid indexing looks like this + ty::TyRef(_, ty::TypeAndMut { ty: ref base_ty, .. }) => { + let index_ty = tables.expr_ty_adjusted(&index); + let index_ty = self.fcx.resolve_type_vars_if_possible(&index_ty); + + if base_ty.builtin_index().is_some() + && index_ty == self.fcx.tcx.types.usize { + // Remove the method call record + tables.type_dependent_defs_mut().remove(e.hir_id); + tables.node_substs_mut().remove(e.hir_id); + + tables.adjustments_mut().get_mut(base.hir_id).map(|a| { + // Discard the need for a mutable borrow + match a.pop() { + // Extra adjustment made when indexing causes a drop + // of size information - we need to get rid of it + // Since this is "after" the other adjustment to be + // discarded, we do an extra `pop()` + Some(Adjustment { kind: Adjust::Unsize, .. }) => { + // So the borrow discard actually happens here + a.pop(); + }, + _ => {} + } + }); } - }); + }, + // Might encounter non-valid indexes at this point, so there + // has to be a fall-through + _ => {}, } } }