Support const args in type dependent paths#71154
Closed
lcnr wants to merge 14 commits intorust-lang:masterfrom
Closed
Support const args in type dependent paths#71154lcnr wants to merge 14 commits intorust-lang:masterfrom
lcnr wants to merge 14 commits intorust-lang:masterfrom
Conversation
Contributor
|
cc @eddyb |
eddyb
reviewed
Apr 14, 2020
Member
|
I've converted the PR to a draft, and I will not be able to review it too soon. |
705a722 to
d4b7335
Compare
Contributor
Author
|
Now also supports struct A;
impl A {
fn foo<const N: usize>() -> usize { N + 1 }
}
fn main() {
assert_eq!(A::foo::<7>(), 8);
} |
f9d4a7f to
0a12721
Compare
Contributor
|
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 |
282fd2f to
45cfb4c
Compare
Contributor
|
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 |
Collaborator
|
☔ The latest upstream changes (presumably #71049) made this pull request unmergeable. Please resolve the merge conflicts. |
c003803 to
20bced3
Compare
Contributor
|
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 |
5f49c29 to
14358b1
Compare
Member
|
Please keep it draft as per #71154 (comment). |
Contributor
|
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 |
Collaborator
|
Finished benchmarking try commit (0a204d6a1aef863bca037ca6e88ba49ce3e734ed): comparison url. |
varkor
reviewed
Jun 15, 2020
varkor
reviewed
Jun 15, 2020
src/librustc_middle/query/mod.rs
Outdated
Contributor
There was a problem hiding this comment.
Is this expected to be called for non-const-argument def_ids?
Contributor
Author
There was a problem hiding this comment.
Yes, made this more explicit in the query description.
varkor
reviewed
Jun 15, 2020
Contributor
varkor
left a comment
There was a problem hiding this comment.
I've had a skim through, and I don't have any major comments. I also think the perf results are acceptable, consider most differences are negligible, and this is necessary to move const generics forward to a point where it starts to become more usable. However, I don't want to sign off without @eddyb's approval: they may well have more comments, or ideas on how to improve the performance hit.
src/librustc_middle/query/mod.rs
Outdated
Contributor
There was a problem hiding this comment.
Could you expand on this comment a little, for a reader who may not have the full context?
Contributor
Author
There was a problem hiding this comment.
Folding DefIds is a noop anyways 😆 Kind of weird that we explicitly fold them here 🤔 We don't do this while folding TyKind
Contributor
There was a problem hiding this comment.
Could you add a comment explaining the general purpose of this function, with a small example?
Contributor
Author
There was a problem hiding this comment.
Added a comment referring back to the query tcx.const_param_of.
Contributor
|
I'm going to convert this to a proper pull request now that it seems to be ready for a final review. |
Contributor
|
@bors try @rust-timer queue |
Collaborator
|
Awaiting bors try build completion |
Collaborator
|
⌛ Trying commit 34648b9559268f8e7521c458162525f2cf95bb4b with merge 2e0ee43e993a0a97edc6a0af1116bfbccdf1dd36... |
Collaborator
|
☀️ Try build successful - checks-azure |
Collaborator
|
Queued 2e0ee43e993a0a97edc6a0af1116bfbccdf1dd36 with parent a647c0c, future comparison URL. |
Collaborator
|
Finished benchmarking try commit (2e0ee43e993a0a97edc6a0af1116bfbccdf1dd36): comparison url. |
Contributor
|
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 |
Collaborator
|
☔ The latest upstream changes (presumably #73830) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributor
Author
|
Will open a new PR in the next few weeks as I have to rewrite most of this anyways |
Contributor
Author
|
in the next few weeks i guess time is relative #74113 |
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
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