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

Implement a lint that highlights all moves larger than a configured limit #83519

Merged
merged 5 commits into from
Apr 24, 2021
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
3 changes: 3 additions & 0 deletions compiler/rustc_feature/src/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,9 @@ declare_features! (
/// Allows associated types in inherent impls.
(active, inherent_associated_types, "1.52.0", Some(8995), None),

// Allows setting the threshold for the `large_assignments` lint.
(active, large_assignments, "1.52.0", Some(83518), None),

/// Allows `extern "C-unwind" fn` to enable unwinding across ABI boundaries.
(active, c_unwind, "1.52.0", Some(74990), None),

Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
const_eval_limit, CrateLevel, template!(NameValueStr: "N"), const_eval_limit,
experimental!(const_eval_limit)
),
gated!(
move_size_limit, CrateLevel, template!(NameValueStr: "N"), large_assignments,
experimental!(move_size_limit)
),

// Entry point:
ungated!(main, Normal, template!(Word)),
Expand Down
34 changes: 34 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2877,6 +2877,39 @@ declare_lint! {
};
}

declare_lint! {
/// The `large_assignments` lint detects when objects of large
/// types are being moved around.
///
/// ### Example
///
/// ```rust,ignore (can crash on some platforms)
/// let x = [0; 50000];
/// let y = x;
/// ```
///
/// produces:
///
/// ```text
/// warning: moving a large value
/// --> $DIR/move-large.rs:1:3
/// let y = x;
/// - Copied large value here
/// ```
///
/// ### Explanation
///
/// When using a large type in a plain assignment or in a function
/// argument, idiomatic code can be inefficient.
/// Ideally appropriate optimizations would resolve this, but such
/// optimizations are only done in a best-effort manner.
/// This lint will trigger on all sites of large moves and thus allow the
/// user to resolve them in code.
pub LARGE_ASSIGNMENTS,
Warn,
"detects large moves or copies",
}

declare_lint_pass! {
/// Does nothing as a lint pass, but registers some `Lint`s
/// that are used by other parts of the compiler.
Expand Down Expand Up @@ -2962,6 +2995,7 @@ declare_lint_pass! {
LEGACY_DERIVE_HELPERS,
PROC_MACRO_BACK_COMPAT,
OR_PATTERNS_BACK_COMPAT,
LARGE_ASSIGNMENTS,
]
}

Expand Down
15 changes: 10 additions & 5 deletions compiler/rustc_middle/src/middle/limits.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
//! Registering limits, recursion_limit, type_length_limit and const_eval_limit
//! Registering limits:
//! * recursion_limit,
//! * move_size_limit,
//! * type_length_limit, and
//! * const_eval_limit
//!
//! There are various parts of the compiler that must impose arbitrary limits
//! on how deeply they recurse to prevent stack overflow. Users can override
Expand All @@ -8,21 +12,22 @@
use crate::bug;
use rustc_ast as ast;
use rustc_data_structures::sync::OnceCell;
use rustc_session::{Limit, Session};
use rustc_session::Session;
use rustc_span::symbol::{sym, Symbol};

use std::num::IntErrorKind;

pub fn update_limits(sess: &Session, krate: &ast::Crate) {
update_limit(sess, krate, &sess.recursion_limit, sym::recursion_limit, 128);
update_limit(sess, krate, &sess.move_size_limit, sym::move_size_limit, 0);
update_limit(sess, krate, &sess.type_length_limit, sym::type_length_limit, 1048576);
update_limit(sess, krate, &sess.const_eval_limit, sym::const_eval_limit, 1_000_000);
}

fn update_limit(
sess: &Session,
krate: &ast::Crate,
limit: &OnceCell<Limit>,
limit: &OnceCell<impl From<usize> + std::fmt::Debug>,
name: Symbol,
default: usize,
) {
Expand All @@ -34,7 +39,7 @@ fn update_limit(
if let Some(s) = attr.value_str() {
match s.as_str().parse() {
Ok(n) => {
limit.set(Limit::new(n)).unwrap();
limit.set(From::from(n)).unwrap();
return;
}
Err(e) => {
Expand Down Expand Up @@ -63,5 +68,5 @@ fn update_limit(
}
}
}
limit.set(Limit::new(default)).unwrap();
limit.set(From::from(default)).unwrap();
}
25 changes: 24 additions & 1 deletion compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ use crate::ty::print::{FmtPrinter, Printer};
use crate::ty::subst::{Subst, SubstsRef};
use crate::ty::{self, List, Ty, TyCtxt};
use crate::ty::{AdtDef, InstanceDef, Region, ScalarInt, UserTypeAnnotationIndex};
use rustc_hir as hir;
use rustc_hir::def::{CtorKind, Namespace};
use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX};
use rustc_hir::{self, GeneratorKind};
use rustc_hir::{self as hir, HirId};
use rustc_target::abi::{Size, VariantIdx};

use polonius_engine::Atom;
Expand Down Expand Up @@ -1948,6 +1948,29 @@ rustc_index::newtype_index! {
}
}

