Fix ICE when suggesting dereferencing binop operands

[sjwang05]

Fixes #119352

[rustbot]

r? @WaffleLapkin

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

[compiler-errors]

This code continues to fail because it's trying to construct a trait predicate for the Sized trait with 2 generic arguments. We should not be trying to perform this code on traits that are not operators.

[sjwang05]

That makes sense, thanks for the help. I've added a check for that.

@rustbot review

[WaffleLapkin]

Diagnostic code should not require language items. Please use a fallible API.

Suggested change

	&& hir::lang_items::OPERATORS.iter().any(|&op| {
	// Ensure we only run this code on operators
	self.tcx.require_lang_item(op, None) == trait_pred.skip_binder().trait_ref.def_id
	})
	// Ensure we only run this code on operators
	&& hir::lang_items::OPERATORS.iter().filter_map(|&op| self.tcx.lang_items().get(op)).any(|op| {
	op == trait_pred.skip_binder().trait_ref.def_id
	})

@rustbot author

[compiler-errors]

This needs to check that it's a binary operator as well. This still would ICE if I had a nested unary operator as a predicate.

[sjwang05]

Diagnostic code should not require language items. Please use a fallible API.

Fixed, thanks for pointing this out.

This still would ICE if I had a nested unary operator as a predicate.

I don't think this is necessarily the case--we already check that the rhs is Some, which would fail if it were a unary op: https://github.com/sjwang05/rust/blob/issue-119352/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs#L856

@rustbot review

[compiler-errors]

I don't think this is necessarily the case--we already check that the rhs is Some

No, you can have a binary operator predicate with a unary operator predicate as a where clause bound.

[sjwang05]

Ahh, I see. I fixed the check to filter out unary ops.

@rustbot review

[compiler-errors]

Why did you use mk_args_trait here? It's harder to read, imo. You should just use mk_args like before. The indexing that was previous here should be correct if we're always using binop traits here.

[sjwang05]

Mainly due to the indexing problem--I reverted it to mk_args.

[WaffleLapkin]

Just a small nitpick and I think we can merge this :)

[WaffleLapkin]

Suggested change

	.filter_map(|&op|
	(!matches!(op, LangItem::Neg | LangItem::Not))
	.then_some(self.tcx.lang_items().get(op))
	.flatten()
	)
	.filter(|op| !matches!(op, LangItem::Neg | LangItem::Not))
	.filter_map(|&op| self.tcx.lang_items().get(op))

[sjwang05]

Fixed, thanks.

@rustbot review

[WaffleLapkin]

Thanks!

[WaffleLapkin]

@bors r+ rollup

[bors]

📌 Commit c36b5d5 has been approved by WaffleLapkin

It is now in the queue for this repository.

[bors]

⌛ Testing commit c36b5d5 with merge 8847bda...

[bors]

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 8847bda to master...

[rust-timer]

Finished benchmarking commit (8847bda): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.181s -> 668.597s (0.06%)
Artifact size: 308.18 MiB -> 308.21 MiB (0.01%)