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

Add lint for never type regressions #68350

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
da33dc0
Initial work for linting on bad never fallback
Aaron1011 Jan 12, 2020
0c49893
Emit warning
Aaron1011 Jan 12, 2020
3a22be9
Improve lint to check substs
Aaron1011 Jan 12, 2020
d0dc1e3
Make the error message much better
Aaron1011 Jan 12, 2020
ebf1918
Provide a better error when no substs is provided for generic param
Aaron1011 Jan 13, 2020
5253bb6
Factor out code into its own module
Aaron1011 Jan 13, 2020
087af85
Tweak error message
Aaron1011 Jan 15, 2020
932d7c2
Some fixup and cleanup
Aaron1011 Jan 15, 2020
10cd2e3
Work on splitting up post_fallback
Aaron1011 Jan 15, 2020
a8ce823
More refactoring
Aaron1011 Jan 15, 2020
65318da
Add a bunch of comments
Aaron1011 Jan 15, 2020
61b879c
Some additional cleanup
Aaron1011 Jan 15, 2020
c5691a8
Change warning to error
Aaron1011 Jan 15, 2020
1ed39bc
Add some tests
Aaron1011 Jan 17, 2020
d4bd422
Fix tidy errors
Aaron1011 Jan 18, 2020
f1961b5
Fix failing test
Aaron1011 Jan 18, 2020
643a708
Unconditionally enable never-type fallback for Crater
Aaron1011 Jan 18, 2020
a81c513
Bail out on inference variables in function arguments
Aaron1011 Jan 19, 2020
9eb803a
Run never-compat post_fallback after errors are reported
Aaron1011 Jan 19, 2020
78a831f
Bail out if errors have occured
Aaron1011 Jan 19, 2020
3aefe61
Never consider built-in constructor fns to be 'questionable'
Aaron1011 Jan 19, 2020
409e8d7
Only skip fallback when errors have been reported in the same function
Aaron1011 Jan 19, 2020
39af896
Update test output
Aaron1011 Jan 19, 2020
c82ea60
Add explicit type annotations for `vec.push(break)` test
Aaron1011 Jan 19, 2020
fe77a00
Properly handle missing divering/regular inference variables
Aaron1011 Jan 26, 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
3 changes: 2 additions & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2286,7 +2286,8 @@ impl<'tcx> TyCtxt<'tcx> {

#[inline]
pub fn mk_diverging_default(self) -> Ty<'tcx> {
if self.features().never_type_fallback { self.types.never } else { self.types.unit }
self.types.never
//if self.features().never_type_fallback { self.types.never } else { self.types.unit }
}

#[inline]
Expand Down
130 changes: 126 additions & 4 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ mod expr;
mod generator_interior;
pub mod intrinsic;
pub mod method;
mod never_compat;
mod op;
mod pat;
mod regionck;
Expand Down Expand Up @@ -135,6 +136,7 @@ use syntax::util::parser::ExprPrecedence;

use rustc_error_codes::*;

use std::borrow::Cow;
use std::cell::{Cell, Ref, RefCell, RefMut};
use std::cmp;
use std::collections::hash_map::Entry;
Expand Down Expand Up @@ -254,6 +256,14 @@ pub struct Inherited<'a, 'tcx> {
/// not clear.
implicit_region_bound: Option<ty::Region<'tcx>>,

/// Maps each expression with a generic path
/// (e.g. `foo::<u8>()` to `InferredPath` containing
/// additional information used by `NeverCompatHandler`.
///
/// Each entry in this map is gradually filled in during typecheck,
/// as the information we need is not available all at once.
inferred_paths: RefCell<FxHashMap<hir::HirId, InferredPath<'tcx>>>,

body_id: Option<hir::BodyId>,
}

Expand Down Expand Up @@ -619,6 +629,48 @@ pub struct FnCtxt<'a, 'tcx> {
inh: &'a Inherited<'a, 'tcx>,
}

/// Stores additional data about a generic path
/// containing inference variables (e.g. `my_method::<_, u8>(bar)`).
/// This is used by `NeverCompatHandler` to inspect
/// all method calls that contain inference variables.
///
/// This struct is a little strange, in that its data
/// is filled in from two different places in typecheck.
/// Thw two `Option` fields are written by `check_argument_types`
/// and `instantiate_value_path`, since neither method
/// has all of the information it needs.
#[derive(Clone, Debug)]
struct InferredPath<'tcx> {
/// The span of the corresponding expression.
span: Span,
/// The type of this path. For method calls,
/// this is a `ty::FnDef`
ty: Option<Ty<'tcx>>,
/// The types of the arguments (*not* generic substs)
/// provided to this path, if it represents a method
/// call. For example, `foo(true, 25)` would have
/// types `[bool, i32]`. If this path does not
/// correspond to a method, then this will be `None`
///
/// This is a `Cow` rather than a `Vec` or slice
/// to accommodate `check_argument_types`, which may
/// be called with either an interned slice or a Vec.
/// A `Cow` lets us avoid unecessary interning
/// and Vec construction, since we just need to iterate
/// over this
args: Option<Cow<'tcx, [Ty<'tcx>]>>,
/// The unresolved inference variables for each
/// generic substs. Each entry in the outer vec
/// corresponds to a generic substs in the function.
///
/// For example, suppose we have the function
/// `fn foo<T, V> (){ ... }`.
///
/// The method call `foo::<MyStruct<_#0t, #1t>, true>>()`
/// will have an `unresolved_vars` of `[[_#0t, _#1t], []]`
unresolved_vars: Vec<Vec<Ty<'tcx>>>,
}
Comment on lines +632 to +672
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to move this into never_compat to keep all the logic required for that more self-contained. This file is also already huge and I would prefer not to add anything more miscellaneous to it.


impl<'a, 'tcx> Deref for FnCtxt<'a, 'tcx> {
type Target = Inherited<'a, 'tcx>;
fn deref(&self) -> &Self::Target {
Expand Down Expand Up @@ -685,6 +737,7 @@ impl Inherited<'a, 'tcx> {
opaque_types: RefCell::new(Default::default()),
opaque_types_vars: RefCell::new(Default::default()),
implicit_region_bound,
inferred_paths: RefCell::new(Default::default()),
body_id,
}
}
Expand Down Expand Up @@ -1053,6 +1106,7 @@ fn typeck_tables_of_with_fallback<'tcx>(
// All type checking constraints were added, try to fallback unsolved variables.
fcx.select_obligations_where_possible(false, |_| {});
let mut fallback_has_occurred = false;
let never_compat = never_compat::NeverCompatHandler::pre_fallback(&fcx);

// We do fallback in two passes, to try to generate
// better error messages.
Expand Down Expand Up @@ -1110,7 +1164,12 @@ fn typeck_tables_of_with_fallback<'tcx>(
fcx.require_type_is_sized(ty, span, code);
}

fcx.select_all_obligations_or_error();
if !fcx.select_all_obligations_or_error() {
// If we just reported some errors, don't run
// never-type fallbakc - inference variables may
Copy link
Member

Choose a reason for hiding this comment

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

*fallback

// be in a weird state, leading to spurious errors
never_compat.post_fallback(&fcx);
}

if fn_decl.is_some() {
fcx.regionck_fn(id, body);
Expand Down Expand Up @@ -3415,10 +3474,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
true
}

fn select_all_obligations_or_error(&self) {
/// Returns whether or not errors were reported
fn select_all_obligations_or_error(&self) -> bool {
debug!("select_all_obligations_or_error");
if let Err(errors) = self.fulfillment_cx.borrow_mut().select_all_or_error(&self) {
self.report_fulfillment_errors(&errors, self.inh.body_id, false);
true
} else {
false
}
}

Expand Down Expand Up @@ -3624,7 +3687,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.check_argument_types(
sp,
expr,
&err_inputs[..],
err_inputs,
&[],
args_no_rcvr,
false,
Expand Down Expand Up @@ -3732,13 +3795,39 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&self,
sp: Span,
expr: &'tcx hir::Expr<'tcx>,
fn_inputs: &[Ty<'tcx>],
fn_inputs: impl Into<Cow<'tcx, [Ty<'tcx>]>>,
expected_arg_tys: &[Ty<'tcx>],
args: &'tcx [hir::Expr<'tcx>],
c_variadic: bool,
tuple_arguments: TupleArgumentsFlag,
def_span: Option<Span>,
) {
let fn_inputs = fn_inputs.into();
debug!("check_argument_types: storing arguments for expr {:?}", expr);
// We now have the arguments types available for this msthod call,
// so store them in the `inferred_paths` entry for this method call.
// We set `ty` as `None` if we are the first to access the entry
// for this method, and leave it untouched otherwise.
match self.inferred_paths.borrow_mut().entry(expr.hir_id) {
Entry::Vacant(e) => {
debug!("check_argument_types: making new entry for types {:?}", fn_inputs);
e.insert(InferredPath {
span: sp,
ty: None,
args: Some(fn_inputs.clone()),
unresolved_vars: vec![],
});
}
Entry::Occupied(mut e) => {
debug!(
"check_argument_types: modifying existing {:?} with types {:?}",
e.get(),
fn_inputs
);
e.get_mut().args = Some(fn_inputs.clone());
}
}
Comment on lines +3805 to +3829
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto with this bit. Let's move the logic into a new function inside never_compat.


let tcx = self.tcx;
// Grab the argument types, supplying fresh type variables
// if the wrong number of arguments were supplied
Expand Down Expand Up @@ -5425,6 +5514,39 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// the referenced item.
let ty_substituted = self.instantiate_type_scheme(span, &substs, &ty);

if ty_substituted.has_infer_types() {
debug!(
"instantiate_value_path: saving path with infer: ({:?}, {:?})",
span, ty_substituted
);
let parent_id = tcx.hir().get_parent_node(hir_id);
let parent = tcx.hir().get(parent_id);
match parent {
Node::Expr(hir::Expr { span: p_span, kind: ExprKind::Call(..), .. })
| Node::Expr(hir::Expr { span: p_span, kind: ExprKind::MethodCall(..), .. }) => {
// Fill in the type for our parent expression. This might not be
// a method call - if it is, the argumetns will be filled in by
// `check_argument_types`
match self.inferred_paths.borrow_mut().entry(parent_id) {
Entry::Vacant(e) => {
debug!("instantiate_value_path: inserting new path");
e.insert(InferredPath {
span: *p_span,
ty: Some(ty_substituted),
args: None,
unresolved_vars: vec![],
});
}
Entry::Occupied(mut e) => {
debug!("instantiate_value_path: updating existing path {:?}", e.get());
e.get_mut().ty = Some(ty_substituted);
}
}
}
_ => {}
}
}
Comment on lines +5517 to +5548
Copy link
Contributor

Choose a reason for hiding this comment

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

..and let's move this part as well into a new function.


if let Some(UserSelfTy { impl_def_id, self_ty }) = user_self_ty {
// In the case of `Foo<T>::method` and `<Foo<T>>::method`, if `method`
// is inherent, there is no `Self` parameter; instead, the impl needs
Expand Down
Loading