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.

Sign up for GitHub

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

Jump to bottom

Make non-local-def lint Allow by default #124950

Closed
wants to merge 1 commit into from
Closed

Make non-local-def lint Allow by default #124950

wants to merge 1 commit into from

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented May 10, 2024 •
edited
Loading

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 🙂

@rustbot
Copy link
Collaborator

rustbot commented May 10, 2024

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 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
@wesleywiser wesleywiser added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 10, 2024
@wesleywiser
Copy link
Member Author

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

@wesleywiser
Copy link
Member Author

@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.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@wesleywiser wesleywiser force-pushed the allow_non-local-defs branch from 22ba917 to 23015b9 Compare May 10, 2024 01:15
@Urgau
Copy link
Member

Urgau commented May 10, 2024 •
edited
Loading

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).

@wesleywiser
Copy link
Member Author

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

@fmease
Copy link
Member

fmease commented May 15, 2024 •
edited
Loading

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

@apiraino
Copy link
Contributor

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

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 16, 2024
@cuviper
Copy link
Member

cuviper commented May 16, 2024

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.)

@cuviper
Copy link
Member

cuviper commented May 16, 2024

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. :)

@cuviper cuviper mentioned this pull request May 16, 2024
Merged
@cuviper cuviper added this to the 1.79.0 milestone May 16, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 16, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 17, 2024
@bors
Auto merge of rust-lang#125183 - cuviper:beta-next, r=cuviper
0436bf1 
[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
fmease
fmease approved these changes May 19, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

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.

@fmease
Copy link
Member

fmease commented May 19, 2024 •
edited
Loading

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 ^^'.

@fmease fmease closed this May 19, 2024
@fmease fmease removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease approved these changes

Assignees

@fmease fmease

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.
7 participants
@wesleywiser @rustbot @rust-log-analyzer @Urgau @fmease @apiraino @cuviper