include HirIds directly in the THIR, not wrapped in LintLevels
#150846
Conversation
|
Some changes occurred in match checking cc @Nadrieril Some changes occurred in match lowering cc @Nadrieril |
rustbot
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #146923) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Another option here would be to use ItemLocalId instead of HirId to maybe make thir::StmtKind and thir::Arm a bit smaller. I'm not really familiar with layout computation so I don't actually know for sure if it'd do anything. I don't think it'd affect thir::ExprKind's size.
LocalVarId is also a whole HirId despite local variables' scopes being stored by ItemLocalId in the ScopeTree, so maybe there's a stylistic reason to use HirIds instead of ItemLocalIds in the THIR? Not sure.
There was a problem hiding this comment.
I personally feel in favor of using ItemLocalId in the THIR, the same as for TypeckResults. Could defer that to a later PR though. Feels like an unrelated change
| } else { | ||
| f(self) | ||
| } | ||
| fn with_lint_level<T>(&mut self, new_lint_level: HirId, f: impl FnOnce(&mut Self) -> T) -> T { |
There was a problem hiding this comment.
lint_level is a misnomer now 🤔 maybe just with_hir_source or whatever
There was a problem hiding this comment.
with_hir_source sounds good to me. Renaming the lint_level field to hir_source also makes the other uses of it make more sense I think
|
👍 r=me with the naming fixed |
|
@bors r+ r=lcnr |
rust-bors
bot
added
S-waiting-on-bors
|
@bors r- |
rust-bors
bot
added
S-waiting-on-author
|
Commit 8868b47 has been unapproved. |
|
@bors r=lcnr I forgot the syntax, oops ^^; |
rust-bors
bot
added
S-waiting-on-bors
Rollup of 4 pull requests Successful merges: - #150846 (include `HirId`s directly in the THIR, not wrapped in `LintLevel`s) - #150979 (Avoid ICEs after bad patterns, for the other syntactic variants) - #151103 (mir_build: Simplify length-determination and indexing for array/slice patterns) - #151130 (resolve: Downgrade `ambiguous_glob_imports` to warn-by-default) r? @ghost
Rollup merge of #150846 - thir-hir-id, r=lcnr include `HirId`s directly in the THIR, not wrapped in `LintLevel`s Occurrences of `LintLevel` in the THIR were always `LintLevel::Explicit`, containing a `HirId`, so we don't need to make it possible to put `LintLevel::Inherited` there. Removing the unused case where `HirId`s aren't present in the THIR slightly simplifies diagnostics/lints/tools that want to map from the THIR back to the HIR, e.g. #145569. Since `LintLevel` is no longer present in the THIR, I've moved it in the second commit to live in `rustc_mir_build`; that's where it's actually used. I'm not sure exactly where exactly it should live there, but I put it in the `builder::scope` module since it's used by `Builder::in_scope` for determining when to introduce source scopes. r? lcnr as the reviewer of #145569, since this was discussed there
3 participants
Occurrences of
LintLevelin the THIR were alwaysLintLevel::Explicit, containing aHirId, so we don't need to make it possible to putLintLevel::Inheritedthere. Removing the unused case whereHirIds aren't present in the THIR slightly simplifies diagnostics/lints/tools that want to map from the THIR back to the HIR, e.g. #145569.Since
LintLevelis no longer present in the THIR, I've moved it in the second commit to live inrustc_mir_build; that's where it's actually used. I'm not sure exactly where exactly it should live there, but I put it in thebuilder::scopemodule since it's used byBuilder::in_scopefor determining when to introduce source scopes.r? lcnr as the reviewer of #145569, since this was discussed there