Skip to content

fix: no generate unused generic params in trait sign#22519

Merged
A4-Tacks merged 2 commits into
rust-lang:masterfrom
A4-Tacks:gen-trait-from-impl-gen-args
Jun 4, 2026
Merged

fix: no generate unused generic params in trait sign#22519
A4-Tacks merged 2 commits into
rust-lang:masterfrom
A4-Tacks:gen-trait-from-impl-gen-args

Conversation

@A4-Tacks

@A4-Tacks A4-Tacks commented Jun 3, 2026

Copy link
Copy Markdown
Member

Example

struct Foo<T, const N: usize>([T; N]);

impl<T, const N: usize> F$0oo<T, N> {
    fn spec_len(&self) -> usize {
        N
    }
}

Before this PR

struct Foo<T, const N: usize>([T; N]);

trait SpecLen<T, const N: usize> {
    fn spec_len(&self) -> usize;
}

impl<T, const N: usize> SpecLen<T, N> for Foo<T, N> {
    fn spec_len(&self) -> usize {
        N
    }
}

After this PR

struct Foo<T, const N: usize>([T; N]);

trait SpecLen {
    fn spec_len(&self) -> usize;
}

impl<T, const N: usize> SpecLen for Foo<T, N> {
    fn spec_len(&self) -> usize {
        N
    }
}

Example
---
```rust
struct Foo<T, const N: usize>([T; N]);

impl<T, const N: usize> F$0oo<T, N> {
    fn spec_len(&self) -> usize {
        N
    }
}
```

**Before this PR**

```rust
struct Foo<T, const N: usize>([T; N]);

trait SpecLen<T, const N: usize> {
    fn spec_len(&self) -> usize;
}

impl<T, const N: usize> SpecLen<T, N> for Foo<T, N> {
    fn spec_len(&self) -> usize {
        N
    }
}
```

**After this PR**

```rust
struct Foo<T, const N: usize>([T; N]);

trait SpecLen {
    fn spec_len(&self) -> usize;
}

impl<T, const N: usize> SpecLen for Foo<T, N> {
    fn spec_len(&self) -> usize {
        N
    }
}
```
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2026

@ChayimFriedman2 ChayimFriedman2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM minus one nit.

View changes since this review

Comment on lines +198 to +200
.all()
.file_ranges()
.any(|it| trait_ranges.iter().any(|range| range.contains_range(it.range)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
.all()
.file_ranges()
.any(|it| trait_ranges.iter().any(|range| range.contains_range(it.range)))
.at_least_one()

We don't actually need to check whether they're in the range, since they're declared in it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't actually need to check whether they're in the range, since they're declared in it.

This is necessary because the references in self-type and fn-body need to be ignored

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, then references in the body of consts/type aliases should also be ignored.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because const and type exist in both trait and impl after generation, they are retained

It should be up to the user to decide which one to adopt

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think they should be retained, but, fair enough.

.assoc_item_list()
.into_iter()
.flat_map(|list| list.assoc_items())
.map(item_in_trait_range)

@Shourya742 Shourya742 Jun 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't we need to check for where clause as well?

Consider something like this:

impl<T> Foo<T> where T: Copy { fn foo() {} } 

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

True. I think it's better to have a negative list, i.e. a list of excluded ranges.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea

@Shourya742 Shourya742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@A4-Tacks A4-Tacks added this pull request to the merge queue Jun 4, 2026
Merged via the queue into rust-lang:master with commit aadee11 Jun 4, 2026
18 checks passed
@A4-Tacks A4-Tacks deleted the gen-trait-from-impl-gen-args branch June 4, 2026 11:53
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants