Skip to content

Conversation

samueltardieu
Copy link
Member

Tests without unsafe must not run in edition 2024. Also, error messages have been modified to include the full attribute, so that a use of #[unsafe(no_mangle)] does not produce an error message containing #[no_mangle].

changelog: [no_mangle_attribute]: handle #[unsafe(no_mangle)] as well

@rustbot
Copy link
Collaborator

rustbot commented Oct 30, 2024

r? @Centri3

rustbot has assigned @Centri3.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 30, 2024
@samueltardieu samueltardieu mentioned this pull request Oct 30, 2024
4 tasks
@samueltardieu
Copy link
Member Author

Should the lint documentation also show #[unsafe(no_mangle)] instead of #[no_mangle]? The former works on both editions, starting from Rust 1.82, while the later won't work in Rust 2024.

@blyxyas
Copy link
Member

blyxyas commented Oct 30, 2024

I'm trying to find the reasoning for this change but I'm not being able to find, could someone link a PR or changelog?

@samueltardieu
Copy link
Member Author

There is an RFC on that: rust-lang/rfcs#3325

Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

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

I'm not sure if we should change the messages. It feels a bit extraneous even if it's correct; they often have placeholders and the sort, they're not supposed to be code. The user would already know it's an unsafe attribute, it feels a bit like saying unsafe { *null() }. In this case, there is no way to have the unsafe implied in an earlier scope, so it is a slightly different situation, but I think it still applies.

I suppose we wouldn't say "on an extern "Rust" function". Something like "unsafe attribute no_mangle set on ..." would be more consistent but that doesn't exactly feel right. Thoughts?

@samueltardieu
Copy link
Member Author

Ok, I'll let it as-is. If we have complaints later, we can always change it in the documentation.

@samueltardieu
Copy link
Member Author

@Centri3 Anything you would like me to change? Did I misunderstand your comment?

@Centri3
Copy link
Member

Centri3 commented Nov 3, 2024

I did mean in the error messages, but with further thinking this is probably fine imo. @bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2024

📌 Commit 8052451 has been approved by Centri3

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 3, 2024

⌛ Testing commit 8052451 with merge a1a9aae...

@bors
Copy link
Contributor

bors commented Nov 3, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3
Pushing a1a9aae to master...

@bors bors merged commit a1a9aae into rust-lang:master Nov 3, 2024
8 checks passed
@samueltardieu samueltardieu deleted the push-uoowutzkvsrk branch November 3, 2024 19:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants