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

Feature gate the non_exhaustive_omitted_patterns lint #89428

Merged
merged 1 commit into from
Oct 10, 2021
Merged

Feature gate the non_exhaustive_omitted_patterns lint #89428

merged 1 commit into from
Oct 10, 2021

Conversation

DevinR528
Copy link
Contributor

@DevinR528 DevinR528 commented Oct 1, 2021 •
edited
Loading

Fixes #89374

Add the machinery to gate the new non_exhaustive_omitted_patterns lint.

relates to #89105 and #89423

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2021
@DevinR528
Copy link
Contributor Author

cc @Nadrieril @camelid

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@DevinR528
Copy link
Contributor Author

An, even more, succinct error report:

tidy error: /checkout/compiler/rustc_feature/src/active.rs:682: no tracking issue for feature non_exhaustive_omitted_patterns some tidy checks failed

@bors
Copy link
Contributor

bors commented Oct 5, 2021

☔ The latest upstream changes (presumably #89549) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 5, 2021

r? @Nadrieril

@rust-highfive rust-highfive assigned Nadrieril and unassigned oli-obk Oct 5, 2021
@Nadrieril Nadrieril mentioned this pull request Oct 5, 2021
Open
14 tasks
@Nadrieril
Copy link
Member

Ok, I created the tracking issue: #89554
I also propose we rename the feature to non_exhaustive_omitted_patterns_lint, otherwise it sounds like we're making changes to how non_exhaustive works.

@Nadrieril Nadrieril added the F-non_exhaustive_omitted_patterns_lint `#![feature(non_exhaustive_omitted_patterns_lint)]` label Oct 5, 2021
@DevinR528
Copy link
Contributor Author

DevinR528 commented Oct 5, 2021 •
edited
Loading

Sweet thanks!! I rebased and added the tracking issue number.

Should I change the name in the PR?

Edit: I realize now that you just meant the feature, not the actual attribute 🤦.

@Nadrieril
Copy link
Member

Yes please

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Nadrieril
Copy link
Member

Haha so many checks :D Seems like you also need to rename a test file

@camelid
Copy link
Member

camelid commented Oct 5, 2021

Also, please squash your commits once CI is passing :)

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2021
@DevinR528
Copy link
Contributor Author

squashed

Thanks for the help everybody!!!

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@DevinR528 DevinR528 force-pushed the reachable-featuregate branch 2 times, most recently from fa71b08 to 06f75f0 Compare October 7, 2021 15:27
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 7, 2021

☔ The latest upstream changes (presumably #89629) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Nadrieril
Copy link
Member

LGTM! @camelid unless you have any more comments, r=me

camelid
camelid reviewed Oct 8, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

After fixing the tracking issue declaration and squashing, r=Nadrieril,camelid. Thanks for doing this follow-up feature-gating!

compiler/rustc_feature/src/active.rs Outdated
@@ -678,6 +678,9 @@ declare_features! (
/// Allows `#[doc(cfg_hide(...))]`.
(active, doc_cfg_hide, "1.57.0", Some(43781), None),

/// Allows using the `non_exhaustive_omitted_patterns` lint.
(active, non_exhaustive_omitted_patterns_lint, "1.57.0", Some(89549), None),
Copy link
Member

Choose a reason for hiding this comment

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

#89549 isn't a tracking issue (which this field represents) -- it's a rollup PR. I'd suggest just changing this to None for now so we can get this merged.

Suggested change
(active, non_exhaustive_omitted_patterns_lint, "1.57.0", Some(89549), None),
(active, non_exhaustive_omitted_patterns_lint, "1.57.0", None, None),

Copy link
Member

Choose a reason for hiding this comment

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

Must be a copy/paste error, I did create a tracking issue here: #89554

Copy link
Member

@Nadrieril Nadrieril Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

And FYI tidy prevents merging this without a tracking issue, hence why I created one

Copy link
Member

@camelid camelid Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

And FYI tidy prevents merging this without a tracking issue, hence why I created one

Hmm, I wonder if that's new. I'm pretty sure I've seen feature gates merged without tracking issues before.

EDIT: It's not new; that check has been around since at least 2019. But there's this comment:

    // We allow rustc-internal features to omit a tracking issue.
    // To make tidy accept omitting a tracking issue, group the list of features
    // without one inside `// no-tracking-issue` and `// no-tracking-issue-end`.

src/test/ui/feature-gates/feature-gate-non_exhaustive_omitted_patterns_lint.rs
Comment on lines +18 to +21
//~^^^^^ ERROR the `non_exhaustive_omitted_patterns` lint is unstable
//~| ERROR the `non_exhaustive_omitted_patterns` lint is unstable
//~| ERROR the `non_exhaustive_omitted_patterns` lint is unstable
//~| ERROR the `non_exhaustive_omitted_patterns` lint is unstable
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit odd that 4 errors are being reported, but I think outside of the testsuite, rustc deduplicates error, so this should be fine. Either way, I don't think it's your code that is causing this -- it's probably the lint machinery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought that was odd and annoying also.

@DevinR528 DevinR528 force-pushed the reachable-featuregate branch 2 times, most recently from 4c4fb4b to 1ce6481 Compare October 8, 2021 21:15
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@DevinR528
Add feature gate to non_exhaustive_omitted_patterns lint
1433878 
Actually add the feature to the lints ui test
Add tracking issue to the feature declaration
Rename feature gate to non_exhaustive_omitted_patterns_lint
Add more omitted_patterns lint feature gate
@Nadrieril
Copy link
Member

@bors r=Nadrieril,camelid

@bors
Copy link
Contributor

bors commented Oct 9, 2021

📌 Commit 1433878 has been approved by Nadrieril,camelid

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 9, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2021
@matthiaskrgr
Rollup merge of rust-lang#89428 - DevinR528:reachable-featuregate, r=…
29fc7bd 
…Nadrieril,camelid

Feature gate the non_exhaustive_omitted_patterns lint

Fixes rust-lang#89374

Add the machinery to gate the new `non_exhaustive_omitted_patterns` lint.

relates to rust-lang#89105 and rust-lang#89423
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 10, 2021
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2021
@matthiaskrgr
Rollup merge of rust-lang#89428 - DevinR528:reachable-featuregate, r=…
924eff1 
…Nadrieril,camelid

Feature gate the non_exhaustive_omitted_patterns lint

Fixes rust-lang#89374

Add the machinery to gate the new `non_exhaustive_omitted_patterns` lint.

relates to rust-lang#89105 and rust-lang#89423
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 10, 2021
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 10, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2021
@bors
Auto merge of rust-lang#89739 - matthiaskrgr:rollup-kskwqy5, r=matthi…
41dfaaa 
…askrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#88374 (Fix documentation in Cell)
 - rust-lang#88713 (Improve docs for int_log)
 - rust-lang#89428 (Feature gate the non_exhaustive_omitted_patterns lint)
 - rust-lang#89438 (docs: `std::hash::Hash` should ensure prefix-free data)
 - rust-lang#89520 (Don't rebuild GUI test crates every time you run test src/test/rustdoc-gui)
 - rust-lang#89705 (Cfg hide no_global_oom_handling and no_fp_fmt_parse)
 - rust-lang#89713 (Fix ABNF of inline asm options)
 - rust-lang#89718 (Add #[must_use] to is_condition tests)
 - rust-lang#89719 (Add #[must_use] to char escape methods)
 - rust-lang#89720 (Add #[must_use] to math and bit manipulation methods)
 - rust-lang#89735 (Stabilize proc_macro::is_available)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cfa5391 into rust-lang:master Oct 10, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 10, 2021
@camelid camelid mentioned this pull request Oct 12, 2021
Closed
@DevinR528 DevinR528 deleted the reachable-featuregate branch October 12, 2021 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nadrieril Nadrieril Nadrieril left review comments

@camelid camelid camelid left review comments

Assignees

@Nadrieril Nadrieril

Labels
F-non_exhaustive_omitted_patterns_lint `#![feature(non_exhaustive_omitted_patterns_lint)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

Feature gate new non_exhaustive_omitted_patterns lint
8 participants
@DevinR528 @rust-highfive @rust-log-analyzer @bors @oli-obk @Nadrieril @camelid @rustbot