impl SourceScope {
/// Finds the original HirId this MIR item came from.
/// This is necessary after MIR optimizations, as otherwise we get a HirId
/// from the function that was inlined instead of the function call site.
pub fn lint_root(
self,
source_scopes: &IndexVec<SourceScope, SourceScopeData<'tcx>>,
) -> Option<HirId> {
let mut data = &source_scopes[self];
// FIXME(oli-obk): we should be able to just walk the `inlined_parent_scope`, but it
// does not work as I thought it would. Needs more investigation and documentation.
while data.inlined.is_some() {
trace!(?data);
data = &source_scopes[data.parent_scope.unwrap()];
}
trace!(?data);
match &data.local_data {
ClearCrossCrate::Set(data) => Some(data.lint_root),
ClearCrossCrate::Clear => None,
}
}
}

#[derive(Clone, Debug, TyEncodable, TyDecodable, HashStable, TypeFoldable)]
pub struct SourceScopeData<'tcx> {
pub span: Span,
Expand Down
42 changes: 42 additions & 0 deletions compiler/rustc_mir/src/monomorphize/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,9 @@ use rustc_middle::ty::subst::{GenericArgKind, InternalSubsts};
use rustc_middle::ty::{self, GenericParamDefKind, Instance, Ty, TyCtxt, TypeFoldable};
use rustc_middle::{middle::codegen_fn_attrs::CodegenFnAttrFlags, mir::visit::TyContext};
use rustc_session::config::EntryFnType;
use rustc_session::lint::builtin::LARGE_ASSIGNMENTS;
use rustc_span::source_map::{dummy_spanned, respan, Span, Spanned, DUMMY_SP};
use rustc_target::abi::Size;
use smallvec::SmallVec;
use std::iter;
use std::ops::Range;
Expand Down Expand Up @@ -753,6 +755,46 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
self.super_terminator(terminator, location);
}

fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, location: Location) {
self.super_operand(operand, location);
let limit = self.tcx.sess.move_size_limit();
if limit == 0 {
return;
}
let limit = Size::from_bytes(limit);
let ty = operand.ty(self.body, self.tcx);
let ty = self.monomorphize(ty);
let layout = self.tcx.layout_of(ty::ParamEnv::reveal_all().and(ty));
if let Ok(layout) = layout {
if layout.size > limit {
debug!(?layout);
let source_info = self.body.source_info(location);
debug!(?source_info);
let lint_root = source_info.scope.lint_root(&self.body.source_scopes);
debug!(?lint_root);
let lint_root = match lint_root {
Some(lint_root) => lint_root,
// This happens when the issue is in a function from a foreign crate that
// we monomorphized in the current crate. We can't get a `HirId` for things
// in other crates.
// FIXME: Find out where to report the lint on. Maybe simply crate-level lint root
// but correct span? This would make the lint at least accept crate-level lint attributes.
None => return,
};
self.tcx.struct_span_lint_hir(
LARGE_ASSIGNMENTS,
lint_root,
source_info.span,
|lint| {
let mut err = lint.build(&format!("moving {} bytes", layout.size.bytes()));
err.span_label(source_info.span, "value moved from here");
err.emit()
},
);
}
}
}

fn visit_local(
&mut self,
_place_local: &Local,
Expand Down
19 changes: 4 additions & 15 deletions compiler/rustc_mir/src/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ use rustc_middle::mir::visit::{
MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor,
};
use rustc_middle::mir::{
AssertKind, BasicBlock, BinOp, Body, ClearCrossCrate, Constant, ConstantKind, Local, LocalDecl,
LocalKind, Location, Operand, Place, Rvalue, SourceInfo, SourceScope, SourceScopeData,
Statement, StatementKind, Terminator, TerminatorKind, UnOp, RETURN_PLACE,
AssertKind, BasicBlock, BinOp, Body, Constant, ConstantKind, Local, LocalDecl, LocalKind,
Location, Operand, Place, Rvalue, SourceInfo, SourceScope, SourceScopeData, Statement,
StatementKind, Terminator, TerminatorKind, UnOp, RETURN_PLACE,
};
use rustc_middle::ty::layout::{HasTyCtxt, LayoutError, TyAndLayout};
use rustc_middle::ty::subst::{InternalSubsts, Subst};
Expand Down Expand Up @@ -440,18 +440,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}

fn lint_root(&self, source_info: SourceInfo) -> Option<HirId> {
let mut data = &self.source_scopes[source_info.scope];
// FIXME(oli-obk): we should be able to just walk the `inlined_parent_scope`, but it
// does not work as I thought it would. Needs more investigation and documentation.
while data.inlined.is_some() {
trace!(?data);
data = &self.source_scopes[data.parent_scope.unwrap()];
}
trace!(?data);
match &data.local_data {
ClearCrossCrate::Set(data) => Some(data.lint_root),
ClearCrossCrate::Clear => None,
}
source_info.scope.lint_root(&self.source_scopes)
}

fn use_ecx<F, T>(&mut self, f: F) -> Option<T>
Expand Down
16 changes: 16 additions & 0 deletions compiler/rustc_session/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ impl Limit {
}
}

impl From<usize> for Limit {
fn from(value: usize) -> Self {
Self::new(value)
}
}

impl fmt::Display for Limit {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0)
Expand Down Expand Up @@ -143,6 +149,10 @@ pub struct Session {
/// operations such as auto-dereference and monomorphization.
pub recursion_limit: OnceCell<Limit>,

/// The size at which the `large_assignments` lint starts
/// being emitted.
pub move_size_limit: OnceCell<usize>,

/// The maximum length of types during monomorphization.
pub type_length_limit: OnceCell<Limit>,

Expand Down Expand Up @@ -352,6 +362,11 @@ impl Session {
self.recursion_limit.get().copied().unwrap()
}

#[inline]
pub fn move_size_limit(&self) -> usize {
self.move_size_limit.get().copied().unwrap()
}

#[inline]
pub fn type_length_limit(&self) -> Limit {
self.type_length_limit.get().copied().unwrap()
Expand Down Expand Up @@ -1414,6 +1429,7 @@ pub fn build_session(
features: OnceCell::new(),
lint_store: OnceCell::new(),
recursion_limit: OnceCell::new(),
move_size_limit: OnceCell::new(),
type_length_limit: OnceCell::new(),
const_eval_limit: OnceCell::new(),
incr_comp_session: OneThread::new(RefCell::new(IncrCompSession::NotInitialized)),
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,7 @@ symbols! {
label_break_value,
lang,
lang_items,
large_assignments,
lateout,
lazy_normalization_consts,
le,
Expand Down Expand Up @@ -749,6 +750,7 @@ symbols! {
more_struct_aliases,
movbe_target_feature,
move_ref_pattern,
move_size_limit,
mul,
mul_assign,
mul_with_overflow,
Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/async-await/large_moves.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#![deny(large_assignments)]
#![feature(large_assignments)]
#![move_size_limit = "1000"]
// build-fail
// only-x86_64

// edition:2018

fn main() {
let x = async { //~ ERROR large_assignments
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a particularly interesting instance of the problem (and perhaps a huge motivation for this lint).

let y = [0; 9999];
dbg!(y);
thing(&y).await;
dbg!(y);
};
let z = (x, 42); //~ ERROR large_assignments
//~^ ERROR large_assignments
let a = z.0; //~ ERROR large_assignments
let b = z.1;
}

async fn thing(y: &[u8]) {
dbg!(y);
}
38 changes: 38 additions & 0 deletions src/test/ui/async-await/large_moves.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
error: moving 10024 bytes
--> $DIR/large_moves.rs:10:13
|
LL | let x = async {
| _____________^
LL | | let y = [0; 9999];
LL | | dbg!(y);
LL | | thing(&y).await;
LL | | dbg!(y);
LL | | };
| |_____^ value moved from here
|
note: the lint level is defined here
--> $DIR/large_moves.rs:1:9
|
LL | #![deny(large_assignments)]
| ^^^^^^^^^^^^^^^^^

error: moving 10024 bytes
--> $DIR/large_moves.rs:16:14
|
LL | let z = (x, 42);
| ^ value moved from here

error: moving 10024 bytes
--> $DIR/large_moves.rs:16:13
|
LL | let z = (x, 42);
| ^^^^^^^ value moved from here

error: moving 10024 bytes
--> $DIR/large_moves.rs:18:13
|
LL | let a = z.0;
| ^^^ value moved from here

error: aborting due to 4 previous errors

5 changes: 5 additions & 0 deletions src/test/ui/feature-gates/feature-gate-large-assignments.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// check that `move_size_limit is feature-gated

#![move_size_limit = "42"] //~ ERROR the `#[move_size_limit]` attribute is an experimental feature

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/feature-gates/feature-gate-large-assignments.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0658]: the `#[move_size_limit]` attribute is an experimental feature
--> $DIR/feature-gate-large-assignments.rs:3:1
|
LL | #![move_size_limit = "42"]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #83518 <https://github.com/rust-lang/rust/issues/83518> for more information
= help: add `#![feature(large_assignments)]` to the crate attributes to enable

error: aborting due to previous error

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