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

Add ProjectionTerm #745

Closed
wants to merge 14 commits into from
Closed

Conversation

JulianKnodt
Copy link

This starts adding terms into chalk so that associated consts can be equated.

@compiler-errors
Copy link
Member

howdy @JulianKnodt, what's the status of this?

@JulianKnodt
Copy link
Author

@compiler-errors Started on it, been caught up with some other deadlines so have not had a chance to progress, will get to it eventualllyyyy but I'm unsure when I'll be able to.

@bors
Copy link
Collaborator

bors commented Mar 9, 2022

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

@HKalbasi
Copy link
Member

Is this PR in the right direction? I think chalk should not get its hand dirty with const evaluation, since const eval needs handling and interpreting whole rust syntax, and doing that in chalk is against modularization. I imagine that the user of chalk should provide const eval infrastructure, and evaluate const generics so chalk has them in the concrete shape.

In this way, chalk doesn't need to normalize associated constants (it can't normalize completely without full const eval), it just needs to provide a function that computes an ImplId + substitution from a <Type as Trait>, so that the caller can find associated constants and evaluate them (and other associated items for other reasons).

@JulianKnodt
Copy link
Author

@HKalbasi It's been a minute since I started this, and it's a good reminder for me to go back and actually implement this (I'm starting to finally get more time on my hands), but the context for this is that it would make sense to support const equality in the context of trait bounds (see here). While I don't know whether Chalk needs to support evaluating them, my understanding from a few months ago that chalk needs to support consts in a new position which it previously did not.

If any of this is wrong, please let me know, I haven't thought about this in a minute and should get back to it.

@HKalbasi
Copy link
Member

I thought it is going to add a NormalizeConst (which is essentially const eval) similar to NormalizeFn in #726 and handles associated constants in <Type as Trait>::ASSOC_CONST position, but it is about associated constants in Trait<ASSOC_CONST=?> position, so my comment above is irrelevant. Sorry for noise.

@JulianKnodt JulianKnodt force-pushed the add_term branch 3 times, most recently from 0d9d9af to 27e6925 Compare April 23, 2022 19:19
This just changes the name of projection type to term, which highlights where
other changes to using a term should go.
@JulianKnodt
Copy link
Author

JulianKnodt commented May 5, 2022

@compiler-errors Do you know who I can ask about Chalk? I'm starting to run into things I am unsure about, and am unsure where to look. Specifically, is there any existing infrastructure for consts inside of chalk?

@jackh726
Copy link
Member

jackh726 commented May 5, 2022

Generally I or @nikomatsakis should have the most knowledge of Chalk things. Easiest to ask in Zulip.

There is very little infrastructure for consts in Chalk. I think most/all of it is in chalk-ir, for variable kinds and such.

Where trivial, add ConstEq, otherwise defer to todos
);
}
AssociatedTermValueBound::Const(ct) => {
todo!();
Copy link
Author

Choose a reason for hiding this comment

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

Am unsure why this is not causing anything to break

@bors
Copy link
Collaborator

bors commented Jun 21, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants