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