-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Closed
Changes from 15 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 0c49893
Emit warning
Aaron1011 3a22be9
Improve lint to check substs
Aaron1011 d0dc1e3
Make the error message much better
Aaron1011 ebf1918
Provide a better error when no substs is provided for generic param
Aaron1011 5253bb6
Factor out code into its own module
Aaron1011 087af85
Tweak error message
Aaron1011 932d7c2
Some fixup and cleanup
Aaron1011 10cd2e3
Work on splitting up post_fallback
Aaron1011 a8ce823
More refactoring
Aaron1011 65318da
Add a bunch of comments
Aaron1011 61b879c
Some additional cleanup
Aaron1011 c5691a8
Change warning to error
Aaron1011 1ed39bc
Add some tests
Aaron1011 d4bd422
Fix tidy errors
Aaron1011 f1961b5
Fix failing test
Aaron1011 643a708
Unconditionally enable never-type fallback for Crater
Aaron1011 a81c513
Bail out on inference variables in function arguments
Aaron1011 9eb803a
Run never-compat post_fallback after errors are reported
Aaron1011 78a831f
Bail out if errors have occured
Aaron1011 3aefe61
Never consider built-in constructor fns to be 'questionable'
Aaron1011 409e8d7
Only skip fallback when errors have been reported in the same function
Aaron1011 39af896
Update test output
Aaron1011 c82ea60
Add explicit type annotations for `vec.push(break)` test
Aaron1011 fe77a00
Properly handle missing divering/regular inference variables
Aaron1011 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,7 @@ mod expr; | |
mod generator_interior; | ||
pub mod intrinsic; | ||
pub mod method; | ||
mod never_compat; | ||
mod op; | ||
mod pat; | ||
mod regionck; | ||
|
@@ -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; | ||
|
@@ -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>, | ||
} | ||
|
||
|
@@ -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>>>, | ||
} | ||
|
||
impl<'a, 'tcx> Deref for FnCtxt<'a, 'tcx> { | ||
type Target = Inherited<'a, 'tcx>; | ||
fn deref(&self) -> &Self::Target { | ||
|
@@ -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, | ||
} | ||
} | ||
|
@@ -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. | ||
|
@@ -1095,6 +1149,8 @@ fn typeck_tables_of_with_fallback<'tcx>( | |
// See if we can make any more progress. | ||
fcx.select_obligations_where_possible(fallback_has_occurred, |_| {}); | ||
|
||
never_compat.post_fallback(&fcx); | ||
|
||
// Even though coercion casts provide type hints, we check casts after fallback for | ||
// backwards compatibility. This makes fallback a stronger type hint than a cast coercion. | ||
fcx.check_casts(); | ||
|
@@ -3624,7 +3680,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |
self.check_argument_types( | ||
sp, | ||
expr, | ||
&err_inputs[..], | ||
err_inputs, | ||
&[], | ||
args_no_rcvr, | ||
false, | ||
|
@@ -3732,13 +3788,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
let tcx = self.tcx; | ||
// Grab the argument types, supplying fresh type variables | ||
// if the wrong number of arguments were supplied | ||
|
@@ -5425,6 +5507,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.