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

Move structures from ty/mod.rs into their own modules #111503

Closed
wants to merge 39 commits into from

Conversation

WaffleLapkin
Copy link
Member

Revival of #111208, because github refuses to work correctly.

r? @compiler-errors
cc @cjgillot


I've implemented most of the suggestions from #111208's reviews.

Exceptions:

  • ToPredicate and InstantiatedPredicates
    • They are both not part of the predicate representation, but rather related things
    • IMO they make more sense as submodules
  • One-off queries stuff
    • I'm not sure where to put them
    • An option could be ty::query (and move the macros in like ty::query::macros), but idk

I'd also like to split adt stuff at least a little, for example

mod adt {
    mod field {}
    mod variant {}
}

…tlivesPredicate, RegionOutlivesPredicate, TypeOutlivesPredicate}` to their own little module (cute)
…InherentImpls, CrateVariancesMap, Destructor, DestructuredConst, ImplHeader, ImplOverlapKind, ImplSubject, InferVarInfo, OpaqueTypeKey, VariantDiscr, VariantFlags}` to their own little modules (cute)
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 12, 2023
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Maybe let's see what @cjgillot thinks. Also this should be squashed at the end -- it doesn't necessarily need to be 1 commit, but also I think 40 commits is a bit excessive.

@@ -0,0 +1,3 @@
use crate::ty::{BoundRegion, Placeholder};
Copy link
Member

Choose a reason for hiding this comment

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

You should just move it into sty with the rest of the region variants, then later if you want to break up sty, all the region variants can be moved into a file here. This file in its current state is kinda misleading, lmao

@@ -0,0 +1,103 @@
use crate::ty::{
Copy link
Member

Choose a reason for hiding this comment

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

I still think that this should live in predicates. These are essentially just Into implementations for Predicate, and it's easier to just have them live with the types they're defined on.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we ought to just not have any of the predicate/ files and isntead put it all in predicate.rs there's barely anything in the submodules and they would fit in fine in predicate.rs

Copy link
Member Author

Choose a reason for hiding this comment

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

@compiler-errors Most of those are not implemented for Predicates though, so I'm not sure I follow...

@BoxyUwU I don't agree, having the same points as before: adding more entities to the same file makes it harder to add something there (it's easy to mess with the order) and harder to navigate... I can understand the reasoning behind having all predicate parts in a single file (after all, it's essentially a single thing split into multiple types), but I can't see the point in merging ToPredicate, InstantiatedPredicates and CratePredicatesMap there too.

Copy link
Member

@compiler-errors compiler-errors May 16, 2023

Choose a reason for hiding this comment

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

Most of those are not implemented for Predicates though, so I'm not sure I follow...

What? Except for the blanket impl, almost all of these are turning PolyXPredicates into ty::Predicate.

@@ -0,0 +1,8 @@
use crate::ty::{self, VariantIdx};
Copy link
Member

Choose a reason for hiding this comment

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

I think this probably should just live with the rest of the consts..

@bors
Copy link
Contributor

bors commented May 15, 2023

☔ The latest upstream changes (presumably #111570) made this pull request unmergeable. Please resolve the merge conflicts.

span_bug,
ty::{ResolverAstLowering, TyCtxt},
};
use rustc_middle::{resolver_outputs::ResolverAstLowering, span_bug, ty::TyCtxt};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make 3 imports of this?

/// Contains information needed to resolve types and (in the future) look up
/// the types of AST nodes.
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
pub struct CReaderCacheKey {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is should be in ty::context, close to its actual use.


#[derive(Clone, Debug, PartialEq, Eq, Copy, Hash, TyEncodable, TyDecodable, HashStable)]
#[derive(TypeFoldable, TypeVisitable)]
pub struct ClosureSizeProfileData<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In ty::closure or typeck_results, where it is used?

use rustc_hir::def_id::DefId;

#[derive(Copy, Clone, Debug, HashStable, Encodable, Decodable)]
pub struct Destructor {
Copy link
Contributor

Choose a reason for hiding this comment

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

In ty::adt?

/// `tcx.variances_of()` to get the variance for a *particular*
/// item.
#[derive(HashStable, Debug)]
pub struct CrateVariancesMap<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Variances are mostly used in ty::relate, aren't they?

/// another.
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
#[derive(HashStable, TyEncodable, TyDecodable)]
pub struct Placeholder<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In rustc_type_ir?

/// like `Foo<isize,usize>`, then the `InstantiatedPredicates` would be `[[],
/// [usize:Bar<isize>]]`.
#[derive(Clone, Debug, TypeFoldable, TypeVisitable)]
pub struct InstantiatedPredicates<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in ty::predicate too?

use crate::ty::TyCtxt;

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, TyEncodable, HashStable)]
pub struct SymbolName<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In mir::mono?

pub type PlaceholderType = Placeholder<BoundTy>;

#[derive(Debug, Default, Copy, Clone)]
pub struct InferVarInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be moved to rustc_hir_typeck.

Comment on lines +3 to +9
/// Use this rather than `TyKind`, whenever possible.
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, HashStable)]
#[rustc_diagnostic_item = "Ty"]
#[rustc_pass_by_value]
pub struct Ty<'tcx>(pub(super) Interned<'tcx, WithCachedTypeInfo<TyKind<'tcx>>>);

pub type PlaceholderType = Placeholder<BoundTy>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Those should be in sty until you get to break it.

@apiraino
Copy link
Contributor

I see a few open comments so I think this can be switched to waiting on the author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2023
@WaffleLapkin
Copy link
Member Author

This is not going anywhere in this state. I'll try to do smaller PRs that only move items of a certain theme, so that it's actually possible to review and do the changes (this is basically impossible to rebase).

@WaffleLapkin WaffleLapkin deleted the 🌸cutify_ty🌸 branch February 6, 2025 21:58
@WaffleLapkin
Copy link
Member Author

Status update: I did not in fact do smaller PRs. I still think that giant mod.rs is not the best, but oh well. Maybe I'll find time some day

@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants