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

Resolve lang items in AST #103662

Closed
wants to merge 7 commits into from
Closed

Conversation

camsteffen
Copy link
Contributor

Based on #103603

Lang items can be resolved in the AST rather than the HIR. And then any HIR structures referencing LangItem can be replaced with their "normal" couterpart. Specifically QPath::LangItem and GenericBound::LangItemTrait are factored out.

Best reviewed commit-at-a-time.

@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2022

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 28, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2022

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in need_type_info.rs

cc @lcnr

@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov self-assigned this Oct 28, 2022
@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2022
@bors

This comment was marked as resolved.

@camsteffen camsteffen added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 31, 2022
@rust-log-analyzer

This comment has been minimized.

@camsteffen
Copy link
Contributor Author

Changed the self-type path segment to a dummy segment (Res::Err) instead of using the parent lang item.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@camsteffen camsteffen force-pushed the lang-ast branch 2 times, most recently from 9d50f27 to 141d06a Compare November 1, 2022 18:37
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@TaKO8Ki TaKO8Ki 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 Nov 2, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 2, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@camsteffen
Copy link
Contributor Author

@rustbot ready (sorry for the false alarm earlier)

@rustbot rustbot 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 Nov 2, 2022
@bors
Copy link
Contributor

bors commented Nov 2, 2022

☔ The latest upstream changes (presumably #103888) made this pull request unmergeable. Please resolve the merge conflicts.


pub fn generics(&self) -> Option<&Generics> {
match self {
Self::Const(..) => None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Self::Const(..) => None,
AssocItemKind::Const(..) => None,

Could you avoid using Self outside of generic contexts, it makes it harder to search and read code.

@@ -0,0 +1,126 @@
use crate::errors::{
IncorrectTarget, LangItemOnIncorrectTarget, UnknownExternLangItem, UnknownLangItem,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could you split imports so they fit on a single line?

visit::walk_crate(&mut collector, krate);
let LanguageItemCollector { items, missing, .. } = collector;
self.lang_items = items;
self.missing_lang_items = missing;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could keep a mutable reference to Resolver and push directly to resolver.items/resolver.missing instead of keeping temporary copies.

@@ -955,6 +957,8 @@ pub struct Resolver<'a> {
/// the surface (`macro` items in libcore), but are actually attributes or derives.
builtin_macro_kinds: FxHashMap<LocalDefId, MacroKind>,
registered_tools: RegisteredTools,
lang_items: Vec<(LocalDefId, LangItem)>,
missing_lang_items: Vec<LangItem>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could you move it to some other place (e.g. to the end), so these fields don't end up in the middle of macro-related stuff.

}
// range literals desugar to a struct literal or `RangeInclusive::new(..)`
matches!(expr.kind, ExprKind::Call(..) | ExprKind::Struct(..))
&& expr.span.is_desugaring(DesugaringKind::RangeLiteral)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be possible to use comparisons with lang item def ids to detect range expressions without introducing a new DesugaringKind.
Using expansion machinery in AST lowering is not a good practice, and I'd like to try eliminating it somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've wondered about this too. Do you think all of hygiene::ExpnKind::{AstPass, Desugaring, Inlined} are bad?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentatively yes, I just never had time to think what better alternatives are possible.
(Except for AstPass, it's used just like it's supposed to be.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could work similar to hir_attrs.

@@ -518,7 +518,7 @@ pub fn panicking() -> bool {
}

/// Entry point of panics from the libcore crate (`panic_impl` lang item).
#[cfg(not(test))]
#[cfg(not(any(test, doctest)))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is necessary because rustdoc unconditionally runs the lang item collection pass now.
We probably need to run benchmarks on this PR to check how it affects performance.

@@ -45,6 +45,5 @@ fn main() {
StructAsync { callback }.await;
//~^ ERROR expected `fn() -> impl Future<Output = ()> {callback}` to be a fn item that returns `Pin<Box<(dyn Future<Output = ()> + 'static)>>`, but it returns `impl Future<Output = ()>`
//~| ERROR expected `fn() -> impl Future<Output = ()> {callback}` to be a fn item that returns `Pin<Box<(dyn Future<Output = ()> + 'static)>>`, but it returns `impl Future<Output = ()>`
//~| ERROR expected `fn() -> impl Future<Output = ()> {callback}` to be a fn item that returns `Pin<Box<(dyn Future<Output = ()> + 'static)>>`, but it returns `impl Future<Output = ()>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if this PR was a pure refactoring not changing any logic, including diagnostics, if possible.
This PR is large and it's hard to notice and review any such changes.

@@ -1,8 +1,11 @@
error[E0152]: found duplicate lang item `panic_impl`
--> $DIR/duplicate_entry_error.rs:11:1
|
LL | fn panic_impl(info: &PanicInfo) -> ! {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | / fn panic_impl(info: &PanicInfo) -> ! {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "head span" logic implemented for HIR only?
It should be portable to AST pretty easily, IIRC.

@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 4, 2022

The HIR paths for lang items are still artificial, but this still looks like an improvement because they mimic regular paths now and can be processed by the same logic.

Was this idea discussed anywhere previously? Does it interfere with any other plans?
I remember someone mentioned moving lang item collection to an earlier stage for some reason, but I don't remember who exactly.
cc @rust-lang/compiler (I'm not going to nominate this or start an FCP, but will still ping people just in case.)

@petrochenkov petrochenkov 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 Nov 4, 2022
@Dylan-DPC
Copy link
Member

@camsteffen any updates on this? it has bitrotted severely :P

@camsteffen
Copy link
Contributor Author

We currently use QPath::LangItem to know whether a node is compiler-generated in the case of range literals. This PR currently changes that to use DesugaringKind instead. @petrochenkov noted that this is furthering a bad pattern of misusing the expansion machinery. So if we don't want to do that, then this PR is blocked on changing DesugaringKind to something better. I've experimented in that direction a bit but I don't know if I'll have a presentable solution any time soon.

Maybe this PR is a net-positive change as is (merge it), and the DesugaringKind problem can be handled down the road, but I'm not so sure. Seems just as well to let this sit. If you agree, I could write up an issue to block this on.

@Dylan-DPC
Copy link
Member

If you agree, I could write up an issue to block this on.

Yeah that would be helpful.

Thanks for the update

@Dylan-DPC Dylan-DPC added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 17, 2022
@camsteffen
Copy link
Contributor Author

As I try to put the issue into words, maybe I don't really understand what is the problem with ExpnKind::Desugaring... Desugaring and macro expansion are two very similar things (some syntax expands into different code) and it works neatly for them to share an abstraction. I do think it would be good for desugaring data to be linked to IR nodes rather than spans, but I think that is just as much true for macro expansion data.

@camsteffen
Copy link
Contributor Author

Closing (for now) since I haven't had time for rust for a long while 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants