Skip to content

fix sorting of use statements with raw identifiers #3795

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

calebcartwright
Copy link
Member

Resolves #3791

Does this need to be version gated? It looks more like a sorting bug fix IMO, but I can certainly add a version gate if this is indeed a formatting change.

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. The code LGTM, but as you mentioned I think that we need to version gate this. If that would require a lot of work, then I will create a branch for 2.0 and change the base to that branch.

@calebcartwright
Copy link
Member Author

If that would require a lot of work

The challenge from my perspective is how to make the config version value accessible in the cmp implementation for UseSegment so that it is available when doing the comparison.

A relatively quick and easy way I can see to do that would be to include the Version value in the UseSegment variants, though that seems like it would be a lot of duplicative representation of the version value.

Something like:

pub(crate) enum UseSegment {
    Ident(String, Option<String>, Version),
    Slf(Option<String>, Version),
    Super(Option<String>, Version),
    Crate(Option<String>, Version),
    Glob,
    List(Vec<UseTree>),
}

And then in cmp we could do:

        match (self, other) {
            (&Slf(ref a), &Slf(ref b))
            | (&Super(ref a), &Super(ref b))
            | (&Crate(ref a), &Crate(ref b)) => match (a, b) {
                (Some(sa), Some(sb)) => {
                    if self.version == Version::One {
                        sa.cmp(sb)
                    } else {
                        sa.trim_start_matches("r#").cmp(sb.trim_start_matches("r#"))
                    }
                }
                (_, _) => a.cmp(b),
            },
           ....

Thoughts?

@topecongiro topecongiro changed the base branch from master to rustfmt-2.0 September 24, 2019 00:32
@topecongiro topecongiro merged commit 1e7f259 into rust-lang:rustfmt-2.0 Sep 24, 2019
@topecongiro
Copy link
Contributor

Thank you for the elaboration. Given the amount of work and the fact that we will remove the version gating code once we prepare for 2.0, I would rather merge it to the branch for 2.0 and worry about merging-disaster later 😛 Thank you again for your work!

@calebcartwright
Copy link
Member Author

SGTM!

@calebcartwright calebcartwright deleted the raw-ident-in-use branch September 24, 2019 00:48
calebcartwright added a commit to calebcartwright/rustfmt that referenced this pull request Nov 7, 2019
@calebcartwright calebcartwright added the 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release label Oct 30, 2021
@ytmimi
Copy link
Contributor

ytmimi commented Apr 3, 2022

Hey @calebcartwright, thinking about how we could backport this and include the version gates. Maybe we could update UseSegment as follows:

pub(crate) enum UseSegmentKind {
    Ident(String, Option<String>),
    Slf(Option<String>),
    Super(Option<String>),
    Crate(Option<String>),
    Glob,
    List(Vec<UseTree>),
}

pub(crate) struct UseSegment {
    kind: UseSegmentKind,
    version: Version
}

Let me know if you think this is the approach we should take or if you think passing the version to each enum variant would be a better solution.

@calebcartwright
Copy link
Member Author

Let me know if you think this is the approach we should take or if you think passing the version to each enum variant would be a better solution.

I'm open to just about anything, but the cleaner/simpler the better!

@ytmimi ytmimi mentioned this pull request Jun 13, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants