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.

Sign up for GitHub

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

Don't keep {Closure,Generator}Substs synthetics in an Instance. #74341

Closed
wants to merge 2 commits into from
Closed

Don't keep {Closure,Generator}Substs synthetics in an Instance. #74341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5533705
rustc_typeck: construct {Closure,Generator}Substs more directly.
eddyb Jul 13, 2020
6e4b8ef
Don't keep {Closure,Generator}Substs synthetics in an Instance.
eddyb Jul 14, 2020
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
2 changes: 1 addition & 1 deletion src/librustc_codegen_ssa/mir/rvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let instance = Instance::resolve_closure(
bx.cx().tcx(),
def_id,
substs,
substs.as_closure(),
ty::ClosureKind::FnOnce,
);
OperandValue::Immediate(bx.cx().get_fn_addr(instance))
Expand Down
18 changes: 9 additions & 9 deletions src/librustc_middle/ty/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,14 +382,14 @@ impl<'tcx> Instance<'tcx> {
pub fn resolve_closure(
tcx: TyCtxt<'tcx>,
def_id: DefId,
substs: ty::SubstsRef<'tcx>,
closure_substs: ty::ClosureSubsts<'tcx>,
requested_kind: ty::ClosureKind,
) -> Instance<'tcx> {
let actual_kind = substs.as_closure().kind();
let actual_kind = closure_substs.kind();

match needs_fn_once_adapter_shim(actual_kind, requested_kind) {
Ok(true) => Instance::fn_once_adapter_instance(tcx, def_id, substs),
_ => Instance::new(def_id, substs),
Ok(true) => Instance::fn_once_adapter_instance(tcx, def_id, closure_substs),
_ => Instance::new(def_id, tcx.intern_substs(closure_substs.base_substs())),
}
}

Expand All @@ -399,12 +399,12 @@ impl<'tcx> Instance<'tcx> {
Instance::resolve(tcx, ty::ParamEnv::reveal_all(), def_id, substs).unwrap().unwrap()
}

pub fn fn_once_adapter_instance(
fn fn_once_adapter_instance(
tcx: TyCtxt<'tcx>,
closure_did: DefId,
substs: ty::SubstsRef<'tcx>,
closure_substs: ty::ClosureSubsts<'tcx>,
) -> Instance<'tcx> {
debug!("fn_once_adapter_shim({:?}, {:?})", closure_did, substs);
debug!("fn_once_adapter_shim({:?}, {:?})", closure_did, closure_substs);
let fn_once = tcx.require_lang_item(FnOnceTraitLangItem, None);
let call_once = tcx
.associated_items(fn_once)
Expand All @@ -414,9 +414,9 @@ impl<'tcx> Instance<'tcx> {
.def_id;
let def = ty::InstanceDef::ClosureOnceShim { call_once };

let self_ty = tcx.mk_closure(closure_did, substs);
let self_ty = tcx.mk_closure(closure_did, closure_substs.substs);

let sig = substs.as_closure().sig();
let sig = closure_substs.sig();
let sig = tcx.normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), &sig);
assert_eq!(sig.inputs().len(), 1);
let substs = tcx.mk_substs_trait(self_ty, &[sig.inputs()[0].into()]);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let instance = ty::Instance::resolve_closure(
*self.tcx,
def_id,
substs,
substs.as_closure(),
ty::ClosureKind::FnOnce,
);
let fn_ptr = self.memory.create_fn_alloc(FnVal::Instance(instance));
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/monomorphize/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
let instance = Instance::resolve_closure(
self.tcx,
def_id,
substs,
substs.as_closure(),
ty::ClosureKind::FnOnce,
);
if should_monomorphize_locally(self.tcx, &instance) {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_ty/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ fn resolve_associated_item<'tcx>(
Some(Instance::resolve_closure(
tcx,
closure_data.closure_def_id,
closure_data.substs,
closure_data.substs.as_closure(),
trait_closure_kind,
))
}
Expand Down
24 changes: 0 additions & 24 deletions src/librustc_typeck/collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,6 @@ impl Visitor<'tcx> for CollectItemTypesVisitor<'tcx> {
if let hir::ExprKind::Closure(..) = expr.kind {
let def_id = self.tcx.hir().local_def_id(expr.hir_id);
self.tcx.ensure().generics_of(def_id);
self.tcx.ensure().type_of(def_id);
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 removed this line to make 29 UI tests pass - their only failure mode was missing some of the errors.

I believe what's happening is that this would now trigger type-checking the body of the closure, before bodies are normally type-checked, and so it trips on an "abort if any errors" check.

Can we start removing those "checkpoints" that turn regular errors into fatal ones, given how on-demand some of the various checks are nowadays? cc @estebank

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are only a handful of FatalError.raise()s left, and the ones that do remain do so because they'd require a much larger refactor to get rid of them.

Copy link
Member Author

@eddyb eddyb Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If they're because of having to return a value, I suspect using Result<T, ErrorReported> could work well.

(But I meant "abort if there were any errors" "checkpoints", which we can just remove)

}
intravisit::walk_expr(self, expr);
}
Expand Down Expand Up @@ -1362,29 +1361,6 @@ fn generics_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::Generics {
}
}));

// provide junk type parameter defs - the only place that
// cares about anything but the length is instantiation,
// and we don't do that for closures.
if let Node::Expr(&hir::Expr { kind: hir::ExprKind::Closure(.., gen), .. }) = node {
let dummy_args = if gen.is_some() {
&["<resume_ty>", "<yield_ty>", "<return_ty>", "<witness>", "<upvars>"][..]
} else {
&["<closure_kind>", "<closure_signature>", "<upvars>"][..]
};

params.extend(dummy_args.iter().enumerate().map(|(i, &arg)| ty::GenericParamDef {
index: type_start + i as u32,
name: Symbol::intern(arg),
def_id,
pure_wrt_drop: false,
kind: ty::GenericParamDefKind::Type {
has_default: false,
object_lifetime_default: rl::Set1::Empty,
synthetic: None,
},
}));
}

let param_def_id_to_index = params.iter().map(|param| (param.def_id, param.index)).collect();

ty::Generics {
Expand Down
9 changes: 2 additions & 7 deletions src/librustc_typeck/collect/type_of.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,8 @@ pub(super) fn type_of(tcx: TyCtxt<'_>, def_id: DefId) -> Ty<'_> {

Node::Field(field) => icx.to_ty(&field.ty),

Node::Expr(&Expr { kind: ExprKind::Closure(.., gen), .. }) => {
let substs = InternalSubsts::identity_for_item(tcx, def_id);
if let Some(movability) = gen {
tcx.mk_generator(def_id, substs, movability)
} else {
tcx.mk_closure(def_id, substs)
}
Node::Expr(&Expr { kind: ExprKind::Closure(..), .. }) => {
tcx.typeck_tables_of(def_id.expect_local()).node_type(hir_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels wrong to me. We basically undid a move much like this for generators, though partly it was to avoid those type error problems you were referring to with your other change.

But also it is just surprising to me to have type_of require us to type-check the closure. For every other def-id, getting type_of doesn't require a full type-check, but rather returns a "fully generic" type. i.e., I would expet the type of a closure or generator ought to be expressed as a purely generic type like Closure<P0...Pn>, much like we'd expect for a function, not one that embeds full types like Closure<..., (u32, u64)> or whatever. Do we have any other example that works like that?

Anyway, what is the exact motivation here? It's confusing to me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok, I see, the point is that you want Instance to be able to substitute only the base_params and to still get a valid type... hmm. Interesting. I'm not sure what I think about it. I'm surprised this gives much of a win, can you explain why it does?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do land this, I feel like we need some kind of comment..somewhere...that explains this rather subtle point. I guess it would be best to document it on the ClosureSubsts and GeneratorSubsts structs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ok, I see, the point is that you want Instance to be able to substitute only the base_params and to still get a valid type... hmm. Interesting. I'm not sure what I think about it.

Me neither, I think I'd prefer if we could implement e.g. fn_sig on closures instead, and use that from Instance.
I could look into that approach once #74314 lands.

I'm surprised this gives much of a win, can you explain why it does?

All the wins are incremental, so it probably only makes sense if a closure changes its capture set/types, although that would still imply codegen is needed?

Or maybe non-generic closures are involved, which would now result in an Instance with empty substs, which may be handled differently by CGUs.

}

Node::AnonConst(_) => {
Expand Down