Skip to content

The #[panic_handler] attribute can be applied to non-functions #54997

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 13, 2018

Conversation

davidtwco
Copy link
Member

Fixes #54896.

This commit extends the existing lang items functionality to assert
that the #[lang_item] attribute is only found on the appropriate item
for any given lang item. That is, language items representing traits
must only ever have their corresponding attribute placed on a trait, for
example.

r? @nagisa

This commit extends the existing lang items functionality to assert
that the `#[lang_item]` attribute is only found on the appropriate item
for any given lang item. That is, language items representing traits
must only ever have their corresponding attribute placed on a trait, for
example.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2018
@nagisa
Copy link
Member

nagisa commented Oct 11, 2018

@bors r+

This looks good to me.

@bors
Copy link
Collaborator

bors commented Oct 11, 2018

📌 Commit 2ecce7c has been approved by nagisa

@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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2018
PanicImplLangItem, "panic_impl", panic_impl;
PanicFnLangItem, "panic", panic_fn, Target::Fn;
PanicBoundsCheckFnLangItem, "panic_bounds_check", panic_bounds_check_fn, Target::Fn;
PanicInfoLangItem, "panic_info", panic_info, Target::Struct;
Copy link
Member

Choose a reason for hiding this comment

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

In the future there might be a language item that could possibly be some sort of type definition, not specifically a struct.

This is especially notable here. panic_info in no_core could easily be defined to be an enum as well for example – in fact, the only reason this is a language item is to facilitate type-checking the panic_impl function.

Yet, I do not see a nice way out here, and since language items are not stable anyway it seems fine to keep it restrictive for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this could be extended to have a Target::Any that allows the attribute to be applied to anything, as was the behaviour before this PR.

Alternatively, the definition of Target could be changed to a u32 and have each bit represent a variant (similar to AdtFlags) and then the language item definitions could | together these “variants” to indicate that the attribute could be applied to either construct.

I agree that, for the moment, the simplest solution would be to keep this more restrictive approach and change if necessary.

@bors
Copy link
Collaborator

bors commented Oct 13, 2018

⌛ Testing commit 2ecce7c with merge 24faa97...

bors added a commit that referenced this pull request Oct 13, 2018
The #[panic_handler] attribute can be applied to non-functions

Fixes #54896.

This commit extends the existing lang items functionality to assert
that the `#[lang_item]` attribute is only found on the appropriate item
for any given lang item. That is, language items representing traits
must only ever have their corresponding attribute placed on a trait, for
example.

r? @nagisa
@bors
Copy link
Collaborator

bors commented Oct 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 24faa97 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants