Maybe it's better to fix check_unused_traits/lint_level_id_at_node somehow, instead of generation more detailed recovery HIR.
We don't generally require for all DefIds to appear in HIR (that's why MaybeOwner::Phantom exists), so the issue may potentially reproduce on something else than delegation.
Needs some investigation.
@rustbot author

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the research the cause of the error was identified: during resolve stage and before HIR lowering the maybe_unused_trait_imports set is filled with candidate LocalDefIds of unused traits. Next, during the HIR analysis those candidates are checked in the check_unused_traits function. As those LocalDefIds were collected before HIR lowering, there might be situations when some parts of AST were not lowered to HIR (as in the current case #144594 or #137708). Then, when the lint is produced in node_span_lint method, eventually we end up using ParentHirIterator in order to walk from the current HirId up to the crate and get ICE when trying to obtain non-existing HirId for LocalDefId.

There are several options to fix this problem:

1. Lower the delegation body when we failed to resolve delegation signature, thus we will have all requested HirIds,
2. Check LocalDefId in check_unused_traits function for validity, i.e., we want to make sure that all LocalDefIds from the current one up to the root have corresponding HirId,
3. Update ParentHirIterator so it will check whether the processed OwnerId and its parent have HirId,

I think we should stick with the first option for the following reasons:

1. If we will add checks in check_unused_traits to ensure that all DefIds have corresponding HirId, we will make assumptions about the nature of node_span_lint method, because this upwalk is taken place there, this will result in abstraction leak, as the behavior of node_span_lint may change in the future but the check we potentially add in check_unused_traits method will remain,
2. Updating ParentHirIterator with additional checks that HirIds exist for processed DefIds seems not correct, as firstly, if we do this, we should also update ParentOwnerIterator as it performs approximately the same routine and also crashes in this scenario, secondly, looking at the API that is provided to obtain HirIds, I believe that the invariant is that we should have a HirId for the DefId, addition of opt_ version of this APIs will break the current consistency, and it seems like it is the responsibility of the lowering stage to produce all required HirIds for DefIds,
3. The ICE: No HirId for DefId #137708 issue that is almost identical with our issue ICE: delegation: No HirId for DefId #144594 was fixed by lowering parts of AST that were not lowered before, so I think that we want to be consistent with this fix,
4. As maybe_unused_trait_imports are filled before HIR lowering, there are other ICEs that start from check_unused_traits function, they can be found using "check_unused::check_unused_traits" search (i.e., ICE type_of called on const argument's anon const before the const argument was lowered #148730), so if we will add protection from "No HirId for DefId" error there will be others that will probably bypass this check.

@rustbot ready

r=me after addressing #149548 (comment) and squashing commits.
@rustbot author

@bors r+

📌 Commit 3e71712 has been approved by petrochenkov

It is now in the queue for this repository.