Skip to content

feat: introduce path_to_local_with_projections #15396

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ada4a
Copy link
Contributor

@ada4a ada4a commented Aug 2, 2025

As suggested in #15268 (comment)

WIP because:

changelog: none

r? @y21

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 2, 2025
@ada4a ada4a force-pushed the path-to-local-with-projections branch from 4c1f627 to f4deea5 Compare August 3, 2025 10:35
@samueltardieu
Copy link
Member

samueltardieu commented Aug 6, 2025

Won't those changes make double_ended_iterator_last and filter_next lint new cases? Shouldn't there be new tests to account for this?

@ada4a
Copy link
Contributor Author

ada4a commented Aug 6, 2025

double_ended_iterator_last

Yes, but the no-longer-unfixable test case does pretty much that -- it checks that the suggestion goes through a field (in this case, tuple element) projection. I tried modifying the test case to test indexing projection as well:

let v = vec![S("four"), S("five"), S("six")];
- let v = (DropDeIterator(v.into_iter()), 42);
+ let v = [(DropDeIterator(v.into_iter()), 42)];
- println!("Last element is {}", v.0.last().unwrap().0);
+ println!("Last element is {}", v[0].0.last().unwrap().0);
//~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`

But that broke because one can't move out of an array by indexing, which last requires since it consumes the iterator.

filter_next

I think not, because the function that the lint used previously is identical to the new path_to_local_with_projections, so no new cases will be detected

@ada4a ada4a force-pushed the path-to-local-with-projections branch 2 times, most recently from 80d3c1f to d93599f Compare August 7, 2025 21:56
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Looks good to me, I agree that the tests are sufficient. In eager_transmute/filter_next there's no behavioral change since it's the same function just factored out and double_ended_iterator_last indeed already has a test that exercises this change, like you said 👍

Could you squash?

combine two similar arms

use in `eager_transmute`

use in `double_ended_iterator_last`

use different numbers in the new test case to avoid possible confusion

move the other "unfixable" case as well; it shouldn't lint anyway, so
having it in the main test file is fine
@ada4a ada4a force-pushed the path-to-local-with-projections branch from d93599f to 04606e2 Compare August 9, 2025 18:28
@ada4a ada4a changed the title WIP: feat: introduce path_to_local_with_projections feat: introduce path_to_local_with_projections Aug 9, 2025
@ada4a
Copy link
Contributor Author

ada4a commented Aug 9, 2025

squashed (and rebased)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants