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.

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

Investigate mutating the AST during late resolution #99669

Open
4 tasks
cjgillot opened this issue Jul 24, 2022 · 13 comments
Open
4 tasks

Investigate mutating the AST during late resolution #99669

cjgillot opened this issue Jul 24, 2022 · 13 comments
Labels
A-hir Area: The high-level intermediate representation (HIR) A-resolve Area: Path resolution E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-help-wanted Call for participation: Help is requested to fix this issue. 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 Jul 24, 2022

Late name resolution happens on the AST and is responsible for finding which definition each name in the AST points to, be it a type, trait expression, pattern, lifetime or label. The name "late" means that this resolution happens after all macros have been expanded, in opposition to "early" resolution which resolves macros. The corresponding code lives in rustc_resolve::late.

The results of this late resolution are stored in tables, and used by HIR lowering to directly embed the information in HIR.

Later, lowering reinterprets some of the AST where the syntax is ambiguous. For instance, lowering is responsible for making bare trait objects explicit: interpreting types of the form T with dyn T when resolution finds that T is a trait. Lowering also introduces implicit lifetime parameters for lifetime elision. The corresponding code lives in rustc_ast_lowering.

This re-interpretation of the AST needs to closely mirror what resolution is doing. Mismatches can cause bugs, like AST and HIR disagreeing on how to resolve lifetimes in the dyn T discussed above.

The objective of this project is to have this resolution-based desugaring in only one place. We would like to have late resolution modify the AST to directly perform these transformations, and remove them from lowering.

At the same time, lowering also performs its own transformations, like transforming impl Trait to generic parameters or opaque types. Those transformations should stay in lowering for the time being. Rule of thumb: if the transformation inspects resolutions, it should go in resolution, if it's purely syntactic it can stay in lowering.

Steps:

  • Refactor the visitor in rustc_resolve::late by an AST MutVisitor. MutVisitor is designed for macro expansions, so it may lack some methods to make resolution practical. There can be design work here, so feel free to suggest new methods / a different MutVisitor trait / ...
  • Desugar type-relative paths during resolution. The current code in lowering is in rustc_ast_lowering::path, and aims to transform a::b::c::d to <a::b::c>::d when a::b::c can be resolved to a type and ::d cannot (for instance because it is a trait method). See rustc_resolve::late::smart_resolve_path and associated methods.
  • Desugar bare trait object during late resolution when the path is actually a trait. See rustc_resolve::late::visit_ty.
  • Explicit elided lifetimes on the AST by inserting fresh lifetimes.

Please contact me on zulip (cjgillot) for any questions.

Note: this project is a large body of work, with possibly many changes to the resolution code. The point is to simplify the lowering code. If this simplification goal is not reached, or if the perf impact is prohibitive, we may end-up not merging the modifications, and instead document why.
To be merged, the PR will probably require a major change proposal. I will take care of this proposal once the perspectives become a bit clear.

@cjgillot cjgillot added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. A-resolve Area: Path resolution E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. A-hir Area: The high-level intermediate representation (HIR) E-help-wanted Call for participation: Help is requested to fix this issue. labels Jul 24, 2022
@petrochenkov
Copy link
Contributor

I'm not sure whether it's a good idea in general, but the implementation/prototyping here can be simplified by splitting this job into two steps:

  • Step 1: turn the AST mutation into a separate MutVisitor pass that runs immediately after late resolution and uses the tables built by immutable late resolution. This pass becomes a some kind of "pre AST lowering" or "early AST lowering". The equivalent code can then be removed from the "late" AST lowering.
  • Step 2: merge that pass into late resolution and eliminate tables using for communication between late resolution and early lowering.

@petrochenkov
Copy link
Contributor

@cjgillot
How does all this map on incremental compilation changes you've been planning to do?

@vincenzopalazzo
Copy link
Member

I'm taking a lock on it because its feet on my learning path! and see the evolution of the discussion in the meanwhile

@rustbot claim

@Noratrieb
Copy link
Member

@vincenzopalazzo are you still working on this?

@vincenzopalazzo vincenzopalazzo removed their assignment Jan 19, 2023
@vincenzopalazzo
Copy link
Member

@Nilstrieb it is in my todo list my I had some other priority, so I unassign me if you had any other good target to assign this

@jon-chuang
Copy link

@rustbot claim

Hi, I'd like to try this out.

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@kostekIV
Copy link

kostekIV commented Sep 1, 2023

Hi @jon-chuang are you working on this? And @cjgillot is this still a relevant issue?

@jon-chuang
Copy link

Hello, not anymore

@cjgillot
Copy link
Contributor Author

cjgillot commented Sep 1, 2023

This issue is still relevant.

@kostekIV
Copy link

kostekIV commented Sep 1, 2023

cool, I will work on this then.

@axelmagn
Copy link

axelmagn commented Jan 6, 2024

I'm picking this issue up after a conversation with @cjgillot.

@rustbot claim

@axelmagn
Copy link

Stepping down from this issue -- the complexity is a bit much for me right now, and my priorities have shifted.

@rustbot release-assignment

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 20, 2024

Having a pre-lowering pass working on AST may be useful for implementing delegation (#118212).
The part that does this transformation in particular.

impl Foo { // id_foo
    reuse Trait::{a, b, c}; // id_reuse
}

=>

impl Foo { // id_foo
    reuse Trait::a; // id_a
    reuse Trait::b; // id_b
    reuse Trait::c; // id_c
}

Then AST lowering will turn the reuse items into proper fn items.

With the current lowering we immediately go inside the id_reuse owner node when processing an (associated) item.
But I want to generate the delegated fn items immediately under the id_foo parent, and next to id_reuse, because a lot of code in the compiler relies on an impl being a direct parent of its impl items.

Not sure how to do it better without a pre-lowering pass.

UPD: Design for this specific question about delegation - rust-lang/rfcs#3530 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-hir Area: The high-level intermediate representation (HIR) A-resolve Area: Path resolution E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-help-wanted Call for participation: Help is requested to fix this issue. 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
Development

Successfully merging a pull request may close this issue.

7 participants