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

gate rustc_on_unimplemented under rustc_attrs #65794

Merged
merged 1 commit into from
Nov 7, 2019
Merged

gate rustc_on_unimplemented under rustc_attrs #65794

merged 1 commit into from
Nov 7, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 25, 2019 •
edited
Loading

Move rustc_on_implemented from the on_implemented gate to rustc_attrs as it is internal.

Closes #29628

r? @varkor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2019
@Centril Centril changed the title on_unimplemented is internal and doesn't need a feature gate. on_unimplemented is internal and doesn't need a tracking issue. Oct 25, 2019
@varkor
Copy link
Member

varkor commented Oct 25, 2019

We should remove the feature gate too: otherwise it's possible to enable the feature without any effect.

@Centril
Copy link
Contributor Author

Centril commented Oct 26, 2019

@varkor I don't know what you mean by "without any effect". The only technical change is the omission of the tracking issue. Would you prefer to move it to rustc_attrs?

@varkor
Copy link
Member

varkor commented Oct 26, 2019

I mean that you can currently write:

#![feature(on_unimplemented)]

And there won't be any error, which there ought to be after this change.

@Centril
Copy link
Contributor Author

Centril commented Oct 26, 2019

@varkor I didn't intend for that to be an error, only to remove any mention of the tracking issue. The attribute has to be gated by something, either the existing feature gate name or rustc_attrs. Do we want to gate all internal features with rustc_attrs? The current practice seems to be all over the place.

@varkor
Copy link
Member

varkor commented Oct 26, 2019

What other rustc_* attributes use their own feature gate rather than rustc_attrs?

@Centril
Copy link
Contributor Author

Centril commented Oct 26, 2019

Seems like it's a small minority:

  • rustc_const_unstable
  • rustc_diagnostic_item
  • rustc_paren_sugar

(rustc_ attributes are not the only internal attributes)

@varkor
Copy link
Member

varkor commented Oct 26, 2019 •
edited
Loading

Do you know if there's any particular reason for those? I'll investigate later if not. I would have thought that all rustc_* attributes could come under the same feature gate. (It's reasonable for other internal attributes to have different feature gates.)

@Centril
Copy link
Contributor Author

Centril commented Oct 26, 2019

Do you know if there's any particular reason for those?

No idea but my guess is that it would be happenstance.

@varkor
Copy link
Member

varkor commented Oct 26, 2019 •
edited
Loading

As far as I can tell, rustc_paren_sugar and rustc_const_unstable should be gated on rustc_attrs. rustc_diagnostic_item already is gated on rustc_attrs.

So I think we should gate rustc_on_unimplemented on rustc_attrs too (and change the others either here, or in a follow up).

@Centril Centril 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 26, 2019
@Centril Centril changed the title on_unimplemented is internal and doesn't need a tracking issue. gate rustc_on_unimplemented under rustc_attrs Oct 27, 2019
@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2019
@Centril
Copy link
Contributor Author

Centril commented Oct 27, 2019

@varkor Moved rustc_on_unimplemented for now. :)

@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@Centril Centril mentioned this pull request Oct 27, 2019
Merged
@rust-highfive

This comment has been minimized.

Sign in to view
@Centril
Copy link
Contributor Author

Centril commented Oct 28, 2019

@varkor I think it's good to go now.

@JohnCSimon

This comment has been minimized.

Sign in to view
@varkor
Copy link
Member

varkor commented Nov 2, 2019

r=me with typo fixed.

@varkor varkor 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 Nov 6, 2019
@Centril
Copy link
Contributor Author

Centril commented Nov 6, 2019

Typo fixed; @bors r=varkor rollup

@bors
Copy link
Contributor

bors commented Nov 6, 2019

📌 Commit 1c7595f has been approved by varkor

@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 Nov 6, 2019
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 6, 2019
@pietroalbini
Rollup merge of rust-lang#65794 - Centril:unimpl-internal, r=varkor
db341c1 
gate rustc_on_unimplemented under rustc_attrs

Move `rustc_on_implemented` from the `on_implemented` gate to `rustc_attrs` as it is internal.

Closes rust-lang#29628

r? @varkor
@pietroalbini pietroalbini mentioned this pull request Nov 6, 2019
Closed
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 6, 2019
@JohnTitor
Rollup merge of rust-lang#65794 - Centril:unimpl-internal, r=varkor
09287e0 
gate rustc_on_unimplemented under rustc_attrs

Move `rustc_on_implemented` from the `on_implemented` gate to `rustc_attrs` as it is internal.

Closes rust-lang#29628

r? @varkor
@JohnTitor JohnTitor mentioned this pull request Nov 6, 2019
Closed
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 7, 2019
@JohnTitor
Rollup merge of rust-lang#65794 - Centril:unimpl-internal, r=varkor
082a076 
gate rustc_on_unimplemented under rustc_attrs

Move `rustc_on_implemented` from the `on_implemented` gate to `rustc_attrs` as it is internal.

Closes rust-lang#29628

r? @varkor
@JohnTitor JohnTitor mentioned this pull request Nov 7, 2019
Merged
bors added a commit that referenced this pull request Nov 7, 2019
@bors
Auto merge of #66175 - JohnTitor:rollup-ihqk5vn, r=JohnTitor
7a76fe7 
Rollup of 12 pull requests

Successful merges:

 - #65794 (gate rustc_on_unimplemented under rustc_attrs)
 - #65945 (Optimize long-linker-command-line test)
 - #66044 (Improve uninit/zeroed lint)
 - #66076 (HIR docs: mention how to resolve method paths)
 - #66084 (Do not require extra LLVM backends for `x.py test` to pass)
 - #66111 (improve from_raw_parts docs)
 - #66114 (Improve std::thread::Result documentation)
 - #66117 (Fixed PhantomData markers in Arc and Rc)
 - #66146 (Remove unused parameters in `__thread_local_inner`)
 - #66147 (Miri: Refactor to_scalar_ptr out of existence)
 - #66162 (Fix broken link in README)
 - #66171 (Update link on CONTRIBUTING.md)

Failed merges:

r? @ghost
@bors bors merged commit 1c7595f into rust-lang:master Nov 7, 2019
@Centril Centril deleted the unimpl-internal branch November 7, 2019 07:19
@Centril Centril mentioned this pull request Nov 7, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varkor varkor varkor left review comments

Assignees

@varkor varkor

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tracking issue for on_unimplemented feature
5 participants
@Centril @varkor @rust-highfive @JohnCSimon @bors