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

feat: initial support for safe_kw in extern blocks #18350

Merged
merged 2 commits into from
Oct 20, 2024

Conversation

roife
Copy link
Member

@roife roife commented Oct 20, 2024

This PR adds initial support for safe keywords in external blocks.

Changes

  1. Parsing static declarations with safe kw and unsafe kw, as well as functions with safe kw in extern_blocks
  2. Add HAS_SAFE_KW to FnFlags
  3. Handle safe kw in is_fn_unsafe_to_call query
  4. Handle safe_kw in unsafe diagnostics

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2024
@roife
Copy link
Member Author

roife commented Oct 20, 2024

This PR provides basic support to prevent r-a emitting syntax errors, and ensures that hovering does not display incorrect signatures. I create this PR first so that it can be merged before Monday.

Additional support such as completions and assists will be added in next few PRs.

@ChayimFriedman2
Copy link
Contributor

Please add a test that unsafe diagnostics is not triggered for safe fn.

@roife
Copy link
Member Author

roife commented Oct 20, 2024

Tests for unsafe diagnostics have been added; also, I realized I forgot to add checks for safe statics, which have also been implemented in the new commit.

@lnicola
Copy link
Member

lnicola commented Oct 20, 2024

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2024

📌 Commit 002f6ad has been approved by lnicola

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 20, 2024

⌛ Testing commit 002f6ad with merge 6dc5b32...

@bors
Copy link
Contributor

bors commented Oct 20, 2024

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing 6dc5b32 to master...

@bors bors merged commit 6dc5b32 into rust-lang:master Oct 20, 2024
11 checks passed
crates/hir-ty/src/utils.rs Show resolved Hide resolved
@@ -567,6 +571,8 @@ pub struct StaticData {
pub visibility: RawVisibility,
pub mutable: bool,
pub is_extern: bool,
pub has_safe_kw: bool,
pub has_unsafe_kw: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

4 bools is becoming noticeable. Can you check whether that increases the size (hover tells the size)? If then it's probably better to turn it into a bitflags (not necessary to do now though).

Also, why is has_unsafe_kw needed, given it defaults to unsafe?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why is has_unsafe_kw needed, given it defaults to unsafe?

It seems to be used for pretty-printing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we promise pretty-printing will recover exactly the original code, at least if we do there are bunch of cases this promise is broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I'd argue it's better to always write unsafe in pretty printing since it may not be clear that the function is unsafe because it is extern (if we even show that it is extern, I don't remember 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 kept both has_xxx_kw here in case we need them later, and it is similar to FnFlags. I also left a comment here: https://github.com/roife/rust-analyzer/blob/002f6ad6f1ca6aa59e1ebccc8478930101f8150f/crates/hir-def/src/item_tree.rs#L826-L829 that we may use bitflags in the future.

Copy link
Member

Choose a reason for hiding this comment

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

This one shouldn't have changed the size, but the next one (5th bool) might given alignment of 8 and other 4 byte fields in this struct.

I do think we should just swap all of these kinds of flags (this struct and others) to use bitflags, its more uniform that way and generally packs nicer.

Copy link
Member

Choose a reason for hiding this comment

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

I do agree we don't really need to preserve the explicit unsafe in our IRs here though, as in this case its not meaningful. But it doesn't really matter much either

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

6 participants