Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Collect lang items on the AST instead of HIR #115178

Open
cjgillot opened this issue Aug 24, 2023 · 6 comments
Open

Collect lang items on the AST instead of HIR #115178

cjgillot opened this issue Aug 24, 2023 · 6 comments
Labels
A-ast Area: AST A-lang-item Area: Language items C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cjgillot
Copy link
Contributor

cjgillot commented Aug 24, 2023 •
edited by rustbot
Loading

Lang items are collected on HIR in rustc_passes::lang_items.
However, AST -> HIR lowering may need to know of some lang items, for instance for loops of async desugaring.
Having lang items ready before lowering may simplify those code paths.

This requires re-implementing lang item collection on AST instead of HIR.
The simplest way is to make rustc_passes::lang_items::LanguageItemCollector a rustc_ast::Visitor, and have get_lang_items use it to visit the full crate. The AST is accessible using the second field of tcx.resolver_for_lowering(()).borrow().

A statement tcx.ensure_with_value().get_lang_items(LOCAL_CRATE) needs to be added at the top of rustc_ast_lowering::lower_to_hir to ensure that the computation is done before dropping the AST.

Once that is done, hir::QPath::LangItem and hir::GenericBound::LangItemTrait variants can be removed from HIR, replaced by actual resolutions of the lang items. (For particular cases, see how those variants are constructed and used.)

Please contact me on zulip for any question.
@cjgillot cjgillot added C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Aug 24, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 24, 2023
@atsuzaki
Copy link
Contributor

@rustbot claim

@atsuzaki atsuzaki removed their assignment Aug 26, 2023
@allaboutevemirolive
Copy link
Contributor

@rustbot claim

@Noratrieb Noratrieb added A-lang-item Area: Language items and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 29, 2023
@RickleAndMortimer
Copy link
Contributor

Is this still being worked on? I'd like to take a look as well if we still need assistance for this issue

@RickleAndMortimer RickleAndMortimer mentioned this issue Oct 6, 2023
Closed
@compiler-errors compiler-errors mentioned this issue Nov 28, 2023
Merged
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Dec 15, 2023
@workingjubilee
Rollup merge of rust-lang#118396 - compiler-errors:ast-lang-items, r=…
1d54949 
…cjgillot

Collect lang items from AST, get rid of `GenericBound::LangItemTrait`

r? `@cjgillot`
cc rust-lang#115178

Looking forward, the work to remove `QPath::LangItem` will also be significantly more difficult, but I plan on doing it as well. Specifically, we have to change:
1. A lot of `rustc_ast_lowering` for things like expr `..`
2. A lot of astconv, since we actually instantiate lang and non-lang paths quite differently.
3. A ton of diagnostics and clippy lints that are special-cased via `QPath::LangItem`

Meanwhile, it was pretty easy to remove `GenericBound::LangItemTrait`, so I just did that here.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 16, 2023
@rust-timer
Unrolled build for rust-lang#118396
6a29ea3 
Rollup merge of rust-lang#118396 - compiler-errors:ast-lang-items, r=cjgillot

Collect lang items from AST, get rid of `GenericBound::LangItemTrait`

r? `@cjgillot`
cc rust-lang#115178

Looking forward, the work to remove `QPath::LangItem` will also be significantly more difficult, but I plan on doing it as well. Specifically, we have to change:
1. A lot of `rustc_ast_lowering` for things like expr `..`
2. A lot of astconv, since we actually instantiate lang and non-lang paths quite differently.
3. A ton of diagnostics and clippy lints that are special-cased via `QPath::LangItem`

Meanwhile, it was pretty easy to remove `GenericBound::LangItemTrait`, so I just did that here.
@compiler-errors compiler-errors mentioned this issue Dec 23, 2023
Merged
compiler-errors added a commit to compiler-errors/rust that referenced this issue Dec 26, 2023
@compiler-errors
Rollup merge of rust-lang#119240 - compiler-errors:lang-item-more, r=…
e7bd402 
…petrochenkov

Make some non-diagnostic-affecting `QPath::LangItem` into regular `QPath`s

The rest of 'em affect diagnostics, so leave them alone... for now.

cc rust-lang#115178
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 26, 2023
@rust-timer
Unrolled build for rust-lang#119240
95f3409 
Rollup merge of rust-lang#119240 - compiler-errors:lang-item-more, r=petrochenkov

Make some non-diagnostic-affecting `QPath::LangItem` into regular `QPath`s

The rest of 'em affect diagnostics, so leave them alone... for now.

cc rust-lang#115178
flip1995 pushed a commit to flip1995/rust that referenced this issue Dec 28, 2023
@compiler-errors
Rollup merge of rust-lang#119240 - compiler-errors:lang-item-more, r=…
2230add 
…petrochenkov

Make some non-diagnostic-affecting `QPath::LangItem` into regular `QPath`s

The rest of 'em affect diagnostics, so leave them alone... for now.

cc rust-lang#115178
@axelmagn
Copy link

axelmagn commented Jan 4, 2024

@rustbot claim
I am a first-time contributor looking for a good mentorship issue. Please let me know if anything is already in-flight with this.

@compiler-errors
Copy link
Member

@axelmagn: Sorry -- this work was mostly done in #118396. The remaining work is quite tedious, though I guess you could reach out to @cjgillot to see if they would like to mentor it.

@axelmagn
Copy link

axelmagn commented Jan 4, 2024

@compiler-errors no worries - there are plenty of fish in the sea. Glad it got fixed.

@axelmagn axelmagn removed their assignment Jan 4, 2024
@workingjubilee workingjubilee added the A-ast Area: AST label Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ast Area: AST A-lang-item Area: Language items C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@axelmagn @cjgillot @compiler-errors @atsuzaki @RickleAndMortimer @workingjubilee @rustbot @Noratrieb @allaboutevemirolive