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

Preallocate DefIds for lang items and use lang items in lowering #60607

Closed
oli-obk opened this issue May 7, 2019 · 6 comments · Fixed by #75145
Closed

Preallocate DefIds for lang items and use lang items in lowering #60607

oli-obk opened this issue May 7, 2019 · 6 comments · Fixed by #75145
Assignees
Labels
A-hir Area: The high-level intermediate representation (HIR) A-lang-item Area: lang items 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

@oli-obk
Copy link
Contributor

oli-obk commented May 7, 2019

Right now, our lowering code uses string-paths to call e.g. Iterator::next when lowering for loops:

let next_path = &["iter", "Iterator", "next"];
let next_path = P(self.expr_std_path(head_sp, next_path, None, ThinVec::new()));
let next_expr = P(self.expr_call(head_sp, next_path, hir_vec![ref_mut_iter]));

This triggers a name resolution call

let mut path = self.resolver
.resolve_str_path(span, self.crate_root, components, is_value);
path.segments.last_mut().unwrap().args = params;
for seg in path.segments.iter_mut() {
if seg.hir_id.is_some() {
seg.hir_id = Some(self.next_id());
}
}
path

which will then give us a resolved path, but that's

  1. potentially expensive, since it happens every time a specific path is generated (which happens a lot for ? and for expansions/lowerings)
  2. seems uncool since it's stringly typed

I think it would be neat if we could instead create the lang_items table early in the process and just grab the appropriate item out of that table.

cc @Centril @eddyb @petrochenkov

As a first experiment to check out the impact we could create a cache for path resolutions so we only run the path resolution code once and just overwrite all HirIds in the generated path with new HirIds whenever we fetch a cached resolution.

@oli-obk oli-obk added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-hir Area: The high-level intermediate representation (HIR) labels May 7, 2019
@Centril Centril added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 7, 2019
@Centril
Copy link
Contributor

Centril commented May 7, 2019

I'm going to take a look at this as a side-project :)

@oli-obk
Copy link
Contributor Author

oli-obk commented May 8, 2019

So... one thing I just thought about:

We could introduce a third variant to QPath: LangItem. QPath::LangItem just contains a

pub enum LangItem {

We can then just create such paths without actually having to do any work. Code further down will have access to a TyCtxt and thus be able to resolve said lang in a way that queries understand.

Centril added a commit to Centril/rust that referenced this issue May 8, 2019
Document + Cleanup lang_items.rs

Byproduct of work on rust-lang#60607.

r? @oli-obk
@Centril Centril self-assigned this May 8, 2019
Centril added a commit to Centril/rust that referenced this issue May 8, 2019
Document + Cleanup lang_items.rs

Byproduct of work on rust-lang#60607.

r? @oli-obk
Centril added a commit to Centril/rust that referenced this issue May 8, 2019
Document + Cleanup lang_items.rs

Byproduct of work on rust-lang#60607.

r? @oli-obk
Centril added a commit to Centril/rust that referenced this issue May 8, 2019
Document + Cleanup lang_items.rs

Byproduct of work on rust-lang#60607.

r? @oli-obk
@varkor varkor added the A-lang-item Area: lang items label May 8, 2019
ayazhafiz added a commit to ayazhafiz/rust that referenced this issue Jun 23, 2020
Currently, rustc uses a heuristic to determine if a range expression is
not a literal based on whether the expression looks like a function call
or struct initialization. This fails for range literals whose
lower/upper bounds are the results of function calls. A possibly-better
heuristic is to check if the expression contains `..`, required in range
literals.

Of course, this is also not perfect; for example, if the range
expression is a struct which includes some text with `..` this will
fail, but in general I believe it is a better heuristic.

A better alternative altogether is to add the `QPath::LangItem` enum
variant suggested in rust-lang#60607. I would be happy to do this as a precursor
to this patch if someone is able to provide general suggestions on how
usages of `QPath` need to be changed later in the compiler with the
`LangItem` variant.

Closes rust-lang#73553
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 23, 2020
Change heuristic for determining range literal

Currently, rustc uses a heuristic to determine if a range expression is
not a literal based on whether the expression looks like a function call
or struct initialization. This fails for range literals whose
lower/upper bounds are the results of function calls. A possibly-better
heuristic is to check if the expression contains `..`, required in range
literals.

Of course, this is also not perfect; for example, if the range
expression is a struct which includes some text with `..` this will
fail, but in general I believe it is a better heuristic.

A better alternative altogether is to add the `QPath::LangItem` enum
variant suggested in rust-lang#60607. I would be happy to do this as a precursor
to this patch if someone is able to provide general suggestions on how
usages of `QPath` need to be changed later in the compiler with the
`LangItem` variant.

Closes rust-lang#73553
@matthewjasper
Copy link
Contributor

see also #61019

I have an old branch for this here for anyone interested:
matthewjasper@a227c70#diff-c0f791ead38d2d02916faaad0f56f41d

Also see this comment: matthewjasper@a227c70#r40107992

@ayazhafiz
Copy link
Contributor

I am interested in finishing up the work you started @matthewjasper, if you don't already have plans to do so.

@matthewjasper
Copy link
Contributor

I don't have plans to finish that work any time soon.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 24, 2020
Change heuristic for determining range literal

Currently, rustc uses a heuristic to determine if a range expression is
not a literal based on whether the expression looks like a function call
or struct initialization. This fails for range literals whose
lower/upper bounds are the results of function calls. A possibly-better
heuristic is to check if the expression contains `..`, required in range
literals.

Of course, this is also not perfect; for example, if the range
expression is a struct which includes some text with `..` this will
fail, but in general I believe it is a better heuristic.

A better alternative altogether is to add the `QPath::LangItem` enum
variant suggested in rust-lang#60607. I would be happy to do this as a precursor
to this patch if someone is able to provide general suggestions on how
usages of `QPath` need to be changed later in the compiler with the
`LangItem` variant.

Closes rust-lang#73553
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 24, 2020
Change heuristic for determining range literal

Currently, rustc uses a heuristic to determine if a range expression is
not a literal based on whether the expression looks like a function call
or struct initialization. This fails for range literals whose
lower/upper bounds are the results of function calls. A possibly-better
heuristic is to check if the expression contains `..`, required in range
literals.

Of course, this is also not perfect; for example, if the range
expression is a struct which includes some text with `..` this will
fail, but in general I believe it is a better heuristic.

A better alternative altogether is to add the `QPath::LangItem` enum
variant suggested in rust-lang#60607. I would be happy to do this as a precursor
to this patch if someone is able to provide general suggestions on how
usages of `QPath` need to be changed later in the compiler with the
`LangItem` variant.

Closes rust-lang#73553
@davidtwco
Copy link
Member

Opened #75145 to resolve this using @matthewjasper's earlier work.

@davidtwco davidtwco assigned davidtwco and unassigned Centril Aug 4, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Aug 16, 2020
…efid-for-lang-items, r=petrochenkov

Preallocate `DefId`s for lang items

Fixes rust-lang#60607 and fixes rust-lang#61019.

This PR introduces `QPath::LangItem` to the HIR and uses it in AST lowering instead of constructing a `hir::Path` from a slice of symbols:

- Credit for much of this work goes to @matthewjasper, I basically just [rebased their earlier work](matthewjasper@a227c70#diff-c0f791ead38d2d02916faaad0f56f41d).
- Changes to Clippy might not be correct, they compile but attempting to run tests through `./x.py` produced failures which appeared spurious, so I didn't run any clippy tests.
- Changes to save analysis might not be correct - tests pass but I don't have a lot of confidence in those changes being correct.
- I've used `GenericBounds::LangItemTrait` rather than changing `PolyTraitRef`, as suggested by @matthewjasper [in this comment](matthewjasper@a227c70#r40107992) but I'd prefer that be left for a follow-up.
- I've split things into smaller commits fairly arbitrarily to make the diff easier to review, each commit should compile but might not pass tests until the final commit.

r? @oli-obk
cc @matthewjasper
@bors bors closed this as completed in 792c645 Aug 17, 2020
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-lang-item Area: lang items 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

Successfully merging a pull request may close this issue.

6 participants