Add new entries to TypeName
#371
Conversation
There was a problem hiding this comment.
Looks like a good start. But reading through this (and sort of from what I was thinking before), I'm wondering if we're better off keeping these in chalk-rust-ir, instead of chalk-ir. And ApplicationTy.name to be Interner::TypeName. I need to think about this a little bit more.
| pub struct StructId<I: Interner>(pub I::DefId); | ||
|
|
||
| #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct FnDefId<I: Interner>(pub I::DefId); | ||
|
|
||
| #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct ClosureId<I: Interner>(pub I::DefId); |
There was a problem hiding this comment.
We had previously talked about moving all of these as associated types on Interner. This is making me somewhat think this is a good idea. But maybe not in this PR.
| pub struct ClosureDefn { | ||
| // What's a closure's name? Need to generate an `Identifier` or something | ||
| // TODO(markmccaskey): investigate what rustc does here | ||
| pub name: Identifier, |
There was a problem hiding this comment.
Technically, closures aren't nameable. But I'm not sure if rustc generates a name.
There was a problem hiding this comment.
Yes, rustc has an internal name. I think giving them a dummy name is a fine thing to do for chalk tests.
There was a problem hiding this comment.
That said, we'll probably want to (a) consolidate some of this with functions and (b) we may want other things eventually to better match rustc. But we can evolve this part.
chalk-rust-ir/src/lib.rs
Outdated
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| pub struct FnDefDatumBound<I: Interner> { | ||
| pub params: Vec<Ty<I>>, | ||
| pub returns: Vec<Ty<I>>, | ||
| pub where_clauses: Vec<QuantifiedWhereClause<I>>, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| pub struct FnDefFlags { | ||
| pub upstream: bool, | ||
| pub fundamental: bool, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| pub struct FnDefDatum<I: Interner> { | ||
| pub binders: Binders<FnDefDatumBound<I>>, | ||
| pub id: FnDefId<I>, | ||
| pub flags: FnDefFlags, | ||
| } | ||
|
|
||
| impl<I: Interner> FnDefDatum<I> { | ||
| pub fn name(&self, interner: &I) -> TypeName<I> { | ||
| self.id.cast(interner) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| pub struct ClosureDatum<I: Interner> { | ||
| //pub binders: Binders<StructDatumBound<I>>, | ||
| pub id: ClosureId<I>, | ||
| } | ||
|
|
||
| impl<I: Interner> ClosureDatum<I> { | ||
| pub fn name(&self, interner: &I) -> TypeName<I> { | ||
| self.id.cast(interner) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are these here, but the others are in chalk-ir? Plus there's duplicate structs like FnDefFlags/FnFlags
There was a problem hiding this comment.
The current implementation is pretty much just a duplication and slight modification of Struct as a starting point. For example, chalk_ast::StructFlags and chalk_rust_ir::StructFlags turned into FnDefFlags and FnFlags. And FnDefDatum is here because StructDatum is also here!
I'm oscillating between thinking I understand the goal and the pieces I'm working with and thinking I don't understand it 😄 . I'll go back to the basics and see if I can get a better understanding of the pieces involved here! The review helped me realize that my working understanding of Field was off base, for example, and perhaps I've missed other important details!
| @@ -275,6 +275,172 @@ impl<I: Interner> ToProgramClauses<I> for StructDatum<I> { | |||
| } | |||
| } | |||
|
|
|||
| impl<I: Interner> ToProgramClauses<I> for FnDefDatum<I> { | |||
There was a problem hiding this comment.
This seems mostly duplicated from StructDatum?
There was a problem hiding this comment.
Interesting stuff! We'll have to think about just which rules we want here...
MarkMcCaskey
force-pushed
the
feature/improve-type-name
branch
from
1e765c2 to
0430199
Compare
MarkMcCaskey
force-pushed
the
feature/improve-type-name
branch
from
0430199 to
0fe8a29
Compare
|
Thanks for the review! I'm realizing that I've probably gone a bit too far into unknown territory, so I'm going to read through the docs on chalk and think about this some more to try to get a more solid foundation before continuing tomorrow night. |
|
@MarkMcCaskey perhaps, but actually I thnk the code you wrote is all basically "on target" -- that is, the details of closures/functions might be a bit off, but you've got basically the right pieces in basically the right places :) |
|
I added basic parsing for (empty) function definitions so we can have tests for this. I noticed in the tests there's code like, impl<T> Print for OnlySized<T> {
// fn print() {
// println!("{}", std::mem::size_of::<T>());
// }
}Is it in the scope of this PR to make this work? |
|
@MarkMcCaskey definitely not =) |
|
Closing in favor of #394 -- let's revisit this stuff once that one lands. |
Current progress is:
Part of #368