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

Remove const_in_array_repeat #80404

Merged
merged 2 commits into from
Feb 1, 2021
Merged

Remove const_in_array_repeat #80404

merged 2 commits into from
Feb 1, 2021

Conversation

JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Dec 27, 2020

Fixes #80371. Fixes #81315. Fixes #80767. Fixes #75682.

I thought there might be some issue with Repeats(_, 0), but if you increase the items in the array it still ICEs. I'm not sure if this is the best fix but it does fix the given issue.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2020
@lcnr
Copy link
Contributor

lcnr commented Dec 27, 2020

this looks fine to me though I am kind of surprised that this error wasn't hit more frequently as borrowck uses mir_promoted. There seems to be another way to prevent borrowck from accessing promoteds or something I don't know about.

As I don't fully understand that I will leave the final review to someone else.

maybe r? @ecstatic-morse

@rust-highfive rust-highfive assigned ecstatic-morse and unassigned lcnr Dec 27, 2020
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2021
@camelid
Copy link
Member

camelid commented Jan 24, 2021

r? @oli-obk (ecstatic-morse is no longer doing reviews)

@camelid
Copy link
Member

camelid commented Jan 24, 2021

I marked this PR as also fixing #81315, #80767, and #75682.

@tmiasko
Copy link
Contributor

tmiasko commented Jan 25, 2021

I also looked at this issue before noticing that it has PR opened already.

As far as I understand, when originally written this code was working only with MIR before promotion so assertion was always true. Subsequently the code was reused to check whether const_in_array_repeat_expressions feature would successfully promote the operand used in repeat expression and if so to provide that as a suggestion to fix a compilation error. In the latter case the code is running on a MIR after promotion and the assertion is no longer true.

My first impression regarding fixing it was to simply return false for promoteds, since they haven't been disqualified earlier.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 25, 2021

cc @RalfJung we could also just delete the const_in_array_repeat_expressions feature to fix this issue 😆

@RalfJung
Copy link
Member

@oli-obk you mean together with this problem? ;)

Fine for me. The assertion here could still be helpful to catch accidentally created nested promoteds.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 25, 2021

The assertion here could still be helpful to catch accidentally created nested promoteds.

yea, I don't want to remove the assertion, so let's go with removing const_in_array_repeat_expressions.

@JulianKnodt
Copy link
Contributor Author

Should I change this PR or should I close this for someone else's implementation?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 26, 2021

Should I change this PR or should I close this for someone else's implementation?

If you have the time and motivation to do it, the job's all yours. Feel free to do it in this PR.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
   Compiling regex v1.4.3
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 9.98s
tidy check
tidy error: following path contains more than 2669 entries, you should move the test to some relevant subdirectory (current: 2671): /checkout/src/test/ui/issues
Found 435 error codes
Found 0 error codes with no tests
Done!
Done!
tidy error: The Unstable Book has a 'language feature' section 'const-in-array-repeat-expressions' which doesn't correspond to an unstable language feature
some tidy checks failed

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"
expected success, got: exit code: 1

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
   Compiling regex v1.4.3
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 9.28s
tidy check
tidy error: following path contains more than 2669 entries, you should move the test to some relevant subdirectory (current: 2671): /checkout/src/test/ui/issues
Found 435 error codes
Found 0 error codes with no tests
Done!
some tidy checks failed

@oli-obk
Copy link
Contributor

oli-obk commented Jan 27, 2021

you need to move your new test into a test subfolder.

it's also very likey that you need to bless some tests and change or remove some tests that use the const_in_array_repeat_expressions feature gate

@bors
Copy link
Contributor

bors commented Jan 30, 2021

📌 Commit bc653d60f640bb3ddfd905dba038ec8884026d52 has been approved by oli-obk

@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 Jan 30, 2021
@RalfJung
Copy link
Member

@bors r-
I left a comment

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 30, 2021
@RalfJung
Copy link
Member

Thanks. :)
@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jan 31, 2021

📌 Commit 6946534 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2021
This was referenced Jan 31, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2021
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#80092 (2229: Fix issues with move closures and mutability)
 - rust-lang#80404 (Remove const_in_array_repeat)
 - rust-lang#81255 (Don't link with --export-dynamic on wasm32-wasi)
 - rust-lang#81480 (Add suggestion for nested fields)
 - rust-lang#81549 (Misc ip documentation fixes)
 - rust-lang#81566 (Add a test for rust-lang#71202)
 - rust-lang#81568 (Fix an old FIXME in redundant paren lint)
 - rust-lang#81571 (Fix typo in E0759)
 - rust-lang#81572 (Edit multiple error code Markdown files)
 - rust-lang#81589 (Fix small typo in string.rs)
 - rust-lang#81590 (Stabilize int_bits_const)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 99f2f5a into rust-lang:master Feb 1, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 1, 2021
@JulianKnodt JulianKnodt deleted the arr_ref branch February 1, 2021 02:45
retrage added a commit to retrage/rust-hypervisor-firmware that referenced this pull request Feb 22, 2021
The `const_in_array_repeat_expressions` feature has been removed from
Rust [rust-lang/rust #80404](rust-lang/rust#80404).

Signed-off-by: Akira Moroo <retrage01@gmail.com>
retrage added a commit to retrage/rust-hypervisor-firmware that referenced this pull request Mar 5, 2021
The `const_in_array_repeat_expressions` feature has been removed from
Rust [rust-lang/rust #80404](rust-lang/rust#80404).

Signed-off-by: Akira Moroo <retrage01@gmail.com>
retrage added a commit to retrage/rust-hypervisor-firmware that referenced this pull request Mar 5, 2021
The `const_in_array_repeat_expressions` feature has been removed from
Rust [rust-lang/rust #80404](rust-lang/rust#80404).

Signed-off-by: Akira Moroo <retrage01@gmail.com>
rbradford pushed a commit to cloud-hypervisor/rust-hypervisor-firmware that referenced this pull request Mar 5, 2021
The `const_in_array_repeat_expressions` feature has been removed from
Rust [rust-lang/rust #80404](rust-lang/rust#80404).

Signed-off-by: Akira Moroo <retrage01@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet