rework use_self impl based on ty::Ty comparison #3410 #5531

tnielens · 2020-04-26T00:53:42Z

use_self lint refactoring. As suggested by @eddyb , implement the lint based on ty::Ty comparison instead of hir::Path checks.

This PR introduces negative regressions. The cases were covered in the previous implementation. See the test file.

It fixes #3410, #2843, #3859, #4734 and #4140. Should fix #4143 (example doesn't compile). #4305 false positive seems also fixed but the uitest fails to compile the unmodified test case in use_self.rs.fixed. Implements #5078.
Generally the implementation checks are done at mir level and offers higher guarantees against false positives. It is probably fixing other unreported false positives.

changelog: fix use_self generics false positives

clippy_lints/src/use_self.rs

tests/ui/use_self.fixed

phansch · 2020-04-27T06:00:18Z

clippy_lints/src/use_self.rs

                if should_check {
-                    let visitor = &mut UseSelfVisitor {
-                        item_path,
+                    let self_ty= hir_ty_to_ty(cx.tcx, item_type);


My guess is, that this is causing the ICE in could_be_const.rs. It's probably some wrong item_type being passed in, but I'm not sure beyond that

Indeed, commenting sections making use of this seems to solve failing test cases.

Could you help me out on this?

I've been trying to debug this without much success. The backtraces are not very helpful and the errors are raised from rustc location I have no understanding of. What is the hir_ty_to_ty contract? Under which circumstances could this call lead to errors elsewhere?

Sure, I can try =) Beyond that, I'm not really sure about how to use hir_ty_to_ty properly and it's not documented either.
I can see two ways to move a bit forward though:

Could you try not doing the conversion for def::Res::SelfTy? That would look like this:

rust-clippy/clippy_lints/src/use_self.rs

Lines 94 to 98 in f2486b3

if let TyKind::Path(QPath::Resolved(_, path)) = &hir_ty.kind {

match path.res {

def::Res::SelfTy(..) => {},

_ => {

if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty {

dbg! as much as possible around this location and figure out if there's something that looks odd

I think the problem is located in the visit_ty method of the visitor. See line 167 mentioned in another comment hereunder.

flip1995

If you need more help finding a solution for those weird ICEs, let me know and I'll take a closer look.

flip1995 · 2020-04-28T12:32:54Z

clippy_lints/src/use_self.rs

+                            // @flip1995 found an ast lowering issue in
+                            // https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#L142-L162
+                            {
+                                span_lint(self.cx, hir_ty.span.with_hi(last_segment.ident.span.lo() - BytePos(2)))


As per discussion on Discord: Have you tried the SourceMap::span_until_char method?

Not yet :). On the todo list.

Span handling improved. See truncate_last_segment fn.

flip1995 · 2020-04-28T12:42:40Z

clippy_lints/src/use_self.rs

+            cx.tcx
+                .typeck_tables_of(expr.hir_id.owner.to_def_id())
+                .node_type(expr.hir_id)


~~Is there a reason, why you don't use cx.tables.expr_ty{_opt}(expr)?~~ Nevermind, this is in a visitor, so you need the typeck_tables_of call.

This is responsible for the ICE in ui/missing_const_for_fn/could_be_const.rs. Try expr_ty_opt here to possibly prevent this.

Oh, what I forget to tell you, is that you might call cx.tcx.has_typeck_tables(def_id) before trying to get the typeck tables. At least this is how it's done in most of the rest of Clippy.

Thanks for the cx.tcx.has_typeck_tables(def_id) tip. I'll give it a try.
typeck_tables_of is documented in the rustc guide. I tried expr_ty without success, the tests panicked.

If expr_ty panics, node_ty will also panic, since expr_ty calls node_ty. You have to use if let Some(ty) = tcx.typeck_tables_of(def_id).expr_ty_opt(expr), but first make sure to check cx.tcx.has_typeck_tables(def_id).

I should have protected all occurrences of type expression queries with has_typeck_tables. The problem remains.

clippy_lints/src/use_self.rs

tnielens · 2020-04-28T20:10:42Z

clippy_lints/src/use_self.rs

+    }
+
+    fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'tcx>) {
+        if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind {
            match path.res {
                def::Res::SelfTy(..) => {},
                _ => {
                    if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty {


Commenting out the block starting at line 167 makes the other tests pass. Commenting the inner block starting at the next line 168 instead makes them fail again.

I could use a little help on this one, I'm a bit clueless :).

So this call results in a delay_span_bug here: https://github.com/rust-lang/rust/blob/e91aebc1a3835b9b420da0c021e211175a724b8d/src/librustc_typeck/collect.rs#L309-L312

But this method can only be called, if hir_ty.kind == hir::TyKind::Infer (Code). Since we're checking that the hir_ty.kind == Path earlier, this doesn't make any sense.

But it gets weirder. if we insert a dbg!(hir_ty) or println!("{:?}", hir_ty) it doesn't panic anymore and the hir_ty.kind is always printed as TyKind::Path(Resolved)...

In fact, even a print!("x"); before this line makes the ICE go away. It has to print something though, print!(""); won't work.

Since printf debugging won't help here, I will investigate this further with gdb.

Two backtraces of the ICEs for the errors

"bad placeholder type" (eta.rs, question_mark.rs)

error: internal compiler error: src/librustc_typeck/collect.rs:311: bad placeholder type --> tests/ui/eta.rs:116:15 | 116 | Thunk(Box::new(move || option.take().unwrap()())) | ^^^^^^^^ thread 'rustc' panicked at 'Box<Any>', /home/pkrones/rust/src/libstd/macros.rs:13:23 stack backtrace: 0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt 1: core::fmt::write 2: std::io::Write::write_fmt 3: std::sys_common::backtrace::print 4: std::panicking::default_hook::{{closure}} 5: std::panicking::default_hook 6: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call at /home/pkrones/rust/src/liballoc/boxed.rs:1048 7: clippy_driver::report_clippy_ice at src/driver.rs:242 8: <clippy_driver::ICE_HOOK as core::ops::deref::Deref>::deref::__static_ref_initialize::{{closure}} at src/driver.rs:235 9: std::panicking::rust_panic_with_hook 10: std::panicking::begin_panic 11: rustc_errors::HandlerInner::span_bug 12: rustc_errors::Handler::span_bug 13: rustc_middle::util::bug::opt_span_bug_fmt::{{closure}} 14: rustc_middle::ty::context::tls::with_opt::{{closure}} 15: rustc_middle::ty::context::tls::with_opt 16: rustc_middle::util::bug::opt_span_bug_fmt 17: rustc_middle::util::bug::span_bug_fmt 18: <rustc_typeck::collect::ItemCtxt as rustc_typeck::astconv::AstConv>::ty_infer 19: <dyn rustc_typeck::astconv::AstConv>::create_substs_for_ast_path::{{closure}} 20: <dyn rustc_typeck::astconv::AstConv>::create_substs_for_generic_args 21: <dyn rustc_typeck::astconv::AstConv>::create_substs_for_ast_path 22: <dyn rustc_typeck::astconv::AstConv>::ast_path_substs_for_ty 23: <dyn rustc_typeck::astconv::AstConv>::res_to_ty 24: <dyn rustc_typeck::astconv::AstConv>::ast_ty_to_ty 25: rustc_typeck::hir_ty_to_ty 26: <clippy_lints::use_self::ImplVisitor as rustc_hir::intravisit::Visitor>::visit_ty at clippy_lints/src/use_self.rs:166 ...

"unelided lifetime in signature" (crashes/ice-4775.rs, expect_fun_call.rs, unit_arg.rs)

error: internal compiler error: src/librustc_typeck/astconv.rs:182: unelided lifetime in signature --> /home/pkrones/rust/src/libstd/macros.rs:97:28 | 97 | $crate::io::_print($crate::format_args_nl!($($arg)*)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ thread 'rustc' panicked at 'Box<Any>', /home/pkrones/rust/src/libstd/macros.rs:13:23 stack backtrace: 0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt 1: core::fmt::write 2: std::io::Write::write_fmt 3: std::sys_common::backtrace::print 4: std::panicking::default_hook::{{closure}} 5: std::panicking::default_hook 6: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call at /home/pkrones/rust/src/liballoc/boxed.rs:1048 7: clippy_driver::report_clippy_ice at src/driver.rs:242 8: <clippy_driver::ICE_HOOK as core::ops::deref::Deref>::deref::__static_ref_initialize::{{closure}} at src/driver.rs:235 9: std::panicking::rust_panic_with_hook 10: std::panicking::begin_panic 11: rustc_errors::HandlerInner::span_bug 12: rustc_errors::Handler::span_bug 13: rustc_middle::util::bug::opt_span_bug_fmt::{{closure}} 14: rustc_middle::ty::context::tls::with_opt::{{closure}} 15: rustc_middle::ty::context::tls::with_opt 16: rustc_middle::util::bug::opt_span_bug_fmt 17: rustc_middle::util::bug::span_bug_fmt 18: <dyn rustc_typeck::astconv::AstConv>::ast_region_to_region::{{closure}} 19: <dyn rustc_typeck::astconv::AstConv>::ast_region_to_region 20: <dyn rustc_typeck::astconv::AstConv>::create_substs_for_generic_args 21: <dyn rustc_typeck::astconv::AstConv>::create_substs_for_ast_path 22: <dyn rustc_typeck::astconv::AstConv>::ast_path_substs_for_ty 23: <dyn rustc_typeck::astconv::AstConv>::res_to_ty 24: <dyn rustc_typeck::astconv::AstConv>::ast_ty_to_ty 25: rustc_typeck::hir_ty_to_ty 26: <clippy_lints::use_self::ImplVisitor as rustc_hir::intravisit::Visitor>::visit_ty at clippy_lints/src/use_self.rs:166 ...

@flip1995 is this a misuse of the api or a compiler bug?

Quite a few. I committed the hack and flagged all FN in the ui test use_self.rs file with a FIXME comment.

I left a commented block explaining the problem and indicating which code to use in order to resolve most false negatives. We could open a follow-up issue to track the unresolved ICEs.

I think we're better off with those false negatives rather than the false positives of the previous implementation. The lint will catch less cases, but will be more accurate.

We could open a follow-up issue to track the unresolved ICEs.

We can't merge a PR that introduces ICEs. Did you mean for the FPs?

I think we're better off with those false negatives rather than the false positives of the previous implementation. The lint will catch less cases, but will be more accurate.

Agreed ✔️

We can't merge a PR that introduces ICEs. Did you mean for the FPs?

In the current patch state that passes all tests, a block of the implementation is commented out to avoid ICEs. We could create a follow-up ticket to investigate this and try to make that part work without errors. That would solve most FNs.

Oh now the dogfood_clippy test throws compiler errors 😞.

That sadly tells us, that it is not possible to fix the ICEs by excluding specific TyKinds. I kind of expected that, since TyKind is a recursive enum, so just checking the top level type doesn't really help, since some subtype could still be problematic.

I will look in to this closer, when I have some time. I hope I get to it rather sooner than later.

eddyb · 2020-05-01T06:24:37Z

clippy_lints/src/use_self.rs

+fn truncate_last_segment<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, sp: Span) -> Span {
+    match snippet_opt(cx, sp) {
+        Some(snippet) => match snippet.rfind("::") {
+            Some(bidx) => sp.with_hi(sp.lo() + BytePos(bidx as u32)),
+            None => sp,
+        },
+        None => sp,
+    }
+}


This seems unwise when paths can have generic args anywhere. You probably want to instead rely on the fact that segments have their own spans (AFAIK) so you can check that the path's overall span and the span of the last segment have the same hi and then use that to "carve out" the last segment.

I've tried to depend more on paths subelements own span. I had issues with QPath::TypeRelative. @flip1995 spotted issues in the ast lowering. See the associated comment in the patch line 172 and the discussion in discord.

But agreed that this will fail upon generics nested paths. I'll add a test case and look for options to deal with that.

Span handling improved. See span_lint_until_last_segment. I added a nested path test case. It fails to compile due to the reason @flip1995 analyzed above.

bors · 2020-07-03T15:12:39Z

☔ The latest upstream changes (presumably #5763) made this pull request unmergeable. Please resolve the merge conflicts.

tnielens · 2020-10-10T22:43:32Z

I think I found the problem with hir_ty_to_ty. Its implementation is based on ItemCtx wich is part of the rustc_typeck collect module. This module is meant for item-like scanning and information collection. So its usage works fine on function signatures but isn't supposed to work on function bodies. Somehow it worked fine in some cases such as Foo::new() but it caused issues on other cases. One issue example is the src/librustc_typeck/collect.rs:311: bad placeholder type case:

    fn ty_infer(&self, _: Option<&ty::GenericParamDef>, span: Span) -> Ty<'tcx> {
        self.tcx().ty_error_with_message(span, "bad_placeholder_type")
    }

where the ItemCtxt doesn't support certain methods of the trait AstConv it implements.

So I changed use_self impl and didn't use the hir_ty_to_ty on fn bodies. That cause a couple of FN regressions which I marked with FIXMEs in the test file. But the ty::Ty comparison finally passes the test.

I propose this pr is merged like this (with another round of review). The newly introduced FNs can be addressed in a next iteration.

cc @flip1995

flip1995 · 2020-10-13T22:48:26Z

Oh man I never got back to this PR... Thanks for looking into it again!

Let's do a @bors try run. I'll review tomorrow, it's already 1am again...

bors · 2020-10-13T22:48:38Z

⌛ Trying commit f4c3d47 with merge a12ef13...

…itive, r=<try> rework use_self impl based on ty::Ty comparison #3410 `use_self` lint refactoring. As suggested by `@eddyb` , implement the lint based on `ty::Ty` comparison instead of `hir::Path` checks. This PR introduces negative regressions. The cases were covered in the previous implementation. See the test file. It fixes #3410, #2843, #3859, #4734 and #4140. Should fix #4143 (example doesn't compile). #4305 false positive seems also fixed but the uitest fails to compile the unmodified test case in `use_self.rs.fixed`. Implements #5078. Generally the implementation checks are done at mir level and offers higher guarantees against false positives. It is probably fixing other unreported false positives. changelog: fix use_self generics false positives

bors · 2020-10-13T23:09:13Z

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: a12ef13 (a12ef13605840b034ca2340667800a834806d831)

flip1995

Hm with your description of the error behind hir_ty_to_ty I guess, that we're just in the completely wrong ItemCtx, because we're using visitors with the context from some impl item.

What if we don't use visitors, but use the lint framework and every time we encounter an ItemKind::Impl, we push the self_ty on the stack and if we leave the ItemKind::Impl, we pop the self_ty from the stack. I'll try if that works.

flip1995 · 2020-10-15T14:20:11Z

clippy_lints/src/use_self.rs

+                            // The following block correctly identifies applicable lint locations
+                            // but `hir_ty_to_ty` calls cause odd ICEs.
+                            //


Is this comment still applicable here?

No, I'll remove it.

flip1995 · 2020-10-15T17:38:12Z

So my idea to rewrite this and removing the visitors seems to work. My rewrite is at the state, that it fixed all the FIXMEs that you added. Now I have to fix some FPs I didn't yet address in my prototype rewrite.

flip1995 · 2020-10-16T12:11:54Z

Closing in favor of #6179.

@montrivo Your commits will of course be merged, so your work will not get thrown away.

The only issue left to fix is now #4140. I would be happy to work this out together with you in #6179. If you want to work on my branch, I can give you push access to my fork.

Rework use_self impl based on ty::Ty comparison #3410 | Take 2 This builds on top of #5531 I already reviewed and approved the commits by `@montrivo.` So only the review of my commits should be necessary. I would also appreciate your review `@montrivo,` since you are familiar with the challenges here. Fixes #3410 and Fixes #4143 (same problem) Fixes #2843 Fixes #3859 Fixes #4734 and fixes #6221 Fixes #4305 Fixes #5078 (even at expression level now 🎉) Fixes #3881 and Fixes #4887 (same problem) Fixes #3909 Not yet: #4140 (test added) All the credit for the fixes goes to `@montrivo.` I only refactored and copy and pasted his code. changelog: rewrite [`use_self`] lint and fix multiple (8) FPs. One to go.

tnielens commented Apr 26, 2020

View reviewed changes

clippy_lints/src/use_self.rs Show resolved Hide resolved

clippy_lints/src/use_self.rs Show resolved Hide resolved

tests/ui/use_self.fixed Show resolved Hide resolved

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch from 7bbea69 to 15440bb Compare April 26, 2020 14:57

tnielens changed the title ~~WIP: rework use_self impl based on ty::Ty comparison #3410~~ rework use_self impl based on ty::Ty comparison #3410 Apr 26, 2020

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch 4 times, most recently from ae774b4 to 5020fcf Compare April 26, 2020 21:25

flip1995 self-requested a review April 27, 2020 02:21

flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 27, 2020

phansch reviewed Apr 27, 2020

View reviewed changes

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch from edf40c4 to 6853f11 Compare April 27, 2020 21:48

flip1995 reviewed Apr 28, 2020

View reviewed changes

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 28, 2020

flip1995 reviewed Apr 28, 2020

View reviewed changes

clippy_lints/src/use_self.rs Outdated Show resolved Hide resolved

tnielens commented Apr 28, 2020

View reviewed changes

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch 2 times, most recently from 77c4a1f to a7a2bc7 Compare April 29, 2020 22:10

eddyb reviewed May 1, 2020

View reviewed changes

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch 5 times, most recently from b3a4c24 to 6ad21e7 Compare May 6, 2020 21:34

flip1995 self-requested a review May 7, 2020 15:37

flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 25, 2020

flip1995 self-assigned this May 25, 2020

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch from 6ad21e7 to 69afa6f Compare June 20, 2020 22:53

flip1995 added the E-help-wanted Call for participation: Help is requested to fix this issue. label Jul 3, 2020

rework use_self impl based on ty::Ty comparison

fc7ee75

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch from 69afa6f to fc7ee75 Compare October 9, 2020 22:22

use_self - fix issue with hir_ty_to_ty

f4c3d47

tnielens force-pushed the bugfix/3410-use_self_generic_false_positive branch from 070317a to f4c3d47 Compare October 10, 2020 22:47

flip1995 reviewed Oct 15, 2020

View reviewed changes

use_self / remove outdated comment

770b612

flip1995 mentioned this pull request Oct 16, 2020

Rework use_self impl based on ty::Ty comparison #3410 | Take 2 #6179

Merged

flip1995 closed this Oct 16, 2020

	if let TyKind::Path(QPath::Resolved(_, path)) = &hir_ty.kind {
	match path.res {
	def::Res::SelfTy(..) => {},
	_ => {
	if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty {

rework use_self impl based on ty::Ty comparison #3410 #5531

rework use_self impl based on ty::Ty comparison #3410 #5531

Conversation

tnielens commented Apr 26, 2020 • edited Loading

phansch Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnielens Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnielens Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

tnielens Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnielens May 2, 2020 • edited Loading

Choose a reason for hiding this comment

tnielens May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnielens May 6, 2020 • edited Loading

Choose a reason for hiding this comment

tnielens May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 3, 2020

tnielens commented Oct 10, 2020 • edited Loading

flip1995 commented Oct 13, 2020

bors commented Oct 13, 2020

bors commented Oct 13, 2020

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 commented Oct 15, 2020

flip1995 commented Oct 16, 2020

tnielens commented Apr 26, 2020 •

edited

Loading

phansch Apr 27, 2020 •

edited

Loading

flip1995 Apr 28, 2020 •

edited

Loading

tnielens Apr 28, 2020 •

edited

Loading

tnielens Apr 29, 2020 •

edited

Loading

tnielens Apr 28, 2020 •

edited

Loading

flip1995 Apr 29, 2020 •

edited

Loading

tnielens May 2, 2020 •

edited

Loading

tnielens May 5, 2020 •

edited

Loading

tnielens May 6, 2020 •

edited

Loading

tnielens May 6, 2020 •

edited

Loading

tnielens commented Oct 10, 2020 •

edited

Loading