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.

Sign up for GitHub

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

Jump to bottom

check that first arg to panic!() in const is &str #80734

Merged
merged 1 commit into from
Mar 2, 2021
Merged

check that first arg to panic!() in const is &str #80734

merged 1 commit into from
Mar 2, 2021

Conversation

abonander
Copy link
Contributor

@abonander abonander commented Jan 5, 2021 •
edited
Loading

closes #66693

TODO: regression test

cc @RalfJung for error message wording

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Jan 5, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
oli-obk
oli-obk reviewed Jan 5, 2021
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

does this affect any tests? If not, please add a test in a test that has the const_panic feature activated.

compiler/rustc_mir/src/transform/check_consts/validation.rs Outdated Show resolved Hide resolved
compiler/rustc_mir/src/transform/check_consts/validation.rs Outdated Show resolved Hide resolved
@abonander
Copy link
Contributor Author

does this affect any tests? If not, please add a test in a test that has the const_panic feature activated.

Yep, I was getting to that. I just wanted to post as draft for early feedback on the error message.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 5, 2021

Yep, I was getting to that. I just wanted to post as draft for early feedback on the error message.

oh duh, I should have read your main PR message properly..

@abonander abonander force-pushed the ab/issue-66693 branch 3 times, most recently from f437c33 to c6c1997 Compare January 5, 2021 22:25
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@abonander abonander force-pushed the ab/issue-66693 branch 3 times, most recently from 9511e21 to a5e15c7 Compare January 8, 2021 00:37
@abonander
Copy link
Contributor Author

@RalfJung @oli-obk I adjusted the wording, fixed the check and added UI tests. I noticed a discrepancy between panics in array length expressions and normal consts so I put them in separate files. Should the tests cover const fn as well?

@abonander abonander marked this pull request as ready for review January 8, 2021 00:40
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@abonander abonander force-pushed the ab/issue-66693 branch 2 times, most recently from 9bcdbad to aa12833 Compare January 8, 2021 03:23
@abonander
Copy link
Contributor Author

I went ahead and made sure to cover const fn as well.

@abonander abonander requested review from RalfJung and oli-obk January 8, 2021 03:25
@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2021

@RalfJung @oli-obk I adjusted the wording, fixed the check and added UI tests. I noticed a discrepancy between panics in array length expressions and normal consts so I put them in separate files. Should the tests cover const fn as well?

You can have more than one //~ ERROR in a single file, and that is usually preferred to group testcases.

@abonander
Copy link
Contributor Author

@RalfJung well the problem I had with arrays is that it seems their length expressions get const-evaluated in an earlier pass than const or static or const fn because the compiler would issue an error for let _ = [0i32; panic!(1)] and then exit before even visiting the other declarations. I assumed this was to be expected since arrays need to know their lengths for typechecking to finish, thus I separated the test cases into two files. Otherwise, I did combine test cases where I could.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2021

That's a good point, thanks. Maybe add a comment in the array-length-test-file explaining why this is a separate file.

@SergioBenitez
Copy link
Contributor

It seems like this is waiting on rather minor changes, is that right? @RalfJung I'm happy to push this forward if so.

@RalfJung
Copy link
Member

@SergioBenitez I left some comments that are all minor, yes (see the open discussions). I don't know if @oli-obk has further comments; he is more familiar with the const-checking code.

@abonander
Copy link
Contributor Author

abonander commented Feb 28, 2021 •
edited
Loading

@RalfJung sorry, been busy with work. I think that's your nits addressed now.

@JohnCSimon if you want to update the triage label. (nevermind, I got it)

@abonander
Copy link
Contributor Author

@rustbot label: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2021
@abonander abonander force-pushed the ab/issue-66693 branch 2 times, most recently from 9b4f2a0 to 8765a3d Compare February 28, 2021 19:12
oli-obk
oli-obk reviewed Mar 1, 2021
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Just a nit, then this lgtm

compiler/rustc_mir/src/transform/check_consts/validation.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Mar 1, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2021

📌 Commit 5a33f53 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 Mar 1, 2021
@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 1, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2021
@bors
Auto merge of rust-lang#82688 - GuillaumeGomez:rollup-b754t11, r=Guil…
ef0d592 
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#80734 (check that first arg to `panic!()` in const is `&str`)
 - rust-lang#81932 (Always compile rustdoc with debug logging enabled when `download-rustc` is set)
 - rust-lang#82018 (Remove the dummy cache in `DocContext`; delete RenderInfo)
 - rust-lang#82598 (Check stability and feature attributes in rustdoc)
 - rust-lang#82655 (Highlight identifier span instead of whole pattern span in `unused` lint)
 - rust-lang#82662 (Warn about unknown doc attributes)
 - rust-lang#82676 (Change twice used large const table to static)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 865cf0c into rust-lang:master Mar 2, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 2, 2021
@abonander abonander deleted the ab/issue-66693 branch March 2, 2021 17:16
@Alexendoo Alexendoo mentioned this pull request Mar 3, 2021
Closed
@camelid camelid added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Mar 14, 2021
@RalfJung RalfJung mentioned this pull request Jun 1, 2021
Closed
@tmandry tmandry mentioned this pull request Mar 4, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.

const_panic: ICE on non-&str panic payload
10 participants
@abonander @rust-highfive @rust-log-analyzer @oli-obk @RalfJung @JohnCSimon @SergioBenitez @bors @camelid @rustbot