From 230393993ffc255d3f20d98400c8a376bd51d1c0 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Thu, 9 Jul 2020 19:03:15 -0700 Subject: [PATCH 1/5] Don't visit foreign function bodies when lowering ast to hir Previously the existence of bodies inside a foreign function block would cause a panic in the hir `NodeCollector` during its collection of crate bodies to compute a crate hash: https://github.com/rust-lang/rust/blob/e59b08e62ea691916d2f063cac5aab4634128022/src/librustc_middle/hir/map/collector.rs#L154-L158 The collector walks the hir tree and creates a map of hir nodes, then attaching bodies in the crate to their owner in the map. For a code like ```rust extern "C" { fn f() { fn g() {} } } ``` The crate bodies include the body of the function `g`. But foreign functions cannot have bodies, and while the parser AST permits a foreign function to have a body, the hir doesn't. This means that the body of `f` is not present in the hir, and so neither is `g`. So when the `NodeCollector` finishes the walking the hir, it has no record of `g`, cannot find an owner for the body of `g` it sees in the crate bodies, and blows up. Why do the crate bodies include the body of `g`? The AST walker has a need a for walking function bodies, and FFIs share the same AST node as functions in other contexts. There are at least two options to fix this: - Don't unwrap the map entry for an hir node in the `NodeCollector` - Modifier the ast->hir lowering visitor to ignore foreign function blocks I don't think the first is preferrable, since we want to know when we can't find a body for an hir node that we thought had one (dropping this information may lead to an invalid hash). So this commit implements the second option. Closes #74120 --- src/librustc_ast_lowering/item.rs | 40 ++++++++++++++++++- ...ssue-74120-lowering-of-ffi-block-bodies.rs | 11 +++++ ...-74120-lowering-of-ffi-block-bodies.stderr | 19 +++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.rs create mode 100644 src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.stderr diff --git a/src/librustc_ast_lowering/item.rs b/src/librustc_ast_lowering/item.rs index 00665c4cafb6b..b2db9fe1d2666 100644 --- a/src/librustc_ast_lowering/item.rs +++ b/src/librustc_ast_lowering/item.rs @@ -6,7 +6,8 @@ use rustc_ast::ast::*; use rustc_ast::attr; use rustc_ast::node_id::NodeMap; use rustc_ast::ptr::P; -use rustc_ast::visit::{self, AssocCtxt, Visitor}; +use rustc_ast::visit::{self, AssocCtxt, FnCtxt, FnKind, Visitor}; +use rustc_ast::walk_list; use rustc_data_structures::fx::FxHashSet; use rustc_errors::struct_span_err; use rustc_hir as hir; @@ -76,6 +77,43 @@ impl<'a> Visitor<'a> for ItemLowerer<'a, '_, '_> { } } + // Forked from the original method because we don't want to descend into foreign function + // blocks. Such blocks are semantically invalid and the hir does not preserve them, so lowering + // items contained in them may be unexpected by later passes. + fn visit_foreign_item(&mut self, item: &'a ForeignItem) { + let Item { id: _, span: _, ident, ref vis, ref attrs, ref kind, tokens: _ } = *item; + self.visit_vis(vis); + self.visit_ident(ident); + walk_list!(self, visit_attribute, attrs); + match kind { + ForeignItemKind::Static(ty, _, expr) => { + self.visit_ty(ty); + walk_list!(self, visit_expr, expr); + } + ForeignItemKind::Fn(_, sig, generics, body) => { + self.visit_generics(generics); + let kind = FnKind::Fn(FnCtxt::Foreign, ident, sig, vis, body.as_deref()); + match kind { + FnKind::Fn(_, _, sig, _, _) => { + self.visit_fn_header(&sig.header); + visit::walk_fn_decl(self, &sig.decl); + } + FnKind::Closure(decl, _) => { + visit::walk_fn_decl(self, decl); + } + } + } + ForeignItemKind::TyAlias(_, generics, bounds, ty) => { + self.visit_generics(generics); + walk_list!(self, visit_param_bound, bounds); + walk_list!(self, visit_ty, ty); + } + ForeignItemKind::MacCall(mac) => { + self.visit_mac(mac); + } + } + } + fn visit_assoc_item(&mut self, item: &'a AssocItem, ctxt: AssocCtxt) { self.lctx.with_hir_id_owner(item.id, |lctx| match ctxt { AssocCtxt::Trait => { diff --git a/src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.rs b/src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.rs new file mode 100644 index 0000000000000..a84065e021868 --- /dev/null +++ b/src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.rs @@ -0,0 +1,11 @@ +// Previously this ICE'd because `fn g()` would be lowered, but the block associated with `fn f()` +// wasn't. + +// compile-flags: --crate-type=lib + +extern "C" { + fn f() { + //~^ incorrect function inside `extern` block + fn g() {} + } +} diff --git a/src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.stderr b/src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.stderr new file mode 100644 index 0000000000000..d4a9ca3e7c66e --- /dev/null +++ b/src/test/ui/foreign/issue-74120-lowering-of-ffi-block-bodies.stderr @@ -0,0 +1,19 @@ +error: incorrect function inside `extern` block + --> $DIR/issue-74120-lowering-of-ffi-block-bodies.rs:7:8 + | +LL | extern "C" { + | ---------- `extern` blocks define existing foreign functions and functions inside of them cannot have a body +LL | fn f() { + | ________^___- + | | | + | | cannot have a body +LL | | +LL | | fn g() {} +LL | | } + | |_____- help: remove the invalid body: `;` + | + = help: you might have meant to write a function accessible through FFI, which can be done by writing `extern fn` outside of the `extern` block + = note: for more information, visit https://doc.rust-lang.org/std/keyword.extern.html + +error: aborting due to previous error + From ab4275cddc3749caf9f20373eb812e6f09bd3309 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Thu, 9 Jul 2020 22:32:49 -0700 Subject: [PATCH 2/5] fixup! Don't visit foreign function bodies when lowering ast to hir --- src/librustc_ast_lowering/item.rs | 43 +++++++++---------------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/src/librustc_ast_lowering/item.rs b/src/librustc_ast_lowering/item.rs index b2db9fe1d2666..21f1137c46e23 100644 --- a/src/librustc_ast_lowering/item.rs +++ b/src/librustc_ast_lowering/item.rs @@ -77,39 +77,20 @@ impl<'a> Visitor<'a> for ItemLowerer<'a, '_, '_> { } } - // Forked from the original method because we don't want to descend into foreign function - // blocks. Such blocks are semantically invalid and the hir does not preserve them, so lowering - // items contained in them may be unexpected by later passes. - fn visit_foreign_item(&mut self, item: &'a ForeignItem) { - let Item { id: _, span: _, ident, ref vis, ref attrs, ref kind, tokens: _ } = *item; - self.visit_vis(vis); - self.visit_ident(ident); - walk_list!(self, visit_attribute, attrs); - match kind { - ForeignItemKind::Static(ty, _, expr) => { - self.visit_ty(ty); - walk_list!(self, visit_expr, expr); + fn visit_fn(&mut self, fk: FnKind<'a>, _: Span, _: NodeId) { + match fk { + FnKind::Fn(FnCtxt::Foreign, _, sig, _, _) => { + self.visit_fn_header(&sig.header); + visit::walk_fn_decl(self, &sig.decl); } - ForeignItemKind::Fn(_, sig, generics, body) => { - self.visit_generics(generics); - let kind = FnKind::Fn(FnCtxt::Foreign, ident, sig, vis, body.as_deref()); - match kind { - FnKind::Fn(_, _, sig, _, _) => { - self.visit_fn_header(&sig.header); - visit::walk_fn_decl(self, &sig.decl); - } - FnKind::Closure(decl, _) => { - visit::walk_fn_decl(self, decl); - } - } - } - ForeignItemKind::TyAlias(_, generics, bounds, ty) => { - self.visit_generics(generics); - walk_list!(self, visit_param_bound, bounds); - walk_list!(self, visit_ty, ty); + FnKind::Fn(_, _, sig, _, body) => { + self.visit_fn_header(&sig.header); + visit::walk_fn_decl(self, &sig.decl); + walk_list!(self, visit_block, body); } - ForeignItemKind::MacCall(mac) => { - self.visit_mac(mac); + FnKind::Closure(decl, body) => { + visit::walk_fn_decl(self, decl); + self.visit_expr(body); } } } From 68aca3baf6566cea3ad2f83a07726c9ee017b9fc Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 14 Jul 2020 18:13:51 -0700 Subject: [PATCH 3/5] fixup! fixup! Don't visit foreign function bodies when lowering ast to hir --- src/librustc_ast_lowering/item.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/librustc_ast_lowering/item.rs b/src/librustc_ast_lowering/item.rs index 21f1137c46e23..306b599e64b45 100644 --- a/src/librustc_ast_lowering/item.rs +++ b/src/librustc_ast_lowering/item.rs @@ -77,21 +77,13 @@ impl<'a> Visitor<'a> for ItemLowerer<'a, '_, '_> { } } - fn visit_fn(&mut self, fk: FnKind<'a>, _: Span, _: NodeId) { + fn visit_fn(&mut self, fk: FnKind<'a>, sp: Span, _: NodeId) { match fk { FnKind::Fn(FnCtxt::Foreign, _, sig, _, _) => { self.visit_fn_header(&sig.header); visit::walk_fn_decl(self, &sig.decl); } - FnKind::Fn(_, _, sig, _, body) => { - self.visit_fn_header(&sig.header); - visit::walk_fn_decl(self, &sig.decl); - walk_list!(self, visit_block, body); - } - FnKind::Closure(decl, body) => { - visit::walk_fn_decl(self, decl); - self.visit_expr(body); - } + _ => visit::walk_fn(self, fk, sp), } } From 0c64d32a4a439f373f388f7925048f6f349bd5b2 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 14 Jul 2020 18:21:53 -0700 Subject: [PATCH 4/5] fixup! Don't visit foreign function bodies when lowering ast to hir --- src/librustc_ast_lowering/item.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/librustc_ast_lowering/item.rs b/src/librustc_ast_lowering/item.rs index 306b599e64b45..29021749365ad 100644 --- a/src/librustc_ast_lowering/item.rs +++ b/src/librustc_ast_lowering/item.rs @@ -7,7 +7,6 @@ use rustc_ast::attr; use rustc_ast::node_id::NodeMap; use rustc_ast::ptr::P; use rustc_ast::visit::{self, AssocCtxt, FnCtxt, FnKind, Visitor}; -use rustc_ast::walk_list; use rustc_data_structures::fx::FxHashSet; use rustc_errors::struct_span_err; use rustc_hir as hir; From d442bf7162647743f941977a5154676322a5614b Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Wed, 15 Jul 2020 17:22:41 -0700 Subject: [PATCH 5/5] fixup! Don't visit foreign function bodies when lowering ast to hir --- src/librustc_ast_lowering/item.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/librustc_ast_lowering/item.rs b/src/librustc_ast_lowering/item.rs index 29021749365ad..ca3cf11b1a3fe 100644 --- a/src/librustc_ast_lowering/item.rs +++ b/src/librustc_ast_lowering/item.rs @@ -81,6 +81,8 @@ impl<'a> Visitor<'a> for ItemLowerer<'a, '_, '_> { FnKind::Fn(FnCtxt::Foreign, _, sig, _, _) => { self.visit_fn_header(&sig.header); visit::walk_fn_decl(self, &sig.decl); + // Don't visit the foreign function body even if it has one, since lowering the + // body would have no meaning and will have already been caught as a parse error. } _ => visit::walk_fn(self, fk, sp), }