remove AliasTy::def_id()#158013
Conversation
rustbot
added
S-waiting-on-review
rustbot
added
the
WG-trait-system-refactor
|
|
| self.opt_rpitit_info(def_id).is_some() | ||
| } | ||
|
|
||
| pub fn get_impl_future_output_ty(self, ty: Ty<'tcx>) -> Option<Ty<'tcx>> { |
There was a problem hiding this comment.
I moved this from TypeErrCtxt to TyCtxt... maybe I should split this to another commit
There was a problem hiding this comment.
could be nice yeah, but also was fine as is for me when reviewing
| pub type ProjectionAliasTy<'tcx> = ir::ProjectionAliasTy<TyCtxt<'tcx>>; | ||
| pub type InherentAliasTy<'tcx> = ir::InherentAliasTy<TyCtxt<'tcx>>; | ||
| pub type OpaqueAliasTy<'tcx> = ir::OpaqueAliasTy<TyCtxt<'tcx>>; | ||
| pub type FreeAliasTy<'tcx> = ir::FreeAliasTy<TyCtxt<'tcx>>; |
There was a problem hiding this comment.
some of these are unused... I'm not sure what the preference is here, if we should have unused useful things that fill out a "pattern", or if we should strictly add only what is actually used. (same question for try_to_inherent and stuff)
|
|
||
| let (ty::Projection { def_id } | ty::Inherent { def_id }) = proj_ty.kind else { | ||
| panic!("expected projection or inherent alias, found {:?}", proj_ty.kind); | ||
| }; |
There was a problem hiding this comment.
these variants were guarded against and and def_id fetched in the caller... there's sort of three ways to handle this:
- guard in the caller, pass in the matched def_id as an additional argument alongside proj_ty
- guard again in the callee, here, panicing if it's wrong (what I kind of arbitrarily chose to do here, and what I did in previous PRs)
- create another Kind enum, for just Projection/Inherent, and pass in an
Alias<>with that kind
There was a problem hiding this comment.
panic'ing seems fine for now 🤔 needing a ProjectionOrInherentAlias seems like maybe slightly overkill
| pub use rustc_ast_ir::{FloatTy, IntTy, Movability, Mutability, Pinnedness, UintTy}; | ||
| use rustc_type_ir_macros::GenericTypeVisitable; | ||
| pub use ty::{Alias, AliasTerm, AliasTy, UnevaluatedConst}; | ||
| pub use ty::{Alias, *}; |
There was a problem hiding this comment.
unrelated to this PR, I realized there's a conflict here - rustc_type_ir::Alias used to resolve to the TyKind::Alias variant, now it's the Alias struct. A bit unfortunate, but fine I guess.
(also IMO having a nested ty module (recently added, in the Alias<> PR) is very confusing (since rustc_type_ir is itself imported with the alias name of ty absolutely everywhere in the compiler), but oh well)
| ty::Opaque { def_id: key.def_id.into() }, | ||
| key.args, | ||
| ) | ||
| .try_to_opaque() |
There was a problem hiding this comment.
hmm, why this indirection?
There was a problem hiding this comment.
there is currently no way to construct a OpaqueAliasTy directly (in particular, I want to make sure debug_assert_args_compatible is called).
- option 1: make a bespoke constructor for OpaqueAliasTy (and optionally Projection/Inherent/Free too, depending on the policy on unused code, I'm not sure)
- option 2: some kind of generic constructor, hmm. Maybe one that takes
T: Into<AliasTerm>, and then call an AliasTerm-baseddebug_assert_args_compatible? - option 3: hit it with a stupid hammer and use the AliasTy constructor and
.try_to_opaque()
I went with option 3 :s (option 1 I was nervous about unused code, option 2 runs into the issue that we talked about of I::OpaqueTyId not being a newtype wrapper - I::OpaqueTyId can't impl Into<AliasTerm> at the moment, it's just a DefId). Happy to refactor, I just dunno what the nicest thing to do is
There was a problem hiding this comment.
I think that 1 is fine, id personally kinda bias towards adding all the dead code for the other kinds of aliases just so it's less annoying when they do actually need to be used, but it's also fine to only add the stuff actually needed 😌
|
looked over it seems good to me :3 |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| .tcx | ||
| .infer_ctxt() | ||
| .build(cx.typing_mode()) | ||
| .err_ctxt() | ||
| .tcx |
There was a problem hiding this comment.
I think we may no longer need to construct an InferCtxt here. If I understand correctly, it was previously required to obtain an err_ctxt and then call get_impl_future_output_ty. Since we already have access to tcx, would it make sense to call cx.tcx.get_impl_future_output_ty(ret_ty) directly instead (as we moved it to tcx)?
There was a problem hiding this comment.
good point! I did the move too quickly and didn't realize we could skip building it entirely. Nice, pushed!
| @@ -36,7 +35,7 @@ fn result_err_ty<'tcx>( | |||
| .tcx | |||
| .infer_ctxt() | |||
| .build(cx.typing_mode()) | |||
| .err_ctxt() | |||
| .tcx | |||
There was a problem hiding this comment.
I think this follows the same, we can directly use tcx?
| let infcx = cx.tcx.infer_ctxt().build(cx.typing_mode()); | ||
| if let Some(future_ty) = infcx.err_ctxt().get_impl_future_output_ty(return_ty(cx, item_id)) | ||
| if let Some(future_ty) = infcx.tcx.get_impl_future_output_ty(return_ty(cx, item_id)) |
There was a problem hiding this comment.
I think follows the same, we can use cx.tcx.get_impl_future_output_ty?
|
@bors r+ rollup=never p=1 |
rust-bors
Bot
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
rust-bors
Bot
added
the
S-waiting-on-review
rust-bors
Bot
removed
the
S-waiting-on-bors
|
💔 Test for 7f16348 failed: CI. Failed job:
|
rust-bors
Bot
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
remove AliasTy::def_id() try-job: dist-aarch64-apple
This comment has been minimized.
This comment has been minimized.
rust-bors
Bot
added
merged-by-bors
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 4429659 (parent) -> f28ac76 (this PR) Test differencesShow 56 test diffs56 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard f28ac764c36004fa6a6e098d15b4016a838c13c6 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (f28ac76): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (secondary -3.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 504.341s -> 510.446s (1.21%) |
8 participants
View all comments
this is part of #156181
as well as part of #152245
this immediately uses the recently-introduced
Alias<>type to narrow the kinds of aliases allowed in various places in codefyi @lcnr
r? @BoxyUwU