Skip to content

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

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

Jump to bottom
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.

# 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
# for free to join this conversation on GitHub. Already have an account? # 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