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

Jump to bottom

librustc_middle: change query functions to accept impl Into<$K> type #70956

Closed
wants to merge 2 commits into from
Closed

librustc_middle: change query functions to accept impl Into<$K> type #70956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3bf24f
librustc_middle: change query functions to accept impl Into<$K> type
marmeladema Apr 9, 2020
3fe4327
librustc_span: implement From<LocalDefId> for DefId
marmeladema Apr 9, 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/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let values: Vec<_> = (0..fields)
.map(|field| {
let field = bx.tcx().const_field(
ty::ParamEnv::reveal_all().and((&c, mir::Field::new(field as usize))),
ty::ParamEnv::reveal_all().and((c, mir::Field::new(field as usize))),
);
if let Some(prim) = field.try_to_scalar() {
let layout = bx.layout_of(field_ty);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_infer/infer/error_reporting/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1773,7 +1773,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
(Some(ref table), GenericKind::Param(ref param)) => {
let table_owner = table.borrow().hir_owner;
table_owner.and_then(|table_owner| {
let generics = self.tcx.generics_of(table_owner.to_def_id());
let generics = self.tcx.generics_of(table_owner);
// Account for the case where `param` corresponds to `Self`,
// which doesn't have the expected type argument.
if !(generics.has_self && param.index == 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_middle/mir/mono.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ impl<'tcx> MonoItem<'tcx> {
MonoItem::GlobalAsm(..) => return true,
};

tcx.substitute_normalize_and_test_predicates((def_id, &substs))
tcx.substitute_normalize_and_test_predicates((def_id, substs))
}

pub fn to_string(&self, tcx: TyCtxt<'tcx>, debug: bool) -> String {
Expand Down
10 changes: 5 additions & 5 deletions src/librustc_middle/ty/query/plumbing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,8 @@ macro_rules! define_queries_inner {
impl TyCtxtEnsure<$tcx> {
$($(#[$attr])*
#[inline(always)]
pub fn $name(self, key: $K) {
ensure_query::<queries::$name<'_>, _>(self.tcx, key)
pub fn $name(self, key: impl Into<$K>) {
ensure_query::<queries::$name<'_>, _>(self.tcx, key.into())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make some new methods that call the existing ones in this file to avoid making these generic. We want to make sure that ensure_query and get_query is only instantiated in rustc_middle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just that i understand; the proposal here is to add new methods only for methods where the key is a DefId and give them a name similar to the original one but with a prefix (for example). We would end up with methods like local_generics_of for LocalDefId instead of generics_of for DefId?

If so, all callers that have a LocalDefId would have to use the new methods, and then there is not much benefit compared to calling .to_def_id() everywhere. All code will have to be changed anyway, either to call the new methods or to call .to_def_id().

Copy link
Contributor

@ecstatic-morse ecstatic-morse Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe they want the caller of ensure_query to remain monomorphic and for you to introduce a generic wrapper around it that calls .into() on the query param before passing it to the monomorphic version.

})*
}

Expand Down Expand Up @@ -421,7 +421,7 @@ macro_rules! define_queries_inner {

$($(#[$attr])*
#[inline(always)]
pub fn $name(self, key: $K) -> $V {
pub fn $name(self, key: impl Into<$K>) -> $V {
self.at(DUMMY_SP).$name(key)
})*

Expand Down Expand Up @@ -458,8 +458,8 @@ macro_rules! define_queries_inner {
impl TyCtxtAt<$tcx> {
$($(#[$attr])*
#[inline(always)]
pub fn $name(self, key: $K) -> $V {
get_query::<queries::$name<'_>, _>(self.tcx, self.span, key)
pub fn $name(self, key: impl Into<$K>) -> $V {
get_query::<queries::$name<'_>, _>(self.tcx, self.span, key.into())
})*
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_middle/ty/sty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2242,7 +2242,7 @@ impl<'tcx> Const<'tcx> {

let expr = &tcx.hir().body(body_id).value;

let ty = tcx.type_of(def_id.to_def_id());
let ty = tcx.type_of(def_id);

let lit_input = match expr.kind {
hir::ExprKind::Lit(ref lit) => Some(LitToConstInput { lit: &lit.node, ty, neg: false }),
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_middle/ty/trait_def.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ pub(super) fn trait_impls_of_provider(tcx: TyCtxt<'_>, trait_id: DefId) -> &Trai
let mut impls = TraitImpls::default();

{
let mut add_impl = |impl_def_id| {
let mut add_impl = |impl_def_id: DefId| {
let impl_self_ty = tcx.type_of(impl_def_id);
if impl_def_id.is_local() && impl_self_ty.references_error() {
return;
Expand Down
6 changes: 6 additions & 0 deletions src/librustc_span/def_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,12 @@ impl fmt::Debug for DefId {
}
}

impl From<LocalDefId> for DefId {
fn from(local_def_id: LocalDefId) -> DefId {
local_def_id.to_def_id()
}
}
Comment on lines +198 to +202
Copy link
Member

Choose a reason for hiding this comment

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

Another reason to not use From/Into is that we kind of want people to write .to_def_id() instead of .into()... or maybe we don't.

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 am new to all of this and my opinion probably does not matter much but using Into is useful because we don't have to import a new trait everywhere.
Also, if i understand correctly, that new trait would only be implemented for LocalDefId/DefId. What about other key types? Using Into makes it more general, which may or may not be good.

Copy link
Member

Choose a reason for hiding this comment

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

useful because we don't have to import a new trait everywhere.

I was imagining an IntoQueryKey trait which would be private/sealed so that we couldn't import it anywhere, i.e. these aren't conversions we'd let people do by calling .into_query_key() (which has all the same issues .into() does).

Copy link
Contributor

@ecstatic-morse ecstatic-morse Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think to_def_id() adds anything over into(), since anecdotally most variables of type LocalDefId are named def_id anyways. We could always start with a custom trait and later migrate to Into if you still have concerns. We would keep an inherent method on LocalDefId for converting to a DefId so additional trait imports shouldn't be a factor.


rustc_data_structures::define_id_collections!(DefIdMap, DefIdSet, DefId);

/// A LocalDefId is equivalent to a DefId with `krate == LOCAL_CRATE`. Since
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1618,7 +1618,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

fn point_at_param_definition(&self, err: &mut DiagnosticBuilder<'_>, param: ty::ParamTy) {
let generics = self.tcx.generics_of(self.body_id.owner.to_def_id());
let generics = self.tcx.generics_of(self.body_id.owner);
let generic_param = generics.type_param(&param, self.tcx);
if let ty::GenericParamDefKind::Type { synthetic: Some(..), .. } = generic_param.kind {
return;
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/method/suggest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,7 +1023,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if let (Some(ref param), Some(ref table)) = (param_type, self.in_progress_tables) {
let table_owner = table.borrow().hir_owner;
if let Some(table_owner) = table_owner {
let generics = self.tcx.generics_of(table_owner.to_def_id());
let generics = self.tcx.generics_of(table_owner);
let type_param = generics.type_param(param, self.tcx);
let hir = &self.tcx.hir();
if let Some(id) = hir.as_local_hir_id(type_param.def_id) {
Expand Down