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

StaticForeignItem and StaticItem are the same #126767

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

compiler-errors
Copy link
Member

The struct StaticItem and StaticForeignItem are the same, so remove StaticForeignItem. Having them be separate is unique to static items -- unlike ForeignItemKind::{Fn,TyAlias}, which use the normal AST item.

r? @spastorino or @oli-obk

@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 Jun 20, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@spastorino
Copy link
Member

Yes, so I can explain a bit what's going on here. It's true these types are actually the same right now and we can totally proceed with an r+ but they shouldn't be the same and we would need to reintroduce this type later if we remove it now.

StaticForeignItem need to have safety and not expr, probably just a span.
StaticItem shouldn't have safety.

This issue was a preexisting issue before unsafe extern blocks but unsafe extern blocks make it a bit worser by needing to add yet another attribute that make these 2 types a little bit more different. But the problem is that in parsing code we convert from one type to the other just for the sake of code reuse as we parse items and foreign items with the same code and using the same type, this is why these conversions exist. We should be reusing code in a different way. I have this in my todo list and also have a branch with some changes made to disantangle a lot of that code and to make these two types different as they should be.

In any case ... feel free to do whatever you want this PR. I meant, take this as a @bors r=me if you still prefer to merge it :) or we can just close it and we can provide a proper fix to the describe issue for all this mess later.

@bors
Copy link
Contributor

bors commented Jun 21, 2024

📌 Commit 3e59f0c has been approved by `me``

It is now in the queue for this repository.

@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 Jun 21, 2024
@spastorino
Copy link
Member

Ouch, @bors r-

@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 Jun 21, 2024
@compiler-errors
Copy link
Member Author

Well so here's the problem. After #124482, we now allow this code to parse:

macro_rules! s {
    ($item:item) => {}
}

s!(unsafe static FOO: usize = 1;);

I'm kind of mixed on whether that's a good thing or not, (and it should have probably been feature gated rather than silently going from fail -> pass) but we will probably need to support it when unsafe_extern_blocks is stabilized.

In that case, we need to be able to maximally parse statics with an optional safety and expr. So I don't see an obvious way to split out these two kinds of statics, at least not at the AST level, since if we remove the safety from StaticItem, then we have no way to turn a StaticItem to a ForeignStaticItem in parse_foreign_item.

@compiler-errors
Copy link
Member Author

compiler-errors commented Jun 21, 2024

so tl;dr we need to maximally be able to parse StaticItems that are in interpolated positions behind :item macro fragment matchers so they can be expanded in extern { $item } position, which requires StaticItem to at least have a Safety, and I really don't see a benefit of making ForeignStaticItem not have an expr -- we still need to be able for this code parse, for example:

#[cfg(any())]
extern "C" {
    static FOO: usize = 1;
}

@compiler-errors
Copy link
Member Author

So yeah, I don't think there's actually any splitting that's possible to be done here without regressing macro code?

@bors r=spastorino

@bors
Copy link
Contributor

bors commented Jun 21, 2024

📌 Commit 3e59f0c has been approved by spastorino

It is now in the queue for this repository.

@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 Jun 21, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 21, 2024
…m, r=spastorino

`StaticForeignItem` and `StaticItem` are the same

The struct `StaticItem` and `StaticForeignItem` are the same, so remove `StaticForeignItem`. Having them be separate is unique to `static` items -- unlike `ForeignItemKind::{Fn,TyAlias}`, which use the normal AST item.

r? `@spastorino` or `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 21, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#124101 (Add PidFd::{kill, wait, try_wait})
 - rust-lang#126125 (Improve conflict marker recovery)
 - rust-lang#126481 (Add `powerpc-unknown-openbsd` maintenance status)
 - rust-lang#126613 (Print the tested value in int_log tests)
 - rust-lang#126617 (Expand `avx512_target_feature` to include VEX variants)
 - rust-lang#126700 (Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does)
 - rust-lang#126707 (Pass target to inaccessible-temp-dir rmake test)
 - rust-lang#126767 (`StaticForeignItem` and `StaticItem` are the same)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 21, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#126125 (Improve conflict marker recovery)
 - rust-lang#126481 (Add `powerpc-unknown-openbsd` maintenance status)
 - rust-lang#126613 (Print the tested value in int_log tests)
 - rust-lang#126617 (Expand `avx512_target_feature` to include VEX variants)
 - rust-lang#126700 (Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does)
 - rust-lang#126707 (Pass target to inaccessible-temp-dir rmake test)
 - rust-lang#126767 (`StaticForeignItem` and `StaticItem` are the same)
 - rust-lang#126774 (Fix another assertion failure for some Expect diagnostics.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f577d80 into rust-lang:master Jun 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 21, 2024
Rollup merge of rust-lang#126767 - compiler-errors:static-foreign-item, r=spastorino

`StaticForeignItem` and `StaticItem` are the same

The struct `StaticItem` and `StaticForeignItem` are the same, so remove `StaticForeignItem`. Having them be separate is unique to `static` items -- unlike `ForeignItemKind::{Fn,TyAlias}`, which use the normal AST item.

r? ``@spastorino`` or ``@oli-obk``
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. 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.

4 participants