Skip to content

Collect lang items from AST, get rid of GenericBound::LangItemTrait #118396

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Dec 16, 2023

Conversation

compiler-errors
Copy link
Member

r? @cjgillot
cc #115178

Looking forward, the work to remove QPath::LangItem will also be significantly more difficult, but I plan on doing it as well. Specifically, we have to change:

  1. A lot of rustc_ast_lowering for things like expr ..
  2. A lot of astconv, since we actually instantiate lang and non-lang paths quite differently.
  3. A ton of diagnostics and clippy lints that are special-cased via QPath::LangItem

Meanwhile, it was pretty easy to remove GenericBound::LangItemTrait, so I just did that here.

@rustbot rustbot added 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. labels Nov 28, 2023
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

tools! i never check tools!

@rustbot author

@rustbot rustbot 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 28, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@compiler-errors compiler-errors 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 28, 2023
@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors 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 28, 2023
@compiler-errors

This comment was marked as outdated.

@compiler-errors
Copy link
Member Author

compiler-errors commented Nov 28, 2023

Actually, I have no idea what is going on here, and I have no idea where to begin debugging this. Help would be appreciated.

@compiler-errors compiler-errors 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 28, 2023
@petrochenkov
Copy link
Contributor

Previous attempt - #103662.
I left some comments on that PR, not sure how much they are relevant to this one.
(But I'd like to look at this PR too, maybe this or next week.)

@petrochenkov petrochenkov self-assigned this Nov 28, 2023
@compiler-errors
Copy link
Member Author

Oh, thanks for that link, @petrochenkov. I see that the same CI failure here was happening on that PR -- ec485f3. I will just cherry-pick that commit.

@compiler-errors compiler-errors changed the title Collect lang items from AST, get rid of QPath::LangItem Collect lang items from AST, get rid of GenericBound::LangItemTrait Nov 28, 2023
@bors

This comment was marked as resolved.

items: LanguageItems,
tcx: TyCtxt<'tcx>,
resolver: &'ast ResolverAstLowering,
item_spans: FxHashMap<DefId, Span>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should #118552 to avoid having to maintain such map. Could you add a FIXME with the issue so we can retrieve the question?

@compiler-errors
Copy link
Member Author

@rustbot ready

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@cjgillot
Copy link
Contributor

cjgillot commented Dec 9, 2023

r=me after rebase

@compiler-errors
Copy link
Member Author

I had to modify the check for the number of generic params to handle associated items correctly. This should be ready to go, but I guess I will wait for @cjgillot to look at the last commit in particular.

@petrochenkov
Copy link
Contributor

(I'll also look today or tomorrow.)

@@ -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.

The old def_span logic is supposedly reproduced in this PR, so why do some spans change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't reproduce it fully -- I'm just caching the item span here.

In HIR, we do some special span creation to essentially compute the "head span" of each item:

pub fn opt_span(self, hir_id: HirId) -> Option<Span> {
fn until_within(outer: Span, end: Span) -> Span {
if let Some(end) = end.find_ancestor_inside(outer) {
outer.with_hi(end.hi())
} else {
outer
}
}
fn named_span(item_span: Span, ident: Ident, generics: Option<&Generics<'_>>) -> Span {
if ident.name != kw::Empty {
let mut span = until_within(item_span, ident.span);
if let Some(g) = generics
&& !g.span.is_dummy()
&& let Some(g_span) = g.span.find_ancestor_inside(item_span)
{
span = span.to(g_span);
}
span
} else {
item_span
}
}
let span = match self.find(hir_id)? {
// Function-like.
Node::Item(Item { kind: ItemKind::Fn(sig, ..), span: outer_span, .. })
| Node::TraitItem(TraitItem {
kind: TraitItemKind::Fn(sig, ..),
span: outer_span,
..
})
| Node::ImplItem(ImplItem {
kind: ImplItemKind::Fn(sig, ..), span: outer_span, ..
}) => {
// Ensure that the returned span has the item's SyntaxContext, and not the
// SyntaxContext of the visibility.
sig.span.find_ancestor_in_same_ctxt(*outer_span).unwrap_or(*outer_span)
}
// Impls, including their where clauses.
Node::Item(Item {
kind: ItemKind::Impl(Impl { generics, .. }),
span: outer_span,
..
}) => until_within(*outer_span, generics.where_clause_span),
// Constants and Statics.
Node::Item(Item {
kind: ItemKind::Const(ty, ..) | ItemKind::Static(ty, ..),
span: outer_span,
..
})
| Node::TraitItem(TraitItem {
kind: TraitItemKind::Const(ty, ..),
span: outer_span,
..
})
| Node::ImplItem(ImplItem {
kind: ImplItemKind::Const(ty, ..),
span: outer_span,
..
})
| Node::ForeignItem(ForeignItem {
kind: ForeignItemKind::Static(ty, ..),
span: outer_span,
..
}) => until_within(*outer_span, ty.span),
// With generics and bounds.
Node::Item(Item {
kind: ItemKind::Trait(_, _, generics, bounds, _),
span: outer_span,
..
})
| Node::TraitItem(TraitItem {
kind: TraitItemKind::Type(bounds, _),
generics,
span: outer_span,
..
}) => {
let end = if let Some(b) = bounds.last() { b.span() } else { generics.span };
until_within(*outer_span, end)
}
// Other cases.
Node::Item(item) => match &item.kind {
ItemKind::Use(path, _) => {
// Ensure that the returned span has the item's SyntaxContext, and not the
// SyntaxContext of the path.
path.span.find_ancestor_in_same_ctxt(item.span).unwrap_or(item.span)
}
_ => named_span(item.span, item.ident, item.kind.generics()),
},
Node::Variant(variant) => named_span(variant.span, variant.ident, None),
Node::ImplItem(item) => named_span(item.span, item.ident, Some(item.generics)),
Node::ForeignItem(item) => match item.kind {
ForeignItemKind::Fn(decl, _, _) => until_within(item.span, decl.output.span()),
_ => named_span(item.span, item.ident, None),
},
Node::Ctor(_) => return self.opt_span(self.parent_id(hir_id)),
Node::Expr(Expr {
kind: ExprKind::Closure(Closure { fn_decl_span, .. }),
span,
..
}) => {
// Ensure that the returned span has the item's SyntaxContext.
fn_decl_span.find_ancestor_inside(*span).unwrap_or(*span)
}
_ => self.span_with_body(hir_id),
};
debug_assert_eq!(span.ctxt(), self.span_with_body(hir_id).ctxt());
Some(span)
}

@petrochenkov petrochenkov removed their assignment Dec 11, 2023
@bors
Copy link
Collaborator

bors commented Dec 13, 2023

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

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 15, 2023

📌 Commit bc1ca6b has been approved by cjgillot

It is now in the queue for this repository.

@bors bors 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 Dec 15, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2023
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#118396 (Collect lang items from AST, get rid of `GenericBound::LangItemTrait`)
 - rust-lang#118727 (Don't pass lint back out of lint decorator)
 - rust-lang#118956 (Make CStr documentation consistent ("nul" instead of "null"))
 - rust-lang#118981 (Remove an unneeded allocation)
 - rust-lang#118998 (Link to is_benchmark from the Ipv6Addr::is_global documentation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1d54949 into rust-lang:master Dec 16, 2023
@rustbot rustbot added this to the 1.76.0 milestone Dec 16, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2023
Rollup merge of rust-lang#118396 - compiler-errors:ast-lang-items, r=cjgillot

Collect lang items from AST, get rid of `GenericBound::LangItemTrait`

r? `@cjgillot`
cc rust-lang#115178

Looking forward, the work to remove `QPath::LangItem` will also be significantly more difficult, but I plan on doing it as well. Specifically, we have to change:
1. A lot of `rustc_ast_lowering` for things like expr `..`
2. A lot of astconv, since we actually instantiate lang and non-lang paths quite differently.
3. A ton of diagnostics and clippy lints that are special-cased via `QPath::LangItem`

Meanwhile, it was pretty easy to remove `GenericBound::LangItemTrait`, so I just did that here.
@pnkfelix
Copy link
Member

If I'm interpreting the data on PR #119002 correctly, it seems like this PR was the primary cause of some ~38 smallish (<0.5%) regressions, mostly to check builds, that were flagged in that rollup.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants