Lint unused labels and types with `fn new() -> Self` and no `Default` impl #730

mcarton · 2016-03-01T15:28:11Z

oli-obk · 2016-03-01T15:40:06Z

src/new_without_default.rs

+            return;
+        }
+
+        if let FnKind::Method(name, _, _) = kind {


this might be a candidate for an if_let_chain!

oli-obk · 2016-03-01T15:49:25Z

src/unused_label.rs

+    fn check_expr(&mut self, _: &LateContext, expr: &hir::Expr) {
+        match expr.node {
+            hir::ExprBreak(Some(label)) | hir::ExprAgain(Some(label)) => {
+                self.used_labels.insert(label.node.name.as_str());


you could just remove the appropriate label from declared_labels, then you just need to iterate over the hash-map at the end instead of indexing it from the values in the hashset

oli-obk · 2016-03-01T16:31:58Z

lgtm

Manishearth · 2016-03-01T16:34:35Z

src/utils/mod.rs

+/// Return whether a method returns `Self`.
+pub fn returns_self(cx: &LateContext, ret: &FunctionRetTy, ty: ty::Ty) -> bool {
+    if let FunctionRetTy::Return(ref ret_ty) = *ret {
+        let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();


we should not use ast_ty_to_ty_cache. We should use node_ty or something else here (@eddyb can help).

I know it was broken before too, but let's fix it.

Well, I just tried ctxt::node_types which returns None and InferCtxt::node_ty{pe,} which panic 😅.

You should get the return type from the function type and not look at the function's AST/HIR at all.

eddyb · 2016-03-01T20:18:12Z

Also, it would be great if you could remove all mentions of TyBareFn because of rust-lang/rust#31710.

mcarton · 2016-03-09T00:09:34Z

Can this be merged now?

Lint unused labels and types with `fn new() -> Self` and no `Default` impl

This delegates our `same_tys` to [ty::TyS::same_type][same_type] to remove some duplication. Our `same_tys` was introduced 4 years ago in rust-lang#730. Before, `same_tys` was building an inference context to be able to call `can_eq` to compare the types. The `rustc-dev-guide` has some more details about `can_eq` in [Type Inference -> Trying equality][try_eq]. Now, using the rustc function, we are essentially comparing the `DefId`s of the given types, which also makes more sense, IMO. I also confirmed that the FIXME is resolved via a bit of `dbg!`, however no UI tests were affected. [same_type]: https://github.com/rust-lang/rust/blob/659951c4a0d7450e43f61c61c0e87d0ceae17087/src/librustc_middle/ty/util.rs#L777 [try_eq]: https://rustc-dev-guide.rust-lang.org/type-inference.html#trying-equality

This replaces our `same_tys` with [ty::TyS::same_type][same_type] to remove some duplication. Our `same_tys` was introduced 4 years ago in rust-lang#730. Before, `same_tys` was building an inference context to be able to call `can_eq` to compare the types. The `rustc-dev-guide` has some more details about `can_eq` in [Type Inference -> Trying equality][try_eq]. Now, using the rustc function, we are essentially comparing the `DefId`s of the given types, which also makes more sense, IMO. I also confirmed that the FIXME is resolved via a bit of `dbg!`, however no UI tests were affected. [same_type]: https://github.com/rust-lang/rust/blob/659951c4a0d7450e43f61c61c0e87d0ceae17087/src/librustc_middle/ty/util.rs#L777 [try_eq]: https://rustc-dev-guide.rust-lang.org/type-inference.html#trying-equality

Cleanup: Use rustc's `same_type` for our `same_tys` This delegates our `same_tys` to [ty::TyS::same_type][same_type] to remove some duplication. Our `same_tys` was introduced 4 years ago in #730. Before, it was building an inference context to be able to call `can_eq` to compare the types. The `rustc-dev-guide` has some more details about `can_eq` in [Type Inference -> Trying equality][try_eq]. Now, using the rustc function, we are essentially comparing the `DefId`s of the given types, which also makes more sense, IMO. I also confirmed that the FIXME is resolved via a bit of `dbg!`, however no UI tests seem to have been affected. [same_type]: https://github.com/rust-lang/rust/blob/659951c4a0d7450e43f61c61c0e87d0ceae17087/src/librustc_middle/ty/util.rs#L777 [try_eq]: https://rustc-dev-guide.rust-lang.org/type-inference.html#trying-equality --- changelog: none

mcarton force-pushed the unused-labels branch 2 times, most recently from 5fabd44 to 8a2e65e Compare March 1, 2016 15:35

oli-obk reviewed Mar 1, 2016
View reviewed changes

src/new_without_default.rs

return;

}

if let FnKind::Method(name, _, _) = kind {

Copy link

Contributor

oli-obk Mar 1, 2016

Choose a reason for hiding this comment

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

this might be a candidate for an if_let_chain!

mcarton force-pushed the unused-labels branch from 8a2e65e to a90e1b5 Compare March 1, 2016 15:45

oli-obk reviewed Mar 1, 2016
View reviewed changes

mcarton force-pushed the unused-labels branch 2 times, most recently from 11524fe to ddb8cb7 Compare March 1, 2016 16:23

Manishearth reviewed Mar 1, 2016
View reviewed changes

mcarton force-pushed the unused-labels branch 6 times, most recently from 87f379d to 7068f0e Compare March 7, 2016 19:20

Lint unused labels

204c12c

mcarton force-pushed the unused-labels branch from 7068f0e to 7368498 Compare March 8, 2016 15:44

mcarton added 3 commits March 8, 2016 17:00

Lint types with fn new() -> Self and no Default impl

d7129f5

Remove uses of ast_ty_to_ty_cache

8e9e858

Fix types comparison

052f598

mcarton force-pushed the unused-labels branch from 7368498 to 052f598 Compare March 8, 2016 16:00

Manishearth added a commit that referenced this pull request Mar 9, 2016

Merge pull request #730 from mcarton/unused-labels

d9b5b2a

Lint unused labels and types with `fn new() -> Self` and no `Default` impl

Manishearth merged commit d9b5b2a into rust-lang:master Mar 9, 2016

mcarton deleted the unused-labels branch March 9, 2016 10:38

phansch mentioned this pull request Apr 25, 2020

Cleanup: Use rustc's same_type for our same_tys #5528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint unused labels and types with `fn new() -> Self` and no `Default` impl #730

Lint unused labels and types with `fn new() -> Self` and no `Default` impl #730

mcarton commented Mar 1, 2016

oli-obk Mar 1, 2016

oli-obk Mar 1, 2016

oli-obk commented Mar 1, 2016

Manishearth Mar 1, 2016

mcarton Mar 1, 2016

eddyb Mar 1, 2016

eddyb commented Mar 1, 2016

mcarton commented Mar 9, 2016

Lint unused labels and types with fn new() -> Self and no Default impl #730

Lint unused labels and types with fn new() -> Self and no Default impl #730

Conversation

mcarton commented Mar 1, 2016

oli-obk Mar 1, 2016

Choose a reason for hiding this comment

oli-obk Mar 1, 2016

Choose a reason for hiding this comment

oli-obk commented Mar 1, 2016

Manishearth Mar 1, 2016

Choose a reason for hiding this comment

mcarton Mar 1, 2016

Choose a reason for hiding this comment

eddyb Mar 1, 2016

Choose a reason for hiding this comment

eddyb commented Mar 1, 2016

mcarton commented Mar 9, 2016

Lint unused labels and types with `fn new() -> Self` and no `Default` impl #730

Lint unused labels and types with `fn new() -> Self` and no `Default` impl #730