Skip to content

Reference lang items during AST lowering - #75145

Merged
bors merged 9 commits into
rust-lang:masterfrom
davidtwco:issue-60607-preallocate-defid-for-lang-items
Aug 17, 2020
Merged

Reference lang items during AST lowering#75145
bors merged 9 commits into
rust-lang:masterfrom
davidtwco:issue-60607-preallocate-defid-for-lang-items

Conversation

@davidtwco

@davidtwcodavidtwco commented Aug 4, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#60607 and fixes#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.
  • 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 @matthewjasperin this comment 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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2020
@davidtwcodavidtwco changed the title Issue 60607 preallocate defid for lang itemsPreallocate DefIds for lang itemsAug 4, 2020
This was referenced Aug 4, 2020
Closed
Closed
@petrochenkovpetrochenkov self-assigned this Aug 4, 2020
@petrochenkov

petrochenkov commented Aug 9, 2020

Copy link
Copy Markdown
Contributor

(I'll review this next weekend or during the week.)

petrochenkov
petrochenkov reviewed Aug 15, 2020
View reviewed changes
Comment threadsrc/librustc_passes/lang_items.rs Outdated
Comment threadsrc/librustc_hir/hir.rs Outdated
Comment threadsrc/librustc_typeck/check/mod.rs Outdated
Comment threadsrc/librustc_hir/hir.rs Outdated
Comment threadsrc/librustc_hir/hir.rs Outdated
Comment threadsrc/librustc_hir/hir.rs Outdated
@petrochenkovpetrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2020
davidtwcoand others added 8 commits August 16, 2020 15:42
@davidtwco
lang_items: add support for lang items on variants
764dc3d
This commit adds support for lang items (`#[lang = "..."]` attributes)
on enum variants.
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
tests: add test forrust-lang#61019's current behaviour
734441c
This commit adds a test for rust-lang#61019 where a extern crate is imported as
`std` which results in name resolution to fail due to the uses of `std`
types introduced from lowering.
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco@matthewjasper
hir: introduce lang items for AST lowering
7dee5f8
This commit adds new lang items which will be used in AST lowering once
`QPath::LangItem` is introduced.
Co-authored-by: Matthew Jasper <mjjasper1@gmail.com>
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco@matthewjasper
hir: introduce QPath::LangItem
762137e
This commit introduces `QPath::LangItem` to the HIR and uses it in AST
lowering instead of constructing a `hir::Path` from a slice of symbols.
This might be better for performance, but is also much cleaner as the
previous approach is fragile. In addition, it resolves a bug (rust-lang#61019)
where an extern crate imported as "std" would result in the paths
created during AST lowering being resolved incorrectly (or not at all).
Co-authored-by: Matthew Jasper <mjjasper1@gmail.com>
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
save_analysis: support QPath::LangItem
1e2f350
This commit implements support for `QPath::LangItem` and
`GenericBound::LangItemTrait` in save analysis.
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco@matthewjasper
hir: simplify is_range_literal
664ecf1
This commit simplifies `is_range_literal` by checking for
`QPath::LangItem` containing range-related lang items, rather than using
a heuristic.
Co-authored-by: Matthew Jasper <mjjasper1@gmail.com>
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco@matthewjasper
resolve: support GenericBound::LangItemTrait
8367af4
This commit modifies name resolution to ensure that new scopes are
introduced from lang-item generic bounds.
Co-authored-by: Matthew Jasper <mjjasper1@gmail.com>
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
rustdoc: clean QPath::LangItem
dde93c9
This commit adds support for cleaning `QPath::LangItem` and
`hir::GenericBound::LangItemTrait` in rustdoc. `QPath::LangItem`
does not require lowering, and `hir::GenericBound::LangItemTrait`
is lowered to a `GenericBound::TraitBound`.
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
davidtwcoforce-pushed the issue-60607-preallocate-defid-for-lang-items branch from 8d886c4 to bde529fCompareAugust 16, 2020 15:13
@petrochenkov

petrochenkov commented Aug 16, 2020

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Aug 16, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit bde529f has been approved by petrochenkov

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 16, 2020
@Dylan-DPC
Rollup merge ofrust-lang#75145- davidtwco:issue-60607-preallocate-d…
3e5ce7b
…efid-for-lang-items, r=petrochenkov
Preallocate `DefId`s for lang items
Fixesrust-lang#60607 and fixesrust-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
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Aug 16, 2020
Closed
@eddyb

eddyb commented Aug 16, 2020

Copy link
Copy Markdown
Contributor

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.

You can use ./x.py test src/tools/clippy at the very least. Suspecting this is what broke a rollup (#75606), so:
@bors rollup=never

@eddyb

eddyb commented Aug 16, 2020

Copy link
Copy Markdown
Contributor

Btw the title still says "Preallocate DefIds for lang items" but it doesn't look like that's the strategy taken?
Instead, lang items are referenced by the HIR without using their DefIds.

(the alternative to this would be doing lang item collection before the HIR, but I think this solution is nicer)

Also, surely this has perf implications?

@bors try @rust-timer queue

@bors

bors commented Aug 16, 2020

Copy link
Copy Markdown
Collaborator

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

@rust-timer

rust-timer commented Aug 16, 2020

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion

@borsbors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 16, 2020
@bors

bors commented Aug 16, 2020

Copy link
Copy Markdown
Collaborator

⌛ Trying commit bde529f with merge 48b5808e9529407d15e12c40168e01ce15a6e9db...

@bors

bors commented Aug 16, 2020

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-actions, checks-azure
Build commit: 48b5808e9529407d15e12c40168e01ce15a6e9db (48b5808e9529407d15e12c40168e01ce15a6e9db)

@rust-timer

rust-timer commented Aug 16, 2020

Copy link
Copy Markdown
Collaborator

Queued 48b5808e9529407d15e12c40168e01ce15a6e9db with parent 9b4db69, future comparison URL.

@rust-timer

rust-timer commented Aug 17, 2020

Copy link
Copy Markdown
Collaborator

Finished benchmarking try commit (48b5808e9529407d15e12c40168e01ce15a6e9db): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@eddyb

eddyb commented Aug 17, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Perhaps unsurprisingly, this is a nice win for await-heavy code.

@petrochenkov

petrochenkov commented Aug 17, 2020

Copy link
Copy Markdown
Contributor

You can use ./x.py test src/tools/clippy at the very least.

It's ./x.py test src/tools/clippy --stage 2 now that the defaults were changed.

@davidtwco
clippy: support QPath::LangItem
f1ce294
This commit updates clippy with the introduction of `QPath::LangItem` so
that it still compiles.
Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
davidtwcoforce-pushed the issue-60607-preallocate-defid-for-lang-items branch from bde529f to f1ce294CompareAugust 17, 2020 12:55
@davidtwco

davidtwco commented Aug 17, 2020

Copy link
Copy Markdown
MemberAuthor

Alright - I've ran the clippy tests now and fixed all of the issues that this introduced.

@davidtwcodavidtwco changed the title Preallocate DefIds for lang itemsReference lang items during AST loweringAug 17, 2020
@davidtwcodavidtwco added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 17, 2020
@petrochenkov

petrochenkov commented Aug 17, 2020

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Aug 17, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit f1ce294 has been approved by petrochenkov

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2020
@bors

bors commented Aug 17, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit f1ce294 with merge 792c645...

@bors

bors commented Aug 17, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 792c645 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Aug 17, 2020
@bors
bors merged commit 792c645 into rust-lang:masterAug 17, 2020
@davidtwco
davidtwco deleted the issue-60607-preallocate-defid-for-lang-items branch August 17, 2020 23:38
@borsbors mentioned this pull request Aug 17, 2020
Merged
@ecstatic-morse

ecstatic-morse commented Aug 24, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Final perf results show a small win as expected. Nice work!

@davidtwcodavidtwco mentioned this pull request Aug 26, 2020
Closed
@VeykrilVeykril mentioned this pull request Aug 8, 2021
Merged
@bjorn3bjorn3 mentioned this pull request May 15, 2022
Closed
@cuvipercuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petrochenkovpetrochenkovpetrochenkov left review comments

Assignees

@oli-obkoli-obk

@petrochenkovpetrochenkov

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.47.0

Development

Successfully merging this pull request may close these issues.

extern crate ... as std overrides the injected crate on 2018 edition Preallocate DefIds for lang items and use lang items in lowering

9 participants

@davidtwco@petrochenkov@bors@eddyb@rust-timer@ecstatic-morse@cuviper@oli-obk@rust-highfive