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

"path starts with module" lint fails with rustfix when triggered in macro #51205

Closed
killercup opened this issue May 30, 2018 · 7 comments
Closed
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-rust-2018-preview Area: The 2018 edition preview T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management

Comments

@killercup
Copy link
Member

killercup commented May 30, 2018

Thanks to @AdamNiederer opening rust-lang/rustfix#114, we found that it's not possible to fully upgrade faster to the 2018 edition with rustfix.

It fails with "Cannot replace slice of data that was already replaced", because we get the exact same suggestion twice -- because it's part of an expanded macro.

It looks to me like in addition to the changes @alexcrichton introduced in #50665, we'll also need to add a macro check here and here.

There are three solutions. The easy one is to not mark the suggestions as MachineApplicable when the span they come from is in macro. The hard ones are to only trigger this once (or just once as MachineApplicable), or to omit exact duplicates in rustfix (and hope for the best).

@alexcrichton
Copy link
Member

Your analysis looks spot on to me, thanks! Cc @nikomatsakis as well

Id probably err on the side of simicity and remove the machine applicable part of the warning when it comes from a macro, rustfix should be totally fine to have an exception or two it can't fix if they aren't super common

@nikomatsakis
Copy link
Contributor

cc @Manishearth — we had been talking about basically doing this check automatically as part of the suggestion machinery (#48704). I'm not sure what's the status there though, should probably figure that out.

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management A-rust-2018-preview Area: The 2018 edition preview labels May 30, 2018
@Manishearth
Copy link
Member

Er, so there are two different things here and we should be careful not to mix them up.

One is that suggestions should not be marked as machine applicable if they touch macros. The machinery should do this itself, but you still need to handle the macro-ness of span_to_snippets which may not interact. I suspect @zackmdavis may be interested in helping out here.


The other is that most lints shouldn't be triggered for macros at all. However, edition breakage lints are future incompatibility lints and they do not fall in this category.

The status of "most lints shouldn't be triggered for macros at all" is that a different PR (knowingly) broke the infrastructure that lets us do this check, and we're waiting for a fix on this. See #50050 (comment) .

@Mark-Simulacrum Mark-Simulacrum added this to the Rust 2018 Alpha milestone Jun 7, 2018
@Mark-Simulacrum
Copy link
Member

We should fix the failure to migrate due to macros here - rustfix shouldn't abort on it

@Mark-Simulacrum
Copy link
Member

We don't currently abort on this so I'm removing from milestone; transition for faster consisted of a run and 3-4 lines of macro code being updated to use crate:: syntax.

@Mark-Simulacrum Mark-Simulacrum removed this from the Rust 2018 Alpha milestone Jun 7, 2018
@nrc nrc added the A-edition-2018-lints Area: Lints supporting the 2018 edition label Jul 2, 2018
@alexcrichton alexcrichton added this to the Rust 2018 Preview 2 milestone Jul 11, 2018
@alexcrichton
Copy link
Member

I commented at #51211 (comment) with a minimal example of this:

#![feature(rust_2018_preview)]
#![warn(rust_2018_compatibility)]

macro_rules! foo {
    () => {
        {
            use myself::f;
            f();
        }
    }
}

mod myself {
    pub fn f() {}
}

fn main() {
    foo!();
    foo!();
}

and I've also submitted rust-lang/rustfix#131 to add a fix to rustfix for now for this

@alexcrichton
Copy link
Member

Ok this issue itself is specifically fixed by rust-lang/rustfix#131 and macros and edition lints are otherwise covered by #48855 and #48704, so I'm gonna close this in favor of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-rust-2018-preview Area: The 2018 edition preview T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management
Projects
None yet
Development

No branches or pull requests

6 participants