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

Use FxHashSet instead of Vec for well formed tys #88771

Merged
merged 2 commits into from
Sep 12, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ impl UniversalRegionRelationsBuilder<'cx, 'tcx> {
// We add implied bounds from both the unnormalized and normalized ty
// See issue #87748
let constraints_implied_1 = self.add_implied_bounds(ty);
let TypeOpOutput { output: ty, constraints: constraints1, .. } = self
let TypeOpOutput { output: norm_ty, constraints: constraints1, .. } = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
.fully_perform(self.infcx)
Expand All @@ -286,8 +286,9 @@ impl UniversalRegionRelationsBuilder<'cx, 'tcx> {
// }
// ```
// Both &Self::Bar and &() are WF
let constraints_implied_2 = self.add_implied_bounds(ty);
normalized_inputs_and_output.push(ty);
let constraints_implied_2 =
if ty != norm_ty { self.add_implied_bounds(norm_ty) } else { None };
normalized_inputs_and_output.push(norm_ty);
constraints1.into_iter().chain(constraints_implied_1).chain(constraints_implied_2)
})
.collect();
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_trait_selection/src/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::traits::query::evaluate_obligation::InferCtxtExt as _;
use crate::traits::query::outlives_bounds::InferCtxtExt as _;
use crate::traits::{self, TraitEngine, TraitEngineExt};

use rustc_data_structures::stable_set::FxHashSet;
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_hir::lang_items::LangItem;
Expand Down Expand Up @@ -184,7 +185,7 @@ pub trait OutlivesEnvironmentExt<'tcx> {
fn add_implied_bounds(
&mut self,
infcx: &InferCtxt<'a, 'tcx>,
fn_sig_tys: &[Ty<'tcx>],
fn_sig_tys: FxHashSet<Ty<'tcx>>,
body_id: hir::HirId,
span: Span,
);
Expand All @@ -210,13 +211,13 @@ impl<'tcx> OutlivesEnvironmentExt<'tcx> for OutlivesEnvironment<'tcx> {
fn add_implied_bounds(
&mut self,
infcx: &InferCtxt<'a, 'tcx>,
fn_sig_tys: &[Ty<'tcx>],
fn_sig_tys: FxHashSet<Ty<'tcx>>,
body_id: hir::HirId,
span: Span,
) {
debug!("add_implied_bounds()");

for &ty in fn_sig_tys {
for ty in fn_sig_tys {
Comment on lines -213 to +220
Copy link
Member

Choose a reason for hiding this comment

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

This is iterating on a FxHashSet<Ty<'tcx>>, which will depend on ASLR (random memory allocation order) and the results could be observable (e.g. potentially in diagnostics order).

I wonder if we should ban iterating FxHash{Set,Map} and force the use of FxIndex{Set,Map} instead (since presumably the insertion order is deterministic).

Copy link
Member

Choose a reason for hiding this comment

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

See also #63713 (comment) - we should probably use internal lints to force this harder.

Copy link
Contributor

@lcnr lcnr Sep 18, 2021

Choose a reason for hiding this comment

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

we should ban iterating FxHash{Set,Map}

YES! I think recommending FxIndex{Set,Map} is probably the right choice, using an internal lint when using a FxIndexSet in a potentially nondet way. Now if the order doesn't matter, e.g. checking for existence without any sideeffects, this lint could be explicitly allowed.

Not sure what's the best way to write such a lint though 😅

let ty = infcx.resolve_vars_if_possible(ty);
debug!("add_implied_bounds: ty = {}", ty);
let implied_bounds = infcx.implied_outlives_bounds(self.param_env, body_id, ty, span);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_typeck/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -688,7 +688,7 @@ fn check_opaque_meets_bounds<'tcx>(
// Finally, resolve all regions. This catches wily misuses of
// lifetime parameters.
let fcx = FnCtxt::new(&inh, param_env, hir_id);
fcx.regionck_item(hir_id, span, &[]);
fcx.regionck_item(hir_id, span, FxHashSet::default());
});
}

Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_typeck/src/check/compare_method.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::errors::LifetimesOrBoundsMismatchOnTrait;
use rustc_data_structures::stable_set::FxHashSet;
use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticId, ErrorReported};
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
Expand Down Expand Up @@ -250,7 +251,7 @@ fn compare_predicate_entailment<'tcx>(
// Compute placeholder form of impl and trait method tys.
let tcx = infcx.tcx;

let mut wf_tys = vec![];
let mut wf_tys = FxHashSet::default();

let (impl_sig, _) = infcx.replace_bound_vars_with_fresh_vars(
impl_m_span,
Expand Down Expand Up @@ -398,7 +399,7 @@ fn compare_predicate_entailment<'tcx>(
// Finally, resolve all regions. This catches wily misuses of
// lifetime parameters.
let fcx = FnCtxt::new(&inh, param_env, impl_m_hir_id);
fcx.regionck_item(impl_m_hir_id, impl_m_span, &wf_tys);
fcx.regionck_item(impl_m_hir_id, impl_m_span, wf_tys);

Ok(())
})
Expand Down Expand Up @@ -1098,7 +1099,7 @@ crate fn compare_const_impl<'tcx>(
}

let fcx = FnCtxt::new(&inh, param_env, impl_c_hir_id);
fcx.regionck_item(impl_c_hir_id, impl_c_span, &[]);
fcx.regionck_item(impl_c_hir_id, impl_c_span, FxHashSet::default());
});
}

Expand Down Expand Up @@ -1216,7 +1217,7 @@ fn compare_type_predicate_entailment<'tcx>(
// Finally, resolve all regions. This catches wily misuses of
// lifetime parameters.
let fcx = FnCtxt::new(&inh, param_env, impl_ty_hir_id);
fcx.regionck_item(impl_ty_hir_id, impl_ty_span, &[]);
fcx.regionck_item(impl_ty_hir_id, impl_ty_span, FxHashSet::default());

Ok(())
})
Expand Down Expand Up @@ -1436,10 +1437,10 @@ pub fn check_type_bounds<'tcx>(
// lifetime parameters.
let fcx = FnCtxt::new(&inh, param_env, impl_ty_hir_id);
let implied_bounds = match impl_ty.container {
ty::TraitContainer(_) => vec![],
ty::TraitContainer(_) => FxHashSet::default(),
ty::ImplContainer(def_id) => fcx.impl_implied_bounds(def_id, impl_ty_span),
};
fcx.regionck_item(impl_ty_hir_id, impl_ty_span, &implied_bounds);
fcx.regionck_item(impl_ty_hir_id, impl_ty_span, implied_bounds);

Ok(())
})
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_typeck/src/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ fn typeck_with_fallback<'tcx>(
// from normalization. We could just discard these, but to align with
// compare_method and elsewhere, we just add implied bounds for
// these types.
let mut wf_tys = vec![];
let mut wf_tys = FxHashSet::default();
// Compute the fty from point of view of inside the fn.
let fn_sig = tcx.liberate_late_bound_regions(def_id.to_def_id(), fn_sig);
wf_tys.extend(fn_sig.inputs_and_output.iter());
Expand Down Expand Up @@ -451,7 +451,7 @@ fn typeck_with_fallback<'tcx>(

fcx.write_ty(id, expected_type);

(fcx, vec![])
(fcx, FxHashSet::default())
};

let fallback_has_occurred = fcx.type_inference_fallback();
Expand All @@ -475,7 +475,7 @@ fn typeck_with_fallback<'tcx>(
fcx.select_all_obligations_or_error();

if fn_sig.is_some() {
fcx.regionck_fn(id, body, span, &wf_tys);
fcx.regionck_fn(id, body, span, wf_tys);
} else {
fcx.regionck_expr(body);
}
Expand Down
14 changes: 5 additions & 9 deletions compiler/rustc_typeck/src/check/regionck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ use crate::check::dropck;
use crate::check::FnCtxt;
use crate::mem_categorization as mc;
use crate::middle::region;
use rustc_data_structures::stable_set::FxHashSet;
use rustc_hir as hir;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
Expand Down Expand Up @@ -126,7 +127,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

/// Region checking during the WF phase for items. `wf_tys` are the
/// types from which we should derive implied bounds, if any.
pub fn regionck_item(&self, item_id: hir::HirId, span: Span, wf_tys: &[Ty<'tcx>]) {
pub fn regionck_item(&self, item_id: hir::HirId, span: Span, wf_tys: FxHashSet<Ty<'tcx>>) {
debug!("regionck_item(item.id={:?}, wf_tys={:?})", item_id, wf_tys);
let subject = self.tcx.hir().local_def_id(item_id);
let mut rcx = RegionCtxt::new(self, item_id, Subject(subject), self.param_env);
Expand All @@ -149,7 +150,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn_id: hir::HirId,
body: &'tcx hir::Body<'tcx>,
span: Span,
wf_tys: &[Ty<'tcx>],
wf_tys: FxHashSet<Ty<'tcx>>,
) {
debug!("regionck_fn(id={})", fn_id);
let subject = self.tcx.hir().body_owner_def_id(body.id());
Expand Down Expand Up @@ -286,15 +287,10 @@ impl<'a, 'tcx> RegionCtxt<'a, 'tcx> {
// because it will have no effect.
//
// FIXME(#27579) return types should not be implied bounds
let fn_sig_tys: Vec<_> =
let fn_sig_tys: FxHashSet<_> =
fn_sig.inputs().iter().cloned().chain(Some(fn_sig.output())).collect();

self.outlives_environment.add_implied_bounds(
self.fcx,
&fn_sig_tys[..],
body_id.hir_id,
span,
);
self.outlives_environment.add_implied_bounds(self.fcx, fn_sig_tys, body_id.hir_id, span);
self.outlives_environment.save_implied_bounds(body_id.hir_id);
self.link_fn_params(&body.params);
self.visit_body(body);
Expand Down
28 changes: 16 additions & 12 deletions compiler/rustc_typeck/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ struct CheckWfFcxBuilder<'tcx> {
impl<'tcx> CheckWfFcxBuilder<'tcx> {
fn with_fcx<F>(&mut self, f: F)
where
F: for<'b> FnOnce(&FnCtxt<'b, 'tcx>) -> Vec<Ty<'tcx>>,
F: for<'b> FnOnce(&FnCtxt<'b, 'tcx>) -> FxHashSet<Ty<'tcx>>,
{
let id = self.id;
let span = self.span;
Expand All @@ -59,7 +59,7 @@ impl<'tcx> CheckWfFcxBuilder<'tcx> {
}
let wf_tys = f(&fcx);
fcx.select_all_obligations_or_error();
fcx.regionck_item(id, span, &wf_tys);
fcx.regionck_item(id, span, wf_tys);
});
}
}
Expand Down Expand Up @@ -394,7 +394,7 @@ fn check_associated_item(
let item = fcx.tcx.associated_item(fcx.tcx.hir().local_def_id(item_id));

let (mut implied_bounds, self_ty) = match item.container {
ty::TraitContainer(_) => (vec![], fcx.tcx.types.self_param),
ty::TraitContainer(_) => (FxHashSet::default(), fcx.tcx.types.self_param),
ty::ImplContainer(def_id) => {
(fcx.impl_implied_bounds(def_id, span), fcx.tcx.type_of(def_id))
}
Expand Down Expand Up @@ -553,7 +553,7 @@ fn check_type_defn<'tcx, F>(
check_where_clauses(fcx, item.span, item.def_id.to_def_id(), None);

// No implied bounds in a struct definition.
vec![]
FxHashSet::default()
});
}

Expand All @@ -579,7 +579,7 @@ fn check_trait(tcx: TyCtxt<'_>, item: &hir::Item<'_>) {
for_item(tcx, item).with_fcx(|fcx| {
check_where_clauses(fcx, item.span, item.def_id.to_def_id(), None);

vec![]
FxHashSet::default()
});
}

Expand Down Expand Up @@ -620,7 +620,7 @@ fn check_item_fn(
for_id(tcx, item_id, span).with_fcx(|fcx| {
let def_id = tcx.hir().local_def_id(item_id);
let sig = tcx.fn_sig(def_id);
let mut implied_bounds = vec![];
let mut implied_bounds = FxHashSet::default();
check_fn_or_method(fcx, ident.span, sig, decl, def_id.to_def_id(), &mut implied_bounds);
implied_bounds
})
Expand Down Expand Up @@ -659,7 +659,7 @@ fn check_item_type(tcx: TyCtxt<'_>, item_id: hir::HirId, ty_span: Span, allow_fo
}

// No implied bounds in a const, etc.
vec![]
FxHashSet::default()
});
}

Expand Down Expand Up @@ -918,14 +918,14 @@ fn check_fn_or_method<'fcx, 'tcx>(
sig: ty::PolyFnSig<'tcx>,
hir_decl: &hir::FnDecl<'_>,
def_id: DefId,
implied_bounds: &mut Vec<Ty<'tcx>>,
implied_bounds: &mut FxHashSet<Ty<'tcx>>,
) {
let sig = fcx.tcx.liberate_late_bound_regions(def_id, sig);

// Unnormalized types in signature are WF too
implied_bounds.extend(sig.inputs());
// FIXME(#27579) return types should not be implied bounds
implied_bounds.push(sig.output());
implied_bounds.insert(sig.output());

// Normalize the input and output types one at a time, using a different
// `WellFormedLoc` for each. We cannot call `normalize_associated_types`
Expand Down Expand Up @@ -977,7 +977,7 @@ fn check_fn_or_method<'fcx, 'tcx>(
);

// FIXME(#27579) return types should not be implied bounds
implied_bounds.push(sig.output());
implied_bounds.insert(sig.output());

debug!(?implied_bounds);

Expand Down Expand Up @@ -1513,7 +1513,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.collect()
}

pub(super) fn impl_implied_bounds(&self, impl_def_id: DefId, span: Span) -> Vec<Ty<'tcx>> {
pub(super) fn impl_implied_bounds(
&self,
impl_def_id: DefId,
span: Span,
) -> FxHashSet<Ty<'tcx>> {
match self.tcx.impl_trait_ref(impl_def_id) {
Some(trait_ref) => {
// Trait impl: take implied bounds from all types that
Expand All @@ -1526,7 +1530,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// Inherent impl: take implied bounds from the `self` type.
let self_ty = self.tcx.type_of(impl_def_id);
let self_ty = self.normalize_associated_types_in(span, self_ty);
vec![self_ty]
std::array::IntoIter::new([self_ty]).collect()
}
}
}
Expand Down