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

HIR lowering relies on AST node IDs #30809

Closed
durka opened this issue Jan 10, 2016 · 8 comments
Closed

HIR lowering relies on AST node IDs #30809

durka opened this issue Jan 10, 2016 · 8 comments
Labels
A-HIR Area: The high-level intermediate representation (HIR) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@durka
Copy link
Contributor

durka commented Jan 10, 2016

cc @nrc
cc @eddyb

So I was looking at how desugarings work in HIR lowering. Notably, lang items are collected after lowering but it is sometimes desirable to refer to them. @eddyb suggested a side table mapping HIR node IDs to prospective lang items, which would be cross-referenced with the real lang items once they are collected. But this relies on IDs being known during lowering. Currently they are, because they are copied from the AST IDs, but @eddyb was saying that the AST shouldn't have IDs at all. Currently it is needed because lowering needs to be reproducible, but... why? Are these IDs going away, or are they here to stay and usable for this lang item scheme?

@durka
Copy link
Contributor Author

durka commented Jan 10, 2016

cc #20541

@nrc
Copy link
Member

nrc commented Jan 10, 2016

I think the ids issue and the lang items issue are orthogonal. We should definitely find a way to use lang items for the HIR lowering.

I would like to remove node ids from the AST and just use them in the HIR. The current situation is mostly because it was easier to implement. Reproducible lowering may be unnecessary. However, ids do make it easy to identify an AST node, we'd want to be sure that we don't this (and that it is not useful for tools or syntax extensions). So lets not just rip them out and see what happens.

@eddyb
Copy link
Member

eddyb commented Jan 10, 2016

@nrc currently we can identify AST nodes by referential equality (i.e. pointers).
Even if we ditch the allocation-heavy trees, we could still have various ways of doing it (think "pre-order iteration index").

durka added a commit to durka/rust that referenced this issue Jan 27, 2016
The range desugaring does not use the lang items. Hence I did not add
lang items for inclusive ranges. This cleanup commit removes the old
unused ones as well.

Whether the desugaring _should_ use lang items is another question:
see rust-lang#30809. But if we decide on a strategy there we can add back these
lang items, and new ones for inclusive ranges.

For stage0 we need to keep the attributes as the lang items still exist
even if they are never used.

This is surprisingly not a breaking change. Unused #[lang] attributes do
not even trigger a lint (see rust-lang#30881).
durka added a commit to durka/rust that referenced this issue Jan 27, 2016
The range desugaring does not use the lang items. Hence I did not add
lang items for inclusive ranges. This cleanup commit removes the old
unused ones as well.

Whether the desugaring _should_ use lang items is another question:
see rust-lang#30809. But if we decide on a strategy there we can add back these
lang items, and new ones for inclusive ranges.

For stage0 we need to keep the attributes as the lang items still exist
even if they are never used.

This is surprisingly not a breaking change. Unused #[lang] attributes do
not even trigger a lint (see rust-lang#30881).
durka added a commit to durka/rust that referenced this issue Feb 27, 2016
The range desugaring does not use the lang items. Hence I did not add
lang items for inclusive ranges. This cleanup commit removes the old
unused ones as well.

Whether the desugaring _should_ use lang items is another question:
see rust-lang#30809. But if we decide on a strategy there we can add back these
lang items, and new ones for inclusive ranges.

For stage0 we need to keep the attributes as the lang items still exist
even if they are never used.

This is surprisingly not a breaking change. Unused #[lang] attributes do
not even trigger a lint (see rust-lang#30881).
@durka
Copy link
Contributor Author

durka commented Sep 26, 2016

Is this still true?

@Mark-Simulacrum
Copy link
Member

@eddyb @nrc As @durka asks, do you know if this is still the case?

@eddyb
Copy link
Member

eddyb commented May 14, 2017

The "reproductible" lowering is gone but they IDs are still very much reused.
@michaelwoerister did start some work to have different HIR IDs for incremental.

@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 22, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 24, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Jan 25, 2020

Now that we use HirIds everywhere, this is fixed, right?

@jonas-schievink jonas-schievink added the A-HIR Area: The high-level intermediate representation (HIR) label Mar 15, 2020
@bjorn3
Copy link
Member

bjorn3 commented May 15, 2022

Can this be closed?

@oli-obk oli-obk closed this as completed May 16, 2022
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) C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants