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

Unsafe extern blocks #124482

Merged
merged 12 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2501,6 +2501,8 @@ pub enum IsAuto {
pub enum Safety {
/// `unsafe` an item is explicitly marked as `unsafe`.
Unsafe(Span),
/// `safe` an item is explicitly marked as `safe`.
Safe(Span),
/// Default means no value was provided, it will take a default value given the context in
/// which is used.
Default,
Expand Down Expand Up @@ -3162,6 +3164,7 @@ pub struct DelegationMac {
#[derive(Clone, Encodable, Decodable, Debug)]
pub struct StaticItem {
pub ty: P<Ty>,
pub safety: Safety,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unfortunate but making StaticItem and StaticForeignItem carry different types would require a major refactor mainly due to https://github.com/spastorino/rust/blob/f6f8e91adcaf446eb4b38fe960a97d0c1ab88417/compiler/rustc_parse/src/parser/item.rs#L1184 which is an ungreat way of code reusing.
I can refactor this code to disentangle it after this PR lands and then remove this safety field and likely the expr one but I wouldn't block landing this PR on this pre-existing issue.

pub mutability: Mutability,
pub expr: Option<P<Expr>>,
}
Expand All @@ -3171,6 +3174,7 @@ pub struct StaticItem {
#[derive(Clone, Encodable, Decodable, Debug)]
pub struct StaticForeignItem {
pub ty: P<Ty>,
pub safety: Safety,
pub mutability: Mutability,
pub expr: Option<P<Expr>>,
}
Expand All @@ -3179,6 +3183,7 @@ impl From<StaticItem> for StaticForeignItem {
fn from(static_item: StaticItem) -> StaticForeignItem {
StaticForeignItem {
ty: static_item.ty,
safety: static_item.safety,
mutability: static_item.mutability,
expr: static_item.expr,
}
Expand All @@ -3189,6 +3194,7 @@ impl From<StaticForeignItem> for StaticItem {
fn from(static_item: StaticForeignItem) -> StaticItem {
StaticItem {
ty: static_item.ty,
safety: static_item.safety,
mutability: static_item.mutability,
expr: static_item.expr,
}
Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_ast/src/mut_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,7 @@ fn visit_defaultness<T: MutVisitor>(defaultness: &mut Defaultness, vis: &mut T)
fn visit_safety<T: MutVisitor>(safety: &mut Safety, vis: &mut T) {
match safety {
Safety::Unsafe(span) => vis.visit_span(span),
Safety::Safe(span) => vis.visit_span(span),
Safety::Default => {}
}
}
Expand Down Expand Up @@ -1079,7 +1080,7 @@ impl NoopVisitItemKind for ItemKind {
match self {
ItemKind::ExternCrate(_orig_name) => {}
ItemKind::Use(use_tree) => vis.visit_use_tree(use_tree),
ItemKind::Static(box StaticItem { ty, mutability: _, expr }) => {
ItemKind::Static(box StaticItem { ty, safety: _, mutability: _, expr }) => {
vis.visit_ty(ty);
visit_opt(expr, |expr| vis.visit_expr(expr));
}
Expand Down Expand Up @@ -1289,7 +1290,12 @@ pub fn noop_flat_map_item<K: NoopVisitItemKind>(
impl NoopVisitItemKind for ForeignItemKind {
fn noop_visit(&mut self, visitor: &mut impl MutVisitor) {
match self {
ForeignItemKind::Static(box StaticForeignItem { ty, mutability: _, expr }) => {
ForeignItemKind::Static(box StaticForeignItem {
ty,
mutability: _,
expr,
safety: _,
}) => {
visitor.visit_ty(ty);
visit_opt(expr, |expr| visitor.visit_expr(expr));
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_ast/src/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ pub fn ident_can_begin_expr(name: Symbol, span: Span, is_raw: IdentIsRaw) -> boo
kw::Unsafe,
kw::While,
kw::Yield,
kw::Safe,
kw::Static,
]
.contains(&name)
Expand Down Expand Up @@ -577,6 +578,7 @@ impl Token {
kw::Impl,
kw::Unsafe,
kw::Const,
kw::Safe,
kw::Static,
kw::Union,
kw::Macro,
Expand Down
9 changes: 7 additions & 2 deletions compiler/rustc_ast/src/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ impl WalkItemKind for ItemKind {
match self {
ItemKind::ExternCrate(_) => {}
ItemKind::Use(use_tree) => try_visit!(visitor.visit_use_tree(use_tree, item.id, false)),
ItemKind::Static(box StaticItem { ty, mutability: _, expr }) => {
ItemKind::Static(box StaticItem { ty, safety: _, mutability: _, expr }) => {
try_visit!(visitor.visit_ty(ty));
visit_opt!(visitor, visit_expr, expr);
}
Expand Down Expand Up @@ -658,7 +658,12 @@ impl WalkItemKind for ForeignItemKind {
) -> V::Result {
let &Item { id, span, ident, ref vis, .. } = item;
match self {
ForeignItemKind::Static(box StaticForeignItem { ty, mutability: _, expr }) => {
ForeignItemKind::Static(box StaticForeignItem {
ty,
mutability: _,
expr,
safety: _,
}) => {
try_visit!(visitor.visit_ty(ty));
visit_opt!(visitor, visit_expr, expr);
}
Expand Down
27 changes: 18 additions & 9 deletions compiler/rustc_ast_lowering/src/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

self.lower_use_tree(use_tree, &prefix, id, vis_span, ident, attrs)
}
ItemKind::Static(box ast::StaticItem { ty: t, mutability: m, expr: e }) => {
ItemKind::Static(box ast::StaticItem { ty: t, safety: _, mutability: m, expr: e }) => {
let (ty, body_id) =
self.lower_const_item(t, span, e.as_deref(), ImplTraitPosition::StaticTy);
hir::ItemKind::Static(ty, *m, body_id)
Expand Down Expand Up @@ -388,7 +388,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
ImplPolarity::Negative(s) => ImplPolarity::Negative(self.lower_span(*s)),
};
hir::ItemKind::Impl(self.arena.alloc(hir::Impl {
safety: self.lower_safety(*safety),
safety: self.lower_safety(*safety, hir::Safety::Safe),
polarity,
defaultness,
defaultness_span,
Expand Down Expand Up @@ -418,7 +418,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
let items = this.arena.alloc_from_iter(
items.iter().map(|item| this.lower_trait_item_ref(item)),
);
let safety = this.lower_safety(*safety);
let safety = this.lower_safety(*safety, hir::Safety::Safe);
(safety, items, bounds)
},
);
Expand Down Expand Up @@ -660,13 +660,21 @@ impl<'hir> LoweringContext<'_, 'hir> {
this.lower_fn_params_to_names(fdec),
)
});
let safety = self.lower_safety(sig.header.safety, hir::Safety::Unsafe);

hir::ForeignItemKind::Fn(fn_dec, fn_args, generics)
hir::ForeignItemKind::Fn(fn_dec, fn_args, generics, safety)
}
ForeignItemKind::Static(box StaticForeignItem { ty, mutability, expr: _ }) => {
ForeignItemKind::Static(box StaticForeignItem {
ty,
mutability,
expr: _,
safety,
}) => {
let ty = self
.lower_ty(ty, ImplTraitContext::Disallowed(ImplTraitPosition::StaticTy));
hir::ForeignItemKind::Static(ty, *mutability)
let safety = self.lower_safety(*safety, hir::Safety::Unsafe);

hir::ForeignItemKind::Static(ty, *mutability, safety)
}
ForeignItemKind::TyAlias(..) => hir::ForeignItemKind::Type,
ForeignItemKind::MacCall(_) => panic!("macro shouldn't exist here"),
Expand Down Expand Up @@ -1360,7 +1368,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
hir::IsAsync::NotAsync
};
hir::FnHeader {
safety: self.lower_safety(h.safety),
safety: self.lower_safety(h.safety, hir::Safety::Safe),
asyncness: asyncness,
constness: self.lower_constness(h.constness),
abi: self.lower_extern(h.ext),
Expand Down Expand Up @@ -1410,10 +1418,11 @@ impl<'hir> LoweringContext<'_, 'hir> {
}
}

pub(super) fn lower_safety(&mut self, s: Safety) -> hir::Safety {
pub(super) fn lower_safety(&mut self, s: Safety, default: hir::Safety) -> hir::Safety {
match s {
Safety::Unsafe(_) => hir::Safety::Unsafe,
Safety::Default => hir::Safety::Safe,
Safety::Default => default,
Safety::Safe(_) => hir::Safety::Safe,
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast_lowering/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1321,7 +1321,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
let generic_params = self.lower_lifetime_binder(t.id, &f.generic_params);
hir::TyKind::BareFn(self.arena.alloc(hir::BareFnTy {
generic_params,
safety: self.lower_safety(f.safety),
safety: self.lower_safety(f.safety, hir::Safety::Safe),
abi: self.lower_extern(f.ext),
decl: self.lower_fn_decl(&f.decl, t.id, t.span, FnDeclKind::Pointer, None),
param_names: self.lower_fn_params_to_names(&f.decl),
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_ast_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ ast_passes_extern_fn_qualifiers = functions in `extern` blocks cannot have quali
.label = in this `extern` block
.suggestion = remove this qualifier

ast_passes_extern_invalid_safety = items in unadorned `extern` blocks cannot have safety qualifiers
.suggestion = add unsafe to this `extern` block

ast_passes_extern_item_ascii = items in `extern` blocks cannot use non-ascii identifiers
.label = in this `extern` block
.note = this limitation may be lifted in the future; see issue #83942 <https://github.com/rust-lang/rust/issues/83942> for more information
Expand Down Expand Up @@ -174,6 +177,8 @@ ast_passes_match_arm_with_no_body =
`match` arm with no body
.suggestion = add a body after the pattern

ast_passes_missing_unsafe_on_extern = extern blocks must be unsafe

ast_passes_module_nonascii = trying to load file for module `{$name}` with non-ascii identifier name
.help = consider using the `#[path]` attribute to specify filesystem path

Expand Down
81 changes: 61 additions & 20 deletions compiler/rustc_ast_passes/src/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use rustc_data_structures::fx::FxIndexMap;
use rustc_feature::Features;
use rustc_parse::validate_attr;
use rustc_session::lint::builtin::{
DEPRECATED_WHERE_CLAUSE_LOCATION, MISSING_ABI, PATTERNS_IN_FNS_WITHOUT_BODY,
DEPRECATED_WHERE_CLAUSE_LOCATION, MISSING_ABI, MISSING_UNSAFE_ON_EXTERN,
PATTERNS_IN_FNS_WITHOUT_BODY,
};
use rustc_session::lint::{BuiltinLintDiag, LintBuffer};
use rustc_session::Session;
Expand Down Expand Up @@ -86,6 +87,9 @@ struct AstValidator<'a> {
/// or `Foo::Bar<impl Trait>`
is_impl_trait_banned: bool,

/// Used to ban explicit safety on foreign items when the extern block is not marked as unsafe.
extern_mod_safety: Option<Safety>,

lint_buffer: &'a mut LintBuffer,
}

Expand Down Expand Up @@ -116,6 +120,12 @@ impl<'a> AstValidator<'a> {
self.outer_trait_or_trait_impl = old;
}

fn with_in_extern_mod(&mut self, extern_mod_safety: Safety, f: impl FnOnce(&mut Self)) {
let old = mem::replace(&mut self.extern_mod_safety, Some(extern_mod_safety));
f(self);
self.extern_mod_safety = old;
}

fn with_banned_impl_trait(&mut self, f: impl FnOnce(&mut Self)) {
let old = mem::replace(&mut self.is_impl_trait_banned, true);
f(self);
Expand Down Expand Up @@ -429,6 +439,18 @@ impl<'a> AstValidator<'a> {
}
}

fn check_foreign_item_safety(&self, item_span: Span, safety: Safety) {
if matches!(safety, Safety::Unsafe(_) | Safety::Safe(_))
&& (self.extern_mod_safety == Some(Safety::Default)
|| !self.features.unsafe_extern_blocks)
{
self.dcx().emit_err(errors::InvalidSafetyOnExtern {
item_span,
block: self.current_extern_span(),
});
}
}

fn check_defaultness(&self, span: Span, defaultness: Defaultness) {
if let Defaultness::Default(def_span) = defaultness {
let span = self.session.source_map().guess_head_span(span);
Expand Down Expand Up @@ -518,18 +540,14 @@ impl<'a> AstValidator<'a> {
fn check_foreign_fn_headerless(
&self,
// Deconstruct to ensure exhaustiveness
FnHeader { safety, coroutine_kind, constness, ext }: FnHeader,
FnHeader { safety: _, coroutine_kind, constness, ext }: FnHeader,
) {
let report_err = |span| {
self.dcx().emit_err(errors::FnQualifierInExtern {
span: span,
block: self.current_extern_span(),
});
};
match safety {
Safety::Unsafe(span) => report_err(span),
Safety::Default => (),
}
match coroutine_kind {
Some(knd) => report_err(knd.span()),
None => (),
Expand Down Expand Up @@ -1017,19 +1035,39 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
return; // Avoid visiting again.
}
ItemKind::ForeignMod(ForeignMod { abi, safety, .. }) => {
let old_item = mem::replace(&mut self.extern_mod, Some(item));
self.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualForeignItems,
);
if let &Safety::Unsafe(span) = safety {
self.dcx().emit_err(errors::UnsafeItem { span, kind: "extern block" });
}
if abi.is_none() {
self.maybe_lint_missing_abi(item.span, item.id);
}
visit::walk_item(self, item);
self.extern_mod = old_item;
self.with_in_extern_mod(*safety, |this| {
let old_item = mem::replace(&mut this.extern_mod, Some(item));
this.visibility_not_permitted(
&item.vis,
errors::VisibilityNotPermittedNote::IndividualForeignItems,
);

if this.features.unsafe_extern_blocks {
if &Safety::Default == safety {
if item.span.at_least_rust_2024() {
this.dcx()
.emit_err(errors::MissingUnsafeOnExtern { span: item.span });
} else {
this.lint_buffer.buffer_lint(
MISSING_UNSAFE_ON_EXTERN,
item.id,
item.span,
BuiltinLintDiag::MissingUnsafeOnExtern {
suggestion: item.span.shrink_to_lo(),
},
);
}
}
} else if let &Safety::Unsafe(span) = safety {
this.dcx().emit_err(errors::UnsafeItem { span, kind: "extern block" });
}

if abi.is_none() {
this.maybe_lint_missing_abi(item.span, item.id);
}
visit::walk_item(this, item);
this.extern_mod = old_item;
});
return; // Avoid visiting again.
}
ItemKind::Enum(def, _) => {
Expand Down Expand Up @@ -1161,6 +1199,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
fn visit_foreign_item(&mut self, fi: &'a ForeignItem) {
match &fi.kind {
ForeignItemKind::Fn(box Fn { defaultness, sig, body, .. }) => {
self.check_foreign_item_safety(fi.span, sig.header.safety);
self.check_defaultness(fi.span, *defaultness);
self.check_foreign_fn_bodyless(fi.ident, body.as_deref());
self.check_foreign_fn_headerless(sig.header);
Expand All @@ -1180,7 +1219,8 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
self.check_foreign_ty_genericless(generics, where_clauses);
self.check_foreign_item_ascii_only(fi.ident);
}
ForeignItemKind::Static(box StaticForeignItem { ty: _, mutability: _, expr }) => {
ForeignItemKind::Static(box StaticForeignItem { expr, safety, .. }) => {
self.check_foreign_item_safety(fi.span, *safety);
self.check_foreign_kind_bodyless(fi.ident, "static", expr.as_ref().map(|b| b.span));
self.check_foreign_item_ascii_only(fi.ident);
}
Expand Down Expand Up @@ -1736,6 +1776,7 @@ pub fn check_crate(
outer_impl_trait: None,
disallow_tilde_const: Some(DisallowTildeConstContext::Item),
is_impl_trait_banned: false,
extern_mod_safety: None,
lint_buffer: lints,
};
visit::walk_crate(&mut validator, krate);
Expand Down
16 changes: 16 additions & 0 deletions compiler/rustc_ast_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,15 @@ pub enum ExternBlockSuggestion {
},
}

#[derive(Diagnostic)]
#[diag(ast_passes_extern_invalid_safety)]
pub struct InvalidSafetyOnExtern {
#[primary_span]
pub item_span: Span,
#[suggestion(code = "", applicability = "maybe-incorrect")]
pub block: Span,
}

#[derive(Diagnostic)]
#[diag(ast_passes_bound_in_context)]
pub struct BoundInContext<'a> {
Expand Down Expand Up @@ -485,6 +494,13 @@ pub struct UnsafeItem {
pub kind: &'static str,
}

#[derive(Diagnostic)]
#[diag(ast_passes_missing_unsafe_on_extern)]
pub struct MissingUnsafeOnExtern {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(ast_passes_fieldless_union)]
pub struct FieldlessUnion {
Expand Down
Loading
Loading