add comparison operators to must-use lint (under fn_must_use feature)

[zackmdavis]

Although RFC 1940 is about annotating functions with #[must_use], a
key part of the motivation was linting unused equality operators.

(See
https://github.com/rust-lang/rfcs/pull/1812#issuecomment-265695898—it
seems to have not been clear to discussants at the time that marking the
comparison methods as must_use would not give us the lints on
comparison operators, at least in (what the present author understood
as) the most straightforward implementation, as landed in #43728
(3645b06).)

To rectify the situation, we here lint unused comparison operators as
part of the unused-must-use lint (feature gated by the fn_must_use
feature flag, which now arguably becomes a slight (tolerable in the
opinion of the present author) misnomer).

This is in the matter of #43302.

cc @crumblingstatue

[rust-highfive]

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

[oli-obk]

I think it would be less surprising, if instead of whitelisting operators, we'd check whether the corresponding operator method has the attribute.

[oli-obk]

The result of the comparison is unused. The span should be including the rhs and lhs of the operation

[pnkfelix]

r=pnkfelix once @oli-obk 's points are addressed.

[arielb1]

Hi @zackmdavis. Looks like you just need to make a few changes to get this over to ship. Just a friendly ping to keep this on your radar.

[zackmdavis]

@arielb1 Yes, thanks: I spent a couple hours last week trying to figure out how to look up the method definition given the binary-operator HIR node, and couldn't figure out how to make it work; I want to take another look sometime this week before I stoop to the indignity of begging for mentoring.

[oli-obk]

You can get the DefId and Substs of the correct method via cx.tables().type_dependent_defs()[expr.hir_id].def_id() and cx.tables().node_substs(expr.hir_id).

To obtain the attributes, you can check if cx.tcx.item_attrs(def_id) contains everything you need.

indignity of begging for mentoring.

I'm horrified we present the image that any form of mentoring requires begging or is indignified. Please don't hesitate to ask for help either on irc, here or on the internals forum, no matter how trivial you think the topic might be. It's likely it's not that trivial (like in this case, it's actually impossible to obtain the method's HIR in general, because the method is very likely in another crate (e.g. libstd or libcore)).

[aidanhs]

Hi @zackmdavis, just wanted to check in to make sure this PR is still on your radar! Please do reach out for a hand via the methods @oli-obk mentioned if it'd help at all.

[zackmdavis]

Regret the delay; I'll set aside some time Friday. (I swear I tried type_dependent_defs the other week before oli recommended it, and it didn't work because the table was empty; I fear that the emotional scars of that profoundly frustrating debugging session may have contributed to my tardiness.)

[zackmdavis]

I see—if both operands are scalars, we don't fill type_dependent_defs. (And if the operands are custom structs with a PartialEq implementation, looking at type_dependenent_defs and finding the must_use attribute on PartialEq::eq does work.) Given this, it's not clear to me that we can do better than whitelisting the HIR binary comparison operators, as the existing revision of this PR does. @oli-obk thoughts?

(Existing PR revision will collide with #44620, which I expect to be rolled up.)

[zackmdavis]

I'm horrified we present the image that any form of mentoring requires begging or is indignified

We don't; please construe that somewhat tongue-in-cheek remark as reflecting my personal (and, one could argue, neurotic and maladaptive) pride in my ability to RTFM.

[oli-obk]

Hmm... I can't figure out why we remove scalars from the map... not even sure who to ping. That code is older than two years

[zackmdavis]

I can't figure out why we remove scalars from the map... not even sure who to ping.

Looks like d649292. The pull request (by @nikomatsakis) says:

There is one semi-hacky bit in that during writeback, for builtin operators, if the types resolve to scalars (i32 etc) then we clear the method override. This is because we know what the semantics are and it is more efficient to generate the code directly. It also ensures that we can use these overloaded operators in constants and so forth.

[arielb1]

Ah yeah that. I myself don't see a problem with keeping an hard list of binary operators.

[arielb1]

If you wanted to avoid the hardcoding, you could move this part of lookup_op_method to librustc:

rust/src/librustc_typeck/check/op.rs

Lines 362 to 421 in ef227f5

	fn lookup_op_method(&self, lhs_ty: Ty<'tcx>, other_tys: &[Ty<'tcx>], op: Op)
	-> Result<MethodCallee<'tcx>, ()>
	{
	let lang = self.tcx.lang_items();
	let span = match op {
	Op::Binary(op, _) => op.span,
	Op::Unary(_, span) => span
	};
	let (opname, trait_did) = if let Op::Binary(op, IsAssign::Yes) = op {
	match op.node {
	hir::BiAdd => ("add_assign", lang.add_assign_trait()),
	hir::BiSub => ("sub_assign", lang.sub_assign_trait()),
	hir::BiMul => ("mul_assign", lang.mul_assign_trait()),
	hir::BiDiv => ("div_assign", lang.div_assign_trait()),
	hir::BiRem => ("rem_assign", lang.rem_assign_trait()),
	hir::BiBitXor => ("bitxor_assign", lang.bitxor_assign_trait()),
	hir::BiBitAnd => ("bitand_assign", lang.bitand_assign_trait()),
	hir::BiBitOr => ("bitor_assign", lang.bitor_assign_trait()),
	hir::BiShl => ("shl_assign", lang.shl_assign_trait()),
	hir::BiShr => ("shr_assign", lang.shr_assign_trait()),
	hir::BiLt | hir::BiLe |
	hir::BiGe | hir::BiGt |
	hir::BiEq | hir::BiNe |
	hir::BiAnd | hir::BiOr => {
	span_bug!(span,
	"impossible assignment operation: {}=",
	op.node.as_str())
	}
	}
	} else if let Op::Binary(op, IsAssign::No) = op {
	match op.node {
	hir::BiAdd => ("add", lang.add_trait()),
	hir::BiSub => ("sub", lang.sub_trait()),
	hir::BiMul => ("mul", lang.mul_trait()),
	hir::BiDiv => ("div", lang.div_trait()),
	hir::BiRem => ("rem", lang.rem_trait()),
	hir::BiBitXor => ("bitxor", lang.bitxor_trait()),
	hir::BiBitAnd => ("bitand", lang.bitand_trait()),
	hir::BiBitOr => ("bitor", lang.bitor_trait()),
	hir::BiShl => ("shl", lang.shl_trait()),
	hir::BiShr => ("shr", lang.shr_trait()),
	hir::BiLt => ("lt", lang.ord_trait()),
	hir::BiLe => ("le", lang.ord_trait()),
	hir::BiGe => ("ge", lang.ord_trait()),
	hir::BiGt => ("gt", lang.ord_trait()),
	hir::BiEq => ("eq", lang.eq_trait()),
	hir::BiNe => ("ne", lang.eq_trait()),
	hir::BiAnd | hir::BiOr => {
	span_bug!(span, "&& and || are not overloadable")
	}
	}
	} else if let Op::Unary(hir::UnNot, _) = op {
	("not", lang.not_trait())
	} else if let Op::Unary(hir::UnNeg, _) = op {
	("neg", lang.neg_trait())
	} else {
	bug!("lookup_op_method: op not supported: {:?}", op)
	};

and then call it on the binary operator to get the defid of the op method.

[bors]

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

[zackmdavis]

(rebased) fixed comparison expression span, conflict with master

[zackmdavis]

I myself don't see a problem with keeping an hard list of binary operators.

Anyone else have an opinion? My disposition is to merge the most recent revision that I just pushed that leaves the hardcoding (with a comment noting the judgement call).

@arielb1's idea of pulling code out of rustc_typecheck (and putting it onto, say, TyCtxt) would work, but it seems like an unnatural refactoring for no actual gain in functionality. (Looking up the must_use attribute on the trait method would be more graceful æsthetically, but it doesn't come for free, and if we already know that we want to lint comparisons, the dynamicity isn't necessary.)

[oli-obk]

I'm fine with hardcoding, as long as there are sufficient comments explaining the haedcoding. And as long as it works for nonprimitives with the trait attribute check.

[arielb1]

Nice!

@bors r+

[bors]

📌 Commit 6734d39 has been approved by arielb1