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 Clause::ConstArgHasType #107965

Merged
merged 2 commits into from
Feb 17, 2023

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Feb 12, 2023

Currently the way that we check that a const arg has the correct type for the const param it is an argument for is by setting the expected type of typeck on the anon const of the argument to be the const param's type.

In the future for a potential min_generic_const_exprs we will allow providing const arguments that do not have an associated anon const that can be typeck'd which will require us to actually check that the const argument has the correct type. While it would potentially be possible to just call eq when creating substs this would not work if we support generics of the form const N: T, T (the const parameters type referencing generics declared after itself).

Additionally having ConstArgHasType will allow us to potentially make progress on removing the ty field of Const which may be desirable. Once progress has been made on this, ConstArgHasType will also be helpful in ensuring we do not make mistakes in trait/impl checking by declaring functions with the wrong const parameter types as the checks that the param env is compatible would catch it. (We have messed this up in the past, and with generic const parameter types these checks will get more complex)

There is a document about the types of const generics that may provide some general information on this subject


This PR shouldn't have any impact on whether code compiles or not on stable, it primarily exists to make progress on unstable const generics features that are desirable.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 12, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@BoxyUwU BoxyUwU force-pushed the add_const_arg_has_type_predicate branch from bef61ae to 68f6757 Compare February 12, 2023 20:04
@BoxyUwU BoxyUwU added the A-const-generics Area: const generics (parameters and arguments) label Feb 12, 2023
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Feb 12, 2023

@bors try @rust-timer queue

@bors
Copy link
Contributor

bors commented Feb 12, 2023

⌛ Trying commit 68f67579fdf91a0bdd108d513284bc0a9b6b0bdb with merge e660dc534275dfd10344eb9e5571f2003dc9001a...

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 12, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 12, 2023

☀️ Try build successful - checks-actions
Build commit: e660dc534275dfd10344eb9e5571f2003dc9001a (e660dc534275dfd10344eb9e5571f2003dc9001a)

1 similar comment
@bors
Copy link
Contributor

bors commented Feb 12, 2023

☀️ Try build successful - checks-actions
Build commit: e660dc534275dfd10344eb9e5571f2003dc9001a (e660dc534275dfd10344eb9e5571f2003dc9001a)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e660dc534275dfd10344eb9e5571f2003dc9001a): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.3% [0.3%, 0.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.9% [-4.3%, -0.2%] 6
All ❌✅ (primary) 0.3% [0.3%, 0.3%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.7% [2.7%, 2.7%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 12, 2023
@BoxyUwU BoxyUwU force-pushed the add_const_arg_has_type_predicate branch from 68f6757 to 76bf2e9 Compare February 12, 2023 23:44
BoxyUwU

This comment was marked as off-topic.

@BoxyUwU BoxyUwU force-pushed the add_const_arg_has_type_predicate branch from 76bf2e9 to 944a84e Compare February 13, 2023 16:33
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Feb 13, 2023

r? @compiler-errors although i have not yet had a chance to look over the diff so it probably is a little not great :<

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks @BoxyUwU

r=me after couple of nits

compiler/rustc_hir_analysis/src/collect/predicates_of.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/solve/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/wf.rs Outdated Show resolved Hide resolved
@BoxyUwU BoxyUwU force-pushed the add_const_arg_has_type_predicate branch from 944a84e to dda8def Compare February 16, 2023 12:02
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Feb 16, 2023

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Feb 16, 2023

📌 Commit dda8defca37ba66a11982511897835e559cd680b has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2023
@BoxyUwU BoxyUwU force-pushed the add_const_arg_has_type_predicate branch from dda8def to 90c8d6b Compare February 17, 2023 09:32
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Feb 17, 2023

@bors r=compiler-errors

CI failed because the PR making type_of return EarlyBinder merged and I added a call to type_of in this PR

@bors
Copy link
Contributor

bors commented Feb 17, 2023

📌 Commit 90c8d6b has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2023
@bors
Copy link
Contributor

bors commented Feb 17, 2023

⌛ Testing commit 90c8d6b with merge f4f5fc3...

@bors
Copy link
Contributor

bors commented Feb 17, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing f4f5fc3 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Feb 17, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing f4f5fc3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 17, 2023
@bors bors merged commit f4f5fc3 into rust-lang:master Feb 17, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 17, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f4f5fc3): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot removed the perf-regression Performance regression. label Feb 17, 2023
@rust-log-analyzer

This comment has been minimized.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2023
…, r=BoxyUwU

Suppress copy impl error when post-normalized type references errors

Suppress spurious errors from the `Copy` impl validity check when fields have bad types *post*-normalization, instead of just pre-normalization.

----

The const-generics test regressed recently due to rust-lang#107965, cc `@BoxyUwU.`
 * I think it's because `[_; 0u32]: Copy` now fails to hold because a nested obligation `ConstArgHasType(0u32, usize)` fails.
 * It's interesting that `[const_error]` shows up in the type only after normalization, though, but I'm pretty sure that it's due to the evaluate call that happens when normalizing unevaluated consts.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 8, 2023
…, r=BoxyUwU

Suppress copy impl error when post-normalized type references errors

Suppress spurious errors from the `Copy` impl validity check when fields have bad types *post*-normalization, instead of just pre-normalization.

----

The const-generics test regressed recently due to rust-lang#107965, cc ``@BoxyUwU.``
 * I think it's because `[_; 0u32]: Copy` now fails to hold because a nested obligation `ConstArgHasType(0u32, usize)` fails.
 * It's interesting that `[const_error]` shows up in the type only after normalization, though, but I'm pretty sure that it's due to the evaluate call that happens when normalizing unevaluated consts.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2023
…, r=BoxyUwU

Suppress copy impl error when post-normalized type references errors

Suppress spurious errors from the `Copy` impl validity check when fields have bad types *post*-normalization, instead of just pre-normalization.

----

The const-generics test regressed recently due to rust-lang#107965, cc ```@BoxyUwU.```
 * I think it's because `[_; 0u32]: Copy` now fails to hold because a nested obligation `ConstArgHasType(0u32, usize)` fails.
 * It's interesting that `[const_error]` shows up in the type only after normalization, though, but I'm pretty sure that it's due to the evaluate call that happens when normalizing unevaluated consts.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 8, 2023
…, r=BoxyUwU

Suppress copy impl error when post-normalized type references errors

Suppress spurious errors from the `Copy` impl validity check when fields have bad types *post*-normalization, instead of just pre-normalization.

----

The const-generics test regressed recently due to rust-lang#107965, cc ````@BoxyUwU.````
 * I think it's because `[_; 0u32]: Copy` now fails to hold because a nested obligation `ConstArgHasType(0u32, usize)` fails.
 * It's interesting that `[const_error]` shows up in the type only after normalization, though, but I'm pretty sure that it's due to the evaluate call that happens when normalizing unevaluated consts.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2023
…, r=BoxyUwU

Suppress copy impl error when post-normalized type references errors

Suppress spurious errors from the `Copy` impl validity check when fields have bad types *post*-normalization, instead of just pre-normalization.

----

The const-generics test regressed recently due to rust-lang#107965, cc `````@BoxyUwU.`````
 * I think it's because `[_; 0u32]: Copy` now fails to hold because a nested obligation `ConstArgHasType(0u32, usize)` fails.
 * It's interesting that `[const_error]` shows up in the type only after normalization, though, but I'm pretty sure that it's due to the evaluate call that happens when normalizing unevaluated consts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants