Ensure `callee_id`s are body owners #120000

smoelius · 2024-01-15T17:47:14Z

This PR makes the callee_id argument of Clippy's implements_trait_with_env optional, and when it is passed, ensures it is a body owner.

#118661 added the callee_id parameter to alleviate an ICE. Specifically, the callee_id is used to determine an "effect arg" in certain situations.

Frankly, I do not completely understand what an "effect arg" is. But the code that determines it seems to require that callee_id is a body owner:

rust/src/tools/clippy/clippy_utils/src/ty.rs

Lines 286 to 288 in 1ead476

    
           Some(GenericArg::from(callee_id.map_or(tcx.consts.true_, |def_id| { 
        
               tcx.expected_host_effect_param_for_body(def_id) 
        
           })))

rust/compiler/rustc_middle/src/ty/util.rs

Line 834 in 1ead476

let const_context = self.hir().body_const_context(def_id);
rust/compiler/rustc_middle/src/hir/map/mod.rs

Line 372 in 1ead476

let ccx = match self.body_owner_kind(def_id) {

In the current head, some def ids passed as callee_ids are not body owners. This PR fixes that.

cc @rust-lang/clippy

r? @fee1-dead

rustbot · 2024-01-15T17:47:23Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

blyxyas · 2024-01-15T17:53:10Z

Could this be opened as a PR directly to Clippy?

smoelius · 2024-01-15T18:11:00Z

Could this be opened as a PR directly to Clippy?

I'm not sure, TBH.

#118661 was largely unrelated to Clippy. But the way that Clippy called certain APIs ICEd following that PR's changes.

I implemented the follow-up changes here to:

Ensure I didn't reintroduce ICEs.
Get more eyes on my changes, especially those that understand "effect arg"s.

cjgillot · 2024-01-15T23:36:54Z

src/tools/clippy/clippy_utils/src/ty.rs

@@ -214,36 +214,21 @@ pub fn implements_trait<'tcx>(
    trait_id: DefId,
    args: &[GenericArg<'tcx>],
 ) -> bool {
-    let callee_id = cx
-        .enclosing_body
-        .map(|body| cx.tcx.hir().body_owner(body).owner.to_def_id());


Using body_owner.owner is almost never the right thing to do. Would using body_owner_def_id solve the issue directly?

Please forgive me but is that question directed towards me? Please note that the code you've cited is deleted by this PR.

fee1-dead

mostly LGTM, suggested some comment changes to clarify

src/tools/clippy/clippy_utils/src/ty.rs

Co-authored-by: fee1-dead <ent3rm4n@gmail.com>

smoelius · 2024-01-18T15:33:52Z

mostly LGTM, suggested some comment changes to clarify

The additional detail is much appreciated!

fee1-dead · 2024-01-20T15:23:21Z

@bors r+

bors · 2024-01-20T15:23:24Z

📌 Commit 96b8eb7 has been approved by fee1-dead

It is now in the queue for this repository.

Ensure `callee_id`s are body owners This PR makes the `callee_id` argument of Clippy's `implements_trait_with_env` optional, and when it is passed, ensures it is a body owner. rust-lang#118661 added the `callee_id` parameter to alleviate an ICE. Specifically, the `callee_id` is used to determine an "effect arg" in certain situations. Frankly, I [do not completely understand](rust-lang#118661 (comment)) what an "effect arg" is. But the code that determines it seems to require that `callee_id` is a body owner: - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/src/tools/clippy/clippy_utils/src/ty.rs#L286-L288 - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/compiler/rustc_middle/src/ty/util.rs#L834 - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/compiler/rustc_middle/src/hir/map/mod.rs#L372 In the current head, some def ids passed as `callee_id`s are not body owners. This PR fixes that. cc `@rust-lang/clippy` r? `@fee1-dead`

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#118257 (Make traits / trait methods detected by the dead code lint) - rust-lang#119997 (Fix impl stripped in rustdoc HTML whereas it should not be in case the impl is implemented on a type alias) - rust-lang#120000 (Ensure `callee_id`s are body owners) - rust-lang#120015 (coverage: Format all coverage tests with `rustfmt`) - rust-lang#120063 (Remove special handling of `box` expressions from parser) - rust-lang#120138 (Increase vscode settings.json `git.detectSubmodulesLimit`) r? `@ghost` `@rustbot` modify labels: rollup

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#119997 (Fix impl stripped in rustdoc HTML whereas it should not be in case the impl is implemented on a type alias) - rust-lang#120000 (Ensure `callee_id`s are body owners) - rust-lang#120063 (Remove special handling of `box` expressions from parser) - rust-lang#120116 (Remove alignment-changing in-place collect) - rust-lang#120138 (Increase vscode settings.json `git.detectSubmodulesLimit`) - rust-lang#120169 (Spelling fix) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120000 - smoelius:fix-clippy, r=fee1-dead Ensure `callee_id`s are body owners This PR makes the `callee_id` argument of Clippy's `implements_trait_with_env` optional, and when it is passed, ensures it is a body owner. rust-lang#118661 added the `callee_id` parameter to alleviate an ICE. Specifically, the `callee_id` is used to determine an "effect arg" in certain situations. Frankly, I [do not completely understand](rust-lang#118661 (comment)) what an "effect arg" is. But the code that determines it seems to require that `callee_id` is a body owner: - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/src/tools/clippy/clippy_utils/src/ty.rs#L286-L288 - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/compiler/rustc_middle/src/ty/util.rs#L834 - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/compiler/rustc_middle/src/hir/map/mod.rs#L372 In the current head, some def ids passed as `callee_id`s are not body owners. This PR fixes that. cc ``@rust-lang/clippy`` r? ``@fee1-dead``

Ensure `callee_id`s are body owners This PR makes the `callee_id` argument of Clippy's `implements_trait_with_env` optional, and when it is passed, ensures it is a body owner. rust-lang#118661 added the `callee_id` parameter to alleviate an ICE. Specifically, the `callee_id` is used to determine an "effect arg" in certain situations. Frankly, I [do not completely understand](rust-lang#118661 (comment)) what an "effect arg" is. But the code that determines it seems to require that `callee_id` is a body owner: - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/src/tools/clippy/clippy_utils/src/ty.rs#L286-L288 - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/compiler/rustc_middle/src/ty/util.rs#L834 - https://github.com/rust-lang/rust/blob/1ead4761e9e2f056385768614c23ffa7acb6a19e/compiler/rustc_middle/src/hir/map/mod.rs#L372 In the current head, some def ids passed as `callee_id`s are not body owners. This PR fixes that. cc ``@rust-lang/clippy`` r? ``@fee1-dead``

Ensure callee_ids are body owners

65409c4

rustbot assigned fee1-dead Jan 15, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2024

cjgillot reviewed Jan 15, 2024

View reviewed changes

fee1-dead reviewed Jan 18, 2024

View reviewed changes

src/tools/clippy/clippy_utils/src/ty.rs Outdated Show resolved Hide resolved

src/tools/clippy/clippy_utils/src/ty.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

96b8eb7

Co-authored-by: fee1-dead <ent3rm4n@gmail.com>

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 20, 2024

GuillaumeGomez mentioned this pull request Jan 20, 2024

Rollup of 6 pull requests #120163

Closed

GuillaumeGomez mentioned this pull request Jan 20, 2024

Rollup of 6 pull requests #120170

Merged

bors merged commit 71cef76 into rust-lang:master Jan 20, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 20, 2024

smoelius deleted the fix-clippy branch January 20, 2024 22:26

rust-timer mentioned this pull request Jan 26, 2024

Rollup of 9 pull requests #120375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure `callee_id`s are body owners #120000

Ensure `callee_id`s are body owners #120000

smoelius commented Jan 15, 2024

rustbot commented Jan 15, 2024

blyxyas commented Jan 15, 2024

smoelius commented Jan 15, 2024

cjgillot Jan 15, 2024

smoelius Jan 16, 2024

fee1-dead left a comment

smoelius commented Jan 18, 2024

fee1-dead commented Jan 20, 2024

bors commented Jan 20, 2024

	Some(GenericArg::from(callee_id.map_or(tcx.consts.true_, \|def_id\| {
	tcx.expected_host_effect_param_for_body(def_id)
	})))

Ensure callee_ids are body owners #120000

Ensure callee_ids are body owners #120000

Conversation

smoelius commented Jan 15, 2024

rustbot commented Jan 15, 2024

blyxyas commented Jan 15, 2024

smoelius commented Jan 15, 2024

cjgillot Jan 15, 2024

Choose a reason for hiding this comment

smoelius Jan 16, 2024

Choose a reason for hiding this comment

fee1-dead left a comment

Choose a reason for hiding this comment

smoelius commented Jan 18, 2024

fee1-dead commented Jan 20, 2024

bors commented Jan 20, 2024

Ensure `callee_id`s are body owners #120000

Ensure `callee_id`s are body owners #120000