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

Lowering cleanups [1/N] #51806

Merged
merged 18 commits into from
Jun 30, 2018
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: 4 additions & 2 deletions src/librustc/hir/intravisit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,11 @@ pub fn walk_ty<'v, V: Visitor<'v>>(visitor: &mut V, typ: &'v Ty) {
}
visitor.visit_lifetime(lifetime);
}
TyImplTraitExistential(item_id, def_id, ref lifetimes) => {
TyImplTraitExistential(_, def_id, ref lifetimes) => {
// we are not recursing into the `existential` item, because it is already being visited
// as part of the surrounding module. The `NodeId` just exists so we don't have to look
// it up everywhere else in the compiler
visitor.visit_def_mention(Def::Existential(def_id));
visitor.visit_nested_item(item_id);
Copy link
Member

Choose a reason for hiding this comment

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

Hang on, if the NodeId is pointing to the same thing as the DefId, why do we keep both?!
We only really need the DefId for most of the compilation, AFAIK, and the occasional need to get the NodeId from the DefId shouldn't be enough to keep the NodeId in the HIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have further refactorings planned, this entire variant is getting removed ASAP, so I didn't feel very motivated to make it idiomatic

Copy link
Member

Choose a reason for hiding this comment

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

Oh, huh, are you going with a fake path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, not very fake though. The items are already real and in the parent of the function.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mean just not part of the original AST. Seems good!

walk_list!(visitor, visit_lifetime, lifetimes);
}
TyTypeof(ref expression) => {
Expand Down
160 changes: 103 additions & 57 deletions src/librustc/hir/lowering.rs

Large diffs are not rendered by default.

10 changes: 7 additions & 3 deletions src/librustc/hir/map/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,23 +221,27 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
// Make sure that the DepNode of some node coincides with the HirId
// owner of that node.
if cfg!(debug_assertions) {
let hir_id_owner = self.definitions.node_to_hir_id(id).owner;
let hir_id = self.definitions.node_to_hir_id(id);

if hir_id_owner != self.current_dep_node_owner {
if hir_id.owner != self.current_dep_node_owner {
let node_str = match self.definitions.opt_def_index(id) {
Some(def_index) => {
self.definitions.def_path(def_index).to_string_no_crate()
}
None => format!("{:?}", node)
};

if hir_id == ::hir::DUMMY_HIR_ID {
debug!("Maybe you forgot to lower the node id {:?}?", id);
}

bug!("inconsistent DepNode for `{}`: \
current_dep_node_owner={}, hir_id.owner={}",
node_str,
self.definitions
.def_path(self.current_dep_node_owner)
.to_string_no_crate(),
self.definitions.def_path(hir_id_owner).to_string_no_crate())
self.definitions.def_path(hir_id.owner).to_string_no_crate())
}
}

Expand Down
25 changes: 19 additions & 6 deletions src/librustc/hir/map/def_collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ impl<'a> DefCollector<'a> {
&mut self,
Copy link
Member

Choose a reason for hiding this comment

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

Nice-- I'm glad to see these moved to def_collector.

id: NodeId,
async_node_id: NodeId,
return_impl_trait_id: NodeId,
name: Name,
span: Span,
visit_fn: impl FnOnce(&mut DefCollector<'a>)
Expand All @@ -86,6 +87,7 @@ impl<'a> DefCollector<'a> {
let fn_def_data = DefPathData::ValueNs(name.as_interned_str());
let fn_def = self.create_def(id, fn_def_data, ITEM_LIKE_SPACE, span);
return self.with_parent(fn_def, |this| {
this.create_def(return_impl_trait_id, DefPathData::ImplTrait, REGULAR_SPACE, span);
let closure_def = this.create_def(async_node_id,
DefPathData::ClosureExpr,
REGULAR_SPACE,
Expand Down Expand Up @@ -120,10 +122,14 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
ItemKind::Mod(..) if i.ident == keywords::Invalid.ident() => {
return visit::walk_item(self, i);
}
ItemKind::Fn(_, FnHeader { asyncness: IsAsync::Async(async_node_id), .. }, ..) => {
ItemKind::Fn(_, FnHeader { asyncness: IsAsync::Async {
closure_id,
return_impl_trait_id,
}, .. }, ..) => {
return self.visit_async_fn(
i.id,
async_node_id,
closure_id,
return_impl_trait_id,
i.ident.name,
i.span,
|this| visit::walk_item(this, i)
Expand Down Expand Up @@ -228,11 +234,15 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
fn visit_impl_item(&mut self, ii: &'a ImplItem) {
let def_data = match ii.node {
ImplItemKind::Method(MethodSig {
header: FnHeader { asyncness: IsAsync::Async(async_node_id), .. }, ..
header: FnHeader { asyncness: IsAsync::Async {
closure_id,
return_impl_trait_id,
}, .. }, ..
}, ..) => {
return self.visit_async_fn(
ii.id,
async_node_id,
closure_id,
return_impl_trait_id,
ii.ident.name,
ii.span,
|this| visit::walk_impl_item(this, ii)
Expand Down Expand Up @@ -277,8 +287,8 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {

// Async closures desugar to closures inside of closures, so
// we must create two defs.
if let IsAsync::Async(async_id) = asyncness {
let async_def = self.create_def(async_id,
if let IsAsync::Async { closure_id, .. } = asyncness {
let async_def = self.create_def(closure_id,
DefPathData::ClosureExpr,
REGULAR_SPACE,
expr.span);
Expand All @@ -302,6 +312,9 @@ impl<'a> visit::Visitor<'a> for DefCollector<'a> {
fn visit_ty(&mut self, ty: &'a Ty) {
match ty.node {
TyKind::Mac(..) => return self.visit_macro_invoc(ty.id),
TyKind::ImplTrait(node_id, _) => {
self.create_def(node_id, DefPathData::ImplTrait, REGULAR_SPACE, ty.span);
}
_ => {}
}
visit::walk_ty(self, ty);
Expand Down
12 changes: 4 additions & 8 deletions src/librustc/hir/map/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,10 +374,8 @@ pub enum DefPathData {
StructCtor,
/// A constant expression (see {ast,hir}::AnonConst).
AnonConst,
/// An `impl Trait` type node in argument position.
UniversalImplTrait,
/// An `impl Trait` type node in return position.
ExistentialImplTrait,
/// An `impl Trait` type node
ImplTrait,

/// GlobalMetaData identifies a piece of crate metadata that is global to
/// a whole crate (as opposed to just one item). GlobalMetaData components
Expand Down Expand Up @@ -641,8 +639,7 @@ impl DefPathData {
ClosureExpr |
StructCtor |
AnonConst |
ExistentialImplTrait |
UniversalImplTrait => None
ImplTrait => None
}
}

Expand Down Expand Up @@ -672,8 +669,7 @@ impl DefPathData {
ClosureExpr => "{{closure}}",
StructCtor => "{{constructor}}",
AnonConst => "{{constant}}",
ExistentialImplTrait => "{{exist-impl-Trait}}",
UniversalImplTrait => "{{univ-impl-Trait}}",
ImplTrait => "{{impl-Trait}}",
};

Symbol::intern(s).as_interned_str()
Expand Down
3 changes: 1 addition & 2 deletions src/librustc/ty/item_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
data @ DefPathData::AnonConst |
data @ DefPathData::MacroDef(..) |
data @ DefPathData::ClosureExpr |
data @ DefPathData::ExistentialImplTrait |
data @ DefPathData::UniversalImplTrait |
data @ DefPathData::ImplTrait |
data @ DefPathData::GlobalMetaData(..) => {
let parent_def_id = self.parent_def_id(def_id).unwrap();
self.push_item_path(buffer, parent_def_id);
Expand Down
3 changes: 1 addition & 2 deletions src/librustc/util/ppaux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,7 @@ impl PrintContext {
DefPathData::Field(_) |
DefPathData::StructCtor |
DefPathData::AnonConst |
DefPathData::ExistentialImplTrait |
DefPathData::UniversalImplTrait |
DefPathData::ImplTrait |
DefPathData::GlobalMetaData(_) => {
// if we're making a symbol for something, there ought
// to be a value or type-def or something in there
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_driver/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ impl<'a> ReplaceBodyWithLoop<'a> {
if let ast::FunctionRetTy::Ty(ref ty) = ret_ty.output {
fn involves_impl_trait(ty: &ast::Ty) -> bool {
match ty.node {
ast::TyKind::ImplTrait(_) => true,
ast::TyKind::ImplTrait(..) => true,
ast::TyKind::Slice(ref subty) |
ast::TyKind::Array(ref subty, _) |
ast::TyKind::Ptr(ast::MutTy { ty: ref subty, .. }) |
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_passes/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}
self.no_questions_in_bounds(bounds, "trait object types", false);
}
TyKind::ImplTrait(ref bounds) => {
TyKind::ImplTrait(_, ref bounds) => {
if !bounds.iter()
.any(|b| if let GenericBound::Trait(..) = *b { true } else { false }) {
self.err_handler().span_err(ty.span, "at least one trait must be specified");
Expand Down Expand Up @@ -505,7 +505,7 @@ impl<'a> NestedImplTraitVisitor<'a> {

impl<'a> Visitor<'a> for NestedImplTraitVisitor<'a> {
fn visit_ty(&mut self, t: &'a Ty) {
if let TyKind::ImplTrait(_) = t.node {
if let TyKind::ImplTrait(..) = t.node {
if let Some(outer_impl_trait) = self.outer_impl_trait {
struct_span_err!(self.session, t.span, E0666,
"nested `impl Trait` is not allowed")
Expand Down Expand Up @@ -570,7 +570,7 @@ impl<'a> ImplTraitProjectionVisitor<'a> {
impl<'a> Visitor<'a> for ImplTraitProjectionVisitor<'a> {
fn visit_ty(&mut self, t: &'a Ty) {
match t.node {
TyKind::ImplTrait(_) => {
TyKind::ImplTrait(..) => {
if self.is_banned {
struct_span_err!(self.session, t.span, E0667,
"`impl Trait` is not allowed in path parameters")
Expand Down
9 changes: 5 additions & 4 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -777,8 +777,8 @@ impl<'a, 'tcx> Visitor<'tcx> for Resolver<'a> {
visit::walk_fn_ret_ty(self, &declaration.output);

// Resolve the function body, potentially inside the body of an async closure
if let IsAsync::Async(async_closure_id) = asyncness {
let rib_kind = ClosureRibKind(async_closure_id);
if let IsAsync::Async { closure_id, .. } = asyncness {
let rib_kind = ClosureRibKind(closure_id);
self.ribs[ValueNS].push(Rib::new(rib_kind));
self.label_ribs.push(Rib::new(rib_kind));
}
Expand Down Expand Up @@ -3935,8 +3935,9 @@ impl<'a> Resolver<'a> {
// resolve the arguments within the proper scopes so that usages of them inside the
// closure are detected as upvars rather than normal closure arg usages.
ExprKind::Closure(
_, IsAsync::Async(inner_closure_id), _, ref fn_decl, ref body, _span) =>
{
_, IsAsync::Async { closure_id: inner_closure_id, .. }, _,
ref fn_decl, ref body, _span,
) => {
let rib_kind = ClosureRibKind(expr.id);
self.ribs[ValueNS].push(Rib::new(rib_kind));
self.label_ribs.push(Rib::new(rib_kind));
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_save_analysis/sig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ impl Sig for ast::Ty {
let nested = pprust::bounds_to_string(bounds);
Ok(text_sig(nested))
}
ast::TyKind::ImplTrait(ref bounds) => {
ast::TyKind::ImplTrait(_, ref bounds) => {
// FIXME recurse into bounds
let nested = pprust::bounds_to_string(bounds);
Ok(text_sig(format!("impl {}", nested)))
Expand Down
20 changes: 17 additions & 3 deletions src/libsyntax/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1547,7 +1547,11 @@ pub enum TyKind {
TraitObject(GenericBounds, TraitObjectSyntax),
/// An `impl Bound1 + Bound2 + Bound3` type
/// where `Bound` is a trait or a lifetime.
ImplTrait(GenericBounds),
///
/// The `NodeId` exists to prevent lowering from having to
/// generate `NodeId`s on the fly, which would complicate
/// the generation of `existential type` items significantly
ImplTrait(NodeId, GenericBounds),
/// No-op; kept solely so that we can pretty-print faithfully
Paren(P<Ty>),
/// Unused for now
Expand Down Expand Up @@ -1718,18 +1722,28 @@ pub enum Unsafety {

#[derive(Copy, Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
pub enum IsAsync {
Async(NodeId),
Async {
closure_id: NodeId,
return_impl_trait_id: NodeId,
},
NotAsync,
}

impl IsAsync {
pub fn is_async(self) -> bool {
if let IsAsync::Async(_) = self {
if let IsAsync::Async { .. } = self {
true
} else {
false
}
}
/// In case this is an `Async` return the `NodeId` for the generated impl Trait item
pub fn opt_return_id(self) -> Option<NodeId> {
match self {
IsAsync::Async { return_impl_trait_id, .. } => Some(return_impl_trait_id),
IsAsync::NotAsync => None,
}
}
}

#[derive(Copy, Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1726,7 +1726,7 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
"labels on blocks are unstable");
}
}
ast::ExprKind::Closure(_, ast::IsAsync::Async(_), ..) => {
ast::ExprKind::Closure(_, ast::IsAsync::Async { .. }, ..) => {
gate_feature_post!(&self, async_await, e.span, "async closures are unstable");
}
ast::ExprKind::Async(..) => {
Expand Down
35 changes: 23 additions & 12 deletions src/libsyntax/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ pub trait Folder : Sized {
noop_fold_fn_decl(d, self)
}

fn fold_asyncness(&mut self, a: IsAsync) -> IsAsync {
noop_fold_asyncness(a, self)
}

fn fold_block(&mut self, b: P<Block>) -> P<Block> {
noop_fold_block(b, self)
}
Expand Down Expand Up @@ -396,8 +400,8 @@ pub fn noop_fold_ty<T: Folder>(t: P<Ty>, fld: &mut T) -> P<Ty> {
TyKind::TraitObject(bounds, syntax) => {
TyKind::TraitObject(bounds.move_map(|b| fld.fold_param_bound(b)), syntax)
}
TyKind::ImplTrait(bounds) => {
TyKind::ImplTrait(bounds.move_map(|b| fld.fold_param_bound(b)))
TyKind::ImplTrait(id, bounds) => {
TyKind::ImplTrait(fld.new_id(id), bounds.move_map(|b| fld.fold_param_bound(b)))
}
TyKind::Mac(mac) => {
TyKind::Mac(fld.fold_mac(mac))
Expand Down Expand Up @@ -669,6 +673,16 @@ pub fn noop_fold_interpolated<T: Folder>(nt: token::Nonterminal, fld: &mut T)
}
}

pub fn noop_fold_asyncness<T: Folder>(asyncness: IsAsync, fld: &mut T) -> IsAsync {
match asyncness {
IsAsync::Async { closure_id, return_impl_trait_id } => IsAsync::Async {
closure_id: fld.new_id(closure_id),
return_impl_trait_id: fld.new_id(return_impl_trait_id),
},
IsAsync::NotAsync => IsAsync::NotAsync,
}
}

pub fn noop_fold_fn_decl<T: Folder>(decl: P<FnDecl>, fld: &mut T) -> P<FnDecl> {
decl.map(|FnDecl {inputs, output, variadic}| FnDecl {
inputs: inputs.move_map(|x| fld.fold_arg(x)),
Expand Down Expand Up @@ -996,10 +1010,7 @@ pub fn noop_fold_impl_item<T: Folder>(i: ImplItem, folder: &mut T)
}

pub fn noop_fold_fn_header<T: Folder>(mut header: FnHeader, folder: &mut T) -> FnHeader {
header.asyncness = match header.asyncness {
IsAsync::Async(node_id) => IsAsync::Async(folder.new_id(node_id)),
IsAsync::NotAsync => IsAsync::NotAsync,
};
header.asyncness = folder.fold_asyncness(header.asyncness);
header
}

Expand Down Expand Up @@ -1249,12 +1260,8 @@ pub fn noop_fold_expr<T: Folder>(Expr {id, node, span, attrs}: Expr, folder: &mu
arms.move_map(|x| folder.fold_arm(x)))
}
ExprKind::Closure(capture_clause, asyncness, movability, decl, body, span) => {
let asyncness = match asyncness {
IsAsync::Async(node_id) => IsAsync::Async(folder.new_id(node_id)),
IsAsync::NotAsync => IsAsync::NotAsync,
};
ExprKind::Closure(capture_clause,
asyncness,
folder.fold_asyncness(asyncness),
movability,
folder.fold_fn_decl(decl),
folder.fold_expr(body),
Expand All @@ -1265,7 +1272,11 @@ pub fn noop_fold_expr<T: Folder>(Expr {id, node, span, attrs}: Expr, folder: &mu
opt_label.map(|label| folder.fold_label(label)))
}
ExprKind::Async(capture_clause, node_id, body) => {
ExprKind::Async(capture_clause, folder.new_id(node_id), folder.fold_block(body))
ExprKind::Async(
capture_clause,
folder.new_id(node_id),
folder.fold_block(body),
)
}
ExprKind::Assign(el, er) => {
ExprKind::Assign(folder.fold_expr(el), folder.fold_expr(er))
Expand Down
Loading