Make `non-local-def` lint Allow by default by wesleywiser · Pull Request #124950 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Updates the non-local-def lint to be Allow by default. The lint currently triggers undesirably in a few places and we'd like to see that updated (or at least fully investigated) before this ships to stable Rust.

cc #124396
cc #124557
cc #124534
cc #120363

cc @Urgau as discussed in the T-compiler triage meeting today 🙂

r? @fmease

rustbot has assigned @fmease.
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 added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

May 10, 2024

Nominating for beta-backport so it can ride the 1.79 release train with #122747 which marked it Allow by default.

@bors rollup=never

When this lint was changed to Warn by default, it triggered a perf regression. Thus, this PR will likely trigger a perf improvement.

This comment has been minimized.

Should this PR be beta only? Since all the issues pointed are either already fixed or not an issue at all.

#124396 (fixed, backported on 1.79), #124534 (fixed, waited on backport), #124557 (not an FP, closed).

Ah, I didn't realize #124396 had been fixed. Then yeah I will retarget this to the beta branch.

Should this PR be closed then (I don't know if you can retarget a PR)?

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

Then yeah I will retarget this to the beta branch.

I've opened #125183 for other backports -- @wesleywiser do you want to add yours there?
(In a brief attempt, I found that this commit is not a clean cherry-pick to beta.)

Oh, the conflict is that beta doesn't have the rustdoc test from #124568. I'll just skip that for backport.

And since this PR wasn't formally approved yet, I'll switch to my compiler-contributor hat to say r=me. :)

bors added a commit to rust-lang-ci/rust that referenced this pull request

May 17, 2024

[beta] backports

Do not ICE on foreign malformed diagnostic::on_unimplemented rust-lang#124683
Fix more ICEs in diagnostic::on_unimplemented rust-lang#124875
rustdoc: use stability, instead of features, to decide what to show rust-lang#124864
Don't do post-method-probe error reporting steps if we're in a suggestion rust-lang#125100
Make non-local-def lint Allow by default rust-lang#124950

r? cuviper

Choose a reason for hiding this comment

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

#124950 (comment):

And since this PR wasn't formally approved yet, I'll switch to my compiler-contributor hat to say r=me. :)

Right, sorry about that. I hereby formally approve this PR after the fact ;) Got confused by the backporting story.

Thanks @wesleywiser for the fix!

Closing this PR then since it got successfully backported to beta in #125183 and since nightly doesn't need be patched.
Feel free to reopen if I misunderstood the situation ^^'.

Labels

beta-accepted