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.

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

More unsafe attr verification #127543

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

carbotaniuman
Copy link
Contributor

@carbotaniuman carbotaniuman commented Jul 10, 2024

This code denies unsafe on attributes such as #[test] and #[ignore], while also changing the MetaItem parsing so unsafe in args like #[allow(unsafe(dead_code))] is not accidentally allowed.

Tracking:

@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2024

r? @estebank

rustbot has assigned @estebank.
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 Jul 10, 2024
@@ -373,7 +373,7 @@ impl<'a> Parser<'a> {
/// MetaItem = SimplePath ( '=' UNSUFFIXED_LIT | '(' MetaSeq? ')' )? ;
/// MetaSeq = MetaItemInner (',' MetaItemInner)* ','? ;
/// ```
pub fn parse_meta_item(&mut self) -> PResult<'a, ast::MetaItem> {
pub fn parse_meta_item(&mut self, allow_leading_unsafe: bool) -> PResult<'a, ast::MetaItem> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a bool, can we make this an enum AllowLeadingUnsafe { Yes, No }? It makes it easier to read the calls in isolation without having to check the signature.

Comment on lines 1 to 7
error: `cfg` is not an unsafe attribute
--> $DIR/extraneous-unsafe-attributes.rs:5:3
|
LL | #[unsafe(cfg(any()))]
| ^^^^^^
|
= note: extraneous unsafe is not allowed in attributes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
error: `cfg` is not an unsafe attribute
--> $DIR/extraneous-unsafe-attributes.rs:5:3
|
LL | #[unsafe(cfg(any()))]
| ^^^^^^
|
= note: extraneous unsafe is not allowed in attributes
error: `cfg` is not an unsafe attribute
--> $DIR/extraneous-unsafe-attributes.rs:5:3
|
LL | #[unsafe(cfg(any()))]
| ^^^^^^ --- this is not an unsafe attribute
| |
| extraneous unsafe is not allowed in attributes

@estebank
Copy link
Contributor

r=me after the bool change

@estebank estebank added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 12, 2024
@bors
Copy link
Contributor

bors commented Jul 14, 2024

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

@carbotaniuman
Copy link
Contributor Author

@rustbot reviewer

@carbotaniuman
Copy link
Contributor Author

@rustbot review

@carbotaniuman
Copy link
Contributor Author

Oh I did that already, oops.

@traviscross
Copy link
Contributor

@rustbot labels -S-waiting-on-author -S-waiting-on-review

Looks like the bool change was made, so per the comment above...

@bors r=estebank rollup=always

@bors
Copy link
Contributor

bors commented Jul 29, 2024

📌 Commit 95d1101 has been approved by estebank

It is now in the queue for this repository.

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 29, 2024
@bors
Copy link
Contributor

bors commented Jul 29, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 29, 2024
@bors
Copy link
Contributor

bors commented Jul 29, 2024

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 29, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#109174 (Replace `io::Cursor::{remaining_slice, is_empty}`)
 - rust-lang#127290 (Fully document `rustdoc-json-types`)
 - rust-lang#128055 (std: unsafe-wrap personality::dwarf::eh)
 - rust-lang#128269 (improve cargo invocations on bootstrap)
 - rust-lang#128310 (Add missing periods on `BTreeMap` cursor `peek_next` docs)

Failed merges:

 - rust-lang#127543 (More unsafe attr verification)
 - rust-lang#128182 (handle no_std targets on std builds)

r? `@ghost`
`@rustbot` modify labels: rollup
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@traviscross
Copy link
Contributor

It's now been rebased; it looks reasonable to me, so let's try again...

@bors rollup=always r=estebank,traviscross 8eaf51b

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 30, 2024
…ification, r=estebank,traviscross

More unsafe attr verification

This code denies unsafe on attributes such as `#[test]` and `#[ignore]`, while also changing the `MetaItem` parsing so `unsafe` in args like `#[allow(unsafe(dead_code))]` is not accidentally allowed.

Tracking:

- rust-lang#123757
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#127543 (More unsafe attr verification)
 - rust-lang#128357 (Detect non-lifetime binder params shadowing item params)
 - rust-lang#128367 (CI: rfl: build the generated doctests and documentation)
 - rust-lang#128376 (Mark `Parser::eat`/`check` methods as `#[must_use]`)
 - rust-lang#128379 (the output in stderr expects panic-unwind)
 - rust-lang#128380 (make `///` doc comments compatible with naked functions)
 - rust-lang#128382 (cargo-miri: better error when we seem to run inside bootstrap but something is wrong)
 - rust-lang#128398 (tidy: Fix quote in error message)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy
failed on apple #128403 (comment)

This makes it possible for the `unsafe(...)` syntax to only be
valid at the top level, and the `NestedMetaItem`s will automatically
reject `unsafe(...)`.
@traviscross
Copy link
Contributor

@carbotaniuman pushed a fix; let's try again...

@bors rollup=iffy r=estebank,traviscross 49db8a5

@bors
Copy link
Contributor

bors commented Aug 1, 2024

⌛ Testing commit 49db8a5 with merge c0e3298...

@bors
Copy link
Contributor

bors commented Aug 1, 2024

☀️ Test successful - checks-actions
Approved by: estebank,traviscross
Pushing c0e3298 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 1, 2024
@bors bors merged commit c0e3298 into rust-lang:master Aug 1, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 1, 2024
@carbotaniuman carbotaniuman deleted the more_unsafe_attr_verification branch August 1, 2024 18:26
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c0e3298): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 758.41s -> 756.519s (-0.25%)
Artifact size: 336.92 MiB -> 336.96 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants