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

Mark associated constants used in equality constraints as used #127505

Closed
wants to merge 1 commit into from

Conversation

trevyn
Copy link
Contributor

@trevyn trevyn commented Jul 9, 2024

Closes #126729
cc #92827

@rustbot
Copy link
Collaborator

rustbot commented Jul 9, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Jul 9, 2024
Comment on lines +862 to +883
for bound in ty.bounds {
if let hir::GenericBound::Trait(polytraitref, _) = bound
&& 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));
}
}
}
}
}
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

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.

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

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.

@cjgillot cjgillot 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 Jul 13, 2024
@trevyn
Copy link
Contributor Author

trevyn commented Jul 20, 2024

Superseded by #127714

@trevyn trevyn closed this Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

Associated constants used in equality constraints should be considered as used
5 participants