Skip to content

Fix ICE when suggesting dereferencing binop operands #119361

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

sjwang05
Copy link
Contributor

Fixes #119352

@rustbot
Copy link
Collaborator

rustbot commented Dec 27, 2023

r? @WaffleLapkin

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 27, 2023
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

@rustbot review

Comment on lines 864 to 867
&& 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
})
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@rustbot review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 28, 2023
@rustbot rustbot 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 (such as code changes or more information) from the author. labels Jan 3, 2024
self.tcx.mk_args(
&[&[l_ty.into(), r_ty.into()], &inner.trait_ref.args[2..]]
.concat(),
self.tcx.mk_args_trait(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@sjwang05 sjwang05 force-pushed the issue-119352 branch 3 times, most recently from c29b565 to 44e5631 Compare January 4, 2024 08:56
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 867 to 871
.filter_map(|&op|
(!matches!(op, LangItem::Neg | LangItem::Not))
.then_some(self.tcx.lang_items().get(op))
.flatten()
)
Copy link
Member

Choose a reason for hiding this comment

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

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))

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2024
@sjwang05
Copy link
Contributor Author

Fixed, thanks.

@rustbot review

@rustbot rustbot 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 (such as code changes or more information) from the author. labels Jan 13, 2024
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

Thanks!

@WaffleLapkin
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jan 14, 2024

📌 Commit c36b5d5 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2024
@bors
Copy link
Collaborator

bors commented Jan 14, 2024

⌛ Testing commit c36b5d5 with merge 8847bda...

@bors
Copy link
Collaborator

bors commented Jan 14, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 14, 2024
@bors bors merged commit 8847bda into rust-lang:master Jan 14, 2024
@rustbot rustbot added this to the 1.77.0 milestone Jan 14, 2024
@rust-timer
Copy link
Collaborator

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%)

@sjwang05 sjwang05 deleted the issue-119352 branch January 16, 2024 03:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: range start index 2 out of range for slice of length 1
7 participants