-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Create NormalizeTy query #44984
Create NormalizeTy query #44984
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I left a few minor nits.
src/librustc/infer/mod.rs
Outdated
} | ||
|
||
/// Fully normalizes any associated types in `value`, using an | ||
/// empty environment and `Reveal::All` mode (therefore, suitable | ||
/// only for monomorphized code during trans, basically). | ||
pub fn normalize_associated_type<T>(self, value: &T) -> T | ||
pub fn normalize_associated_type_in<T>(self, value: &T) -> T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we call this normalize_associated_types_in
(i.e., pluralize type
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the codebase and there already exists a function named this way, here. Would it be a problem if we had 2 different functions with the same name? (Mainly for people reading both of them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally only one of them would remain as the public API, if I'm not mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. So really these functions serve two distinct roles:
- The existing
normalize_associated_types_in
(implemented onInherited
) are used during type-checking, and hence may have a non-empty environment and/or inference variables. That particular one partly exists to silently register the extra obligations that comes from normalization. - The
normalize_associated_type
function in contrast is intended to be used after monomorphization, and it forces an empty environment andReveal::All
. In this mode, we don't expect extra obligations to matter to the user, so we handle them internally.
I think its pretty clear that the "naming convention" distinguishing the two is... not sufficient. </understatement>
In other parts of the code, we've used the trans_
prefix to distinguish the case of an empty environment with Reveal::All
-- or at least code that runs after type-checking. @eddyb doesn't like that for the -- quite correct! -- reason that the code is generally applicable beyond trans. For example, MIR optimizations may want to use these pathways (though those may not have an empty environment, so that's slightly different).
I think a lot of this comes down to whether you are running "before" or "after" typeck -- in particular, after typeck you know that errors should not occur, which can simplify the code and function signatures in various ways. So perhaps we can call this new function normalize_associated_types_after_typeck
? We could also rename the existing trans_
things in a similar vein.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh if one of them is only in rustc_typeck
then I got confused a bit, it certainly feels like the name shows up in a few places.
How about we just propagate the ParamEnv
in more places and just make sure that in rustc_trans
we give a Reveal::All
? That seems simpler and less headache-inducing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb the signature currently is T -> T
. I agree with changing it to (ParamEnv, T) -> T
but that is not enough to remove the need for two functions. In particular, there are also "auxiliary obligations" and we have to decide what to do with them. The FnCtxt
version ("during typeck") enrolls them into the fulfillment context. This version we are editing here solves them, assering that there are no errors (because it runs after type-check). The unifier form would probably propagate them back but nobody really wants that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, then, just take a Reveal
and don't put trans
in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb I don't see how that solves the problem. First off, taking a parameter environment -- not just reveal -- seems like the right thing to do (and is orthogonal from how we handle subobligations). Second, even if we just took a reveal, I still think we want a naming convention to distinguish these two cases ("expects to run after typeck" vs "does not").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option would be to use a combinator approach. Something like tcx.post_typeck().normalize_associated_types_in(...)
?
src/librustc/ty/maps/config.rs
Outdated
|
||
impl<'tcx> QueryDescription for queries::normalize_ty<'tcx> { | ||
fn describe(_tcx: TyCtxt, _: Ty) -> String { | ||
format!("normalising types") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "normalizing"
src/librustc/ty/maps/mod.rs
Outdated
|
||
fn normalize_ty_node<'tcx>(_: Ty<'tcx>) -> DepConstructor<'tcx> { | ||
DepConstructor::NormalizeTy | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: newline at end of file
src/librustc/ty/maps/config.rs
Outdated
fn describe(_tcx: TyCtxt, _: Ty) -> String { | ||
format!("normalising types") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: newline at end of file
☔ The latest upstream changes (presumably #44901) made this pull request unmergeable. Please resolve the merge conflicts. |
@Maaarcocr needs rebase =) |
@nikomatsakis Done 😄 were you able to understand why the error I'm experiencing is happening? |
@Maaarcocr yep you need to add the |
Also, before landing, we really do need to pick a name for this new function I think. I propose we use the |
@nikomatsakis I have used the |
☔ The latest upstream changes (presumably #44967) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #45261) made this pull request unmergeable. Please resolve the merge conflicts. |
@eddyb and I were talking on IRC about following the naming scheme of |
@nikomatsakis I don't have a strong preference about this. I will rebase the branch now. |
@Maaarcocr great! Let's go with the |
@Maaarcocr btw this still needs to be rebased again :( |
@nikomatsakis i had a problem while rebasing (with the llvm git submodule) and asked on the main impl period lobby on Gitter. Should I ask this kind of questions on IRC to have a higher chance to get an answer? |
@Maaarcocr looks like an unused import |
|
☔ The latest upstream changes (presumably #45381) made this pull request unmergeable. Please resolve the merge conflicts. |
…incremental test-suite.
@michaelwoerister thanks! It seems like now all tests are green 😄 @nikomatsakis are there any other issues that I need to solve before merging this? 😄 |
@Maaarcocr Excellent |
@bors r+ |
📌 Commit 5c51bf5 has been approved by |
⌛ Testing commit 5c51bf5 with merge 9ba208b9896b9df8bebebd6cb322fcf09927a08c... |
💔 Test failed - status-travis |
@bors retry macOS 3-hour timed out. |
Create NormalizeTy query As part of the effort to solve #44891, I've created the normalize_ty query. As outlined in the issue this meant: - renamed `normalize_associated_type()` to `normalize_associated_type_in()` - created the `normalize_ty` query - substituted the use of memoize with the query This PR is not ready. While running tests, one of the incremental ones failed. [This](https://pastebin.com/vGhH6bv6) is the error I got.
☀️ Test successful - status-appveyor, status-travis |
As part of the effort to solve #44891, I've created the normalize_ty query.
As outlined in the issue this meant:
normalize_associated_type()
tonormalize_associated_type_in()
normalize_ty
queryThis PR is not ready. While running tests, one of the incremental ones failed. This is the error I got.