Support const args in type dependent paths #71154
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
rust-highfive added the S-waiting-on-review 
| cc @eddyb |
eddyb reviewed Apr 14, 2020
| I've converted the PR to a draft, and I will not be able to review it too soon. |
lcnr force-pushed the type-dependent-consts branch from 705a722 to d4b7335 Compare 
| Now also supports struct A; impl A { fn foo<const N: usize>() -> usize { N + 1 } } fn main() { assert_eq!(A::foo::<7>(), 8); } |
lcnr force-pushed the type-dependent-consts branch 4 times, most recently from f9d4a7f to 0a12721 Compare 
| The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
lcnr force-pushed the type-dependent-consts branch from 282fd2f to 45cfb4c Compare | The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| ☔ The latest upstream changes (presumably #71049) made this pull request unmergeable. Please resolve the merge conflicts. |
lcnr force-pushed the type-dependent-consts branch 2 times, most recently from c003803 to 20bced3 Compare | The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
lcnr force-pushed the type-dependent-consts branch 5 times, most recently from 5f49c29 to 14358b1 Compare lcnr changed the title | Please keep it draft as per #71154 (comment). |
| The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| ☀️ Try build successful - checks-azure |
| Queued 2e0ee43e993a0a97edc6a0af1116bfbccdf1dd36 with parent a647c0c, future comparison URL. |
| Finished benchmarking try commit (2e0ee43e993a0a97edc6a0af1116bfbccdf1dd36): comparison url. |
lcnr force-pushed the type-dependent-consts branch from 6ae75a1 to 60dae6e Compare lcnr force-pushed the type-dependent-consts branch from 60dae6e to a23a444 Compare | The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| ☔ The latest upstream changes (presumably #73830) made this pull request unmergeable. Please resolve the merge conflicts. |
| Will open a new PR in the next few weeks as I have to rewrite most of this anyways |
| in the next few weeks i guess time is relative #74113 |
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2020
Support const args in type dependent paths (Take 2) once more, except it is sound this time 🥰 previously rust-lang#71154 ----- ```rust #![feature(const_generics)] struct A; impl A { fn foo<const N: usize>(&self) -> usize { N } } struct B; impl B { fn foo<const N: usize>(&self) -> usize { 42 } } fn main() { let a = A; a.foo::<7>(); } ``` When calling `type_of` for generic const arguments, we now use the `TypeckTables` of the surrounding body to get the expected type. This alone causes cycle errors though, as we now have `typeck_tables_of(main)` -> `...` -> `type_of(main_ANON0 := 7)` -> `typeck_tables_of(main)` ⚡ (see rust-lang#68400 (comment)) To prevent this we must not call `type_of(const_arg)` during `typeck_tables_of`. This is achieved by calling `type_of(param_def_id)` instead. We have to somehow remember the `DefId` of the param through all of typeck, which is done using the struct `ty::WithOptConstParam<DefId>`, which replaces `DefId` where needed and contains an `Option<DefId>` to be able to store the const parameter in case it exists. Queries which are currently cached on disk are split into two variants: `query_name`(cached) and `query_name_(of|for)_const_arg`(not cached), with `query_name_of_const_arg` taking a pair `(did, param_did): (LocalDefId, DefId)`. For some queries a method `query_name_of_opt_const_arg` is added to `TyCtxt` which takes a `ty::WithOptConstParam` and either calls `query_name` or `query_name_of_const_arg` depending on the value of `const_param_did`. r? @eddyb @varkor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
When calling
type_offor generic const arguments, we now use theTypeckTablesof the surrounding body to get the expected type.This alone causes cycle errors though, as we now have
typeck_tables_of(main)->...->type_of(main_ANON0 := 7)->typeck_tables_of(main)⚡ (see #68400 (comment))To prevent this we must not call
type_of(const_arg)duringtypeck_tables_of. This is achieved bycalling
type_of(param_def_id)instead. As thisDefIdcan't always be easily known, I changed someDefIds toty::WithOptParam<DefId>which contains the relevant param id. This also changes some queries which may be called during typeck.To prevent us from computing these queries twice,
WithOptParammust always use the correctDefIdof the parameter. This means that it is either constructed using the new methodtcx.with_opt_param(def_id)or manually if and only if a paramDefIdis available.To simplify
WithOptParamcreation, I changedIntoQueryParamfrom private topub(crate).I only use the existing definition of
LocalDefId -> DefId.const_param_ofrequires the HIR, meaning that it can only return the correct parameterDefIdwhile compiling the crate containing the const argument.To fix this,
const_param_ofis called for all const arguments during analysis. (I currently usepar_body_ownerhere, as all const arguments should be a body afaik)This PR may have missed some
type_of(const_arg)during typeck. While this is unfortunate, these cases should only introduce cycle errors and not lead to miscompilations.We should not have to worry about these cases while merging this IMO.
r? @varkor
fixes #70507, fixes #69816, fixes #63695, fixes #61936, fixes #71516