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.

Sign up for GitHub

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

Jump to bottom

Mark associated constants used in equality constraints as used #127505

Closed
wants to merge 1 commit into from
Closed

Mark associated constants used in equality constraints as used #127505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions compiler/rustc_passes/src/dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,31 @@ fn check_item<'tcx>(
// global_asm! is always live.
worklist.push((id.owner_id.def_id, ComesFromAllowExpect::No));
}
DefKind::OpaqueTy => {
if let hir::ItemKind::OpaqueTy(ty) = tcx.hir().item(id).kind {
for bound in ty.bounds {
if let hir::GenericBound::Trait(polytraitref, _) = bound
Copy link
Contributor

@mu001999 mu001999 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What if we do this in visit_poly_trait_ref? For example, things like dyn Tr<X = ()> and fn foo<T: Tr<X = ()>> should also mark X live.

&& let Some(pathsegment) = polytraitref.trait_ref.path.segments.last()
&& let Some(args) = pathsegment.args
{
for constraint in args.constraints {
if let Some(item) = tcx
.associated_items(pathsegment.res.def_id())
.filter_by_name_unhygienic(constraint.ident.name)
.find(|i| {
i.kind == ty::AssocKind::Const
&& i.ident(tcx).normalize_to_macros_2_0()
== constraint.ident
})
&& let Some(local_def_id) = item.def_id.as_local()
{
worklist.push((local_def_id, ComesFromAllowExpect::No));
}
}
}
}
}
Comment on lines +862 to +883
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we get this for free by just visiting the ty with the hir visitor from this module?

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 don’t think so, because the def_id of the associated item actually doesn’t show up anywhere in the hir of the ty. I could totally be missing something, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, this is a more fundamental problem. https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=dd0c2930c397acba2692b51a633b367d will still fail after your PR. If these could be used in trait objects we'd probably see the issue there, too.

Considering that

#![deny(dead_code)]

trait Tr {
    type T;
}

impl Tr for () {
    type T = ();
}

fn foo<T: Tr>(_: T) { } 


fn main() {
    foo(());
}

passes, I'm assuming that if a trait is marked as used, all its assoc types are marked as used. We should do the same for assoc consts, not sure how that didn't happen or if there was an explicit reason for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like linting for associated consts was added recently in #125572, cc @mu001999 @pnkfelix

You're right that if a trait is marked as used, all its assoc types are marked as used.

Copy link
Contributor

@mu001999 mu001999 Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm assuming that if a trait is marked as used, all its assoc types are marked as used.

It was made in #126618. I think it's not the best solution, i.e., marking assoc const/tys live if the trait is used, considering that

trait Trait {
    const X: i32;
    fn foo(&self) {}
}

impl Trait for () {
    const X: i32 = 0;
}

fn main() {
    ().foo();
}

The assoc const is not used and we can lint it.

}
_ => {}
}
}
Expand Down
44 changes: 44 additions & 0 deletions tests/ui/associated-consts/equality-unused-issue-126729.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
//@ check-pass

#![feature(associated_const_equality)]
#![deny(dead_code)]

trait Tr {
const I: i32;
}

impl Tr for () {
const I: i32 = 1;
}

fn foo() -> impl Tr<I = 1> {}

trait Tr2 {
const J: i32;
const K: i32;
}

impl Tr2 for () {
const J: i32 = 1;
const K: i32 = 1;
}

fn foo2() -> impl Tr2<J = 1, K = 1> {}

mod t {
pub trait Tr3 {
const L: i32;
}

impl Tr3 for () {
const L: i32 = 1;
}
}

fn foo3() -> impl t::Tr3<L = 1> {}

fn main() {
foo();
foo2();
foo3();
}
Loading