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

Fix std_instead_of_core for ListStem use #12474

Closed

Conversation

MarcusGrass
Copy link
Contributor

Fixes #12468.

I don't know if this fix is really worth it, it can hopefully be improved if someone knows how to implement proper look-ahead, essentially:

On check_path paths come in one by one as described in #12468. The problematic paths are all contained within an Item which can be gotten by check_item and has the kind UseKind::ListStem.

Simple then, if that kind of item comes in, start collecting Path's in it, and when the last path has passed, emit the diagnostics.

But hold up! There's no way (that I know of), to know if it's the last path in the item.

We can know that all relevant paths are fully contained within the UseKind::ListStem-span, but not when the last path is found. Since that span could contain a lot of other cruft and still be valid, like a trailing long comment.

The pretty naive, and chosen, solution is to implement pretty much all checks, and if something is encountered that has a span that's outside of the currently 'in progress' use item, then close and emit the stored diagnostics.

If anyone has suggestions for better look-ahead then please feel welcome to pitch!

Also, this is pretty much a non-issue, since rustfmt will automatically split these kinds of imports, so getting a ~500 diff on a non-issue may just not be worth the hassle. Since these aren't trivially fixable it also disables the rustfix test for std_instead_of_core. All in all, I'm not ecstatic about the change.

changelog: [std_instead_of_core]: Fixes bad suggestions for ListStem uses.

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2024

r? @blyxyas

rustbot has assigned @blyxyas.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 12, 2024
@bors
Copy link
Contributor

bors commented Apr 1, 2024

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

@blyxyas
Copy link
Member

blyxyas commented Apr 1, 2024

Idea for a simpler iteration system.
After checking the internal structure of the following code:

use std::{io::Write, fmt::Result};

We get this:

Item {
    ident: #0,
    owner_id: DefId(0:21 ~ std_instead_of_core[d9ec]::{use#1}),
    kind: Use(
        Path {
            span: tests/ui/std_instead_of_core.rs:8:5: 8:34 (#0),
            res: [],
            segments: [],
        },
        ListStem,
    ),
    span: tests/ui/std_instead_of_core.rs:8:1: 8:35 (#0),
    vis_span: tests/ui/std_instead_of_core.rs:8:1: 8:1 (#0),
}
Item {
    ident: Write#0,
    owner_id: DefId(0:22 ~ std_instead_of_core[d9ec]::{use#2}),
    kind: Use(
        Path {
            span: tests/ui/std_instead_of_core.rs:8:11: 8:20 (#0),
            res: [
                Def(
                    Trait,
                    DefId(1:3587 ~ std[ef9c]::io::Write),
                ),
            ],
            segments: [
                PathSegment {
                    ident: std#0,
                    hir_id: HirId(DefId(0:22 ~ std_instead_of_core[d9ec]::{use#2}).1),
                    res: Def(
                        Mod,
                        DefId(1:0 ~ std[ef9c]),
                    ),
                    args: None,
                    infer_args: false,
                },
                PathSegment {
                    ident: io#0,
                    hir_id: HirId(DefId(0:22 ~ std_instead_of_core[d9ec]::{use#2}).2),
                    res: Def(
                        Mod,
                        DefId(1:2503 ~ std[ef9c]::io),
                    ),
                    args: None,
                    infer_args: false,
                },
                PathSegment {
                    ident: Write#0,
                    hir_id: HirId(DefId(0:22 ~ std_instead_of_core[d9ec]::{use#2}).3),
                    res: Err,
                    args: None,
                    infer_args: false,
                },
            ],
        },
        Single,
    ),
    span: tests/ui/std_instead_of_core.rs:8:11: 8:20 (#0),
    vis_span: tests/ui/std_instead_of_core.rs:8:1: 8:1 (#0),
}
Item {
    ident: Result#0,
    owner_id: DefId(0:23 ~ std_instead_of_core[d9ec]::{use#3}),
    kind: Use(
        Path {
            span: tests/ui/std_instead_of_core.rs:8:22: 8:33 (#0),
            res: [
                Def(
                    TyAlias,
                    DefId(2:10207 ~ core[154f]::fmt::Result),
                ),
            ],
            segments: [
                PathSegment {
                    ident: std#0,
                    hir_id: HirId(DefId(0:23 ~ std_instead_of_core[d9ec]::{use#3}).1),
                    res: Def(
                        Mod,
                        DefId(1:0 ~ std[ef9c]),
                    ),
                    args: None,
                    infer_args: false,
                },
                PathSegment {
                    ident: fmt#0,
                    hir_id: HirId(DefId(0:23 ~ std_instead_of_core[d9ec]::{use#3}).2),
                    res: Def(
                        Mod,
                        DefId(5:4834 ~ alloc[c1b8]::fmt),
                    ),
                    args: None,
                    infer_args: false,
                },
                PathSegment {
                    ident: Result#0,
                    hir_id: HirId(DefId(0:23 ~ std_instead_of_core[d9ec]::{use#3}).3),
                    res: Err,
                    args: None,
                    infer_args: false,
                },
            ],
        },
        Single,
    ),
    span: tests/ui/std_instead_of_core.rs:8:22: 8:33 (#0),
    vis_span: tests/ui/std_instead_of_core.rs:8:1: 8:1 (#0),
}

We can read that on their owner ID that the number with the # (I'm not sure what the number is, I've been looking at implementations of Display/Debug for DefId and similar structs but I can't seem to find the origin of it).

You could, for a ListStem, iterate over the following item checking for wherever that number is represented in memory, and checking that the span is still contained on the ListStem.

Maybe this new approach could make the 589-lines diff simpler :)

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 20, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@MarcusGrass MarcusGrass force-pushed the mg/fix-std-instead-of-core-use branch from fcc17bd to d7af836 Compare April 20, 2024 16:00
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 20, 2024
@bors
Copy link
Contributor

bors commented Jun 13, 2024

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

@MarcusGrass
Copy link
Contributor Author

I did make some attempts, but it didn't turn out very well, I'll close this since I'm not working on it

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.

std_instead_of_core creates an invalid fix on stacked imports
4 participants