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

Checking that function is const if marked with rustc_const_unstable #86838

Merged

Conversation

lambinoo
Copy link
Contributor

@lambinoo lambinoo commented Jul 3, 2021

Fixes #69630

This one is still missing tests to check the behavior but I checked by hand and it seemed to work.
I would not mind some direction for writing those unit tests!

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LeSeulArtichaut (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2021
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Here's a first quick review

compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jul 3, 2021

I would not mind some direction for writing those unit tests!

For the unit test, you'll probably want something like this:

#[crate_type = "lib"]
#![feature(staged_api)]
#![stable(feature = "foo", since = "1.0.0")]

#[stable(feature = "foo", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_foo", issue = "none")]
pub fn foo() {}
// ~^ ERROR attributes `#[rustc_const_unstable]` and `#[rustc_const_stable]` require the function to be marked `const`

// ... test other cases, like rustc_const_unstable on impl items

For more info on how to write tests for the compiler please read the rustc-dev-guide.

Also, please squash your " removing unwanted edit" commit with the first commit.

@lambinoo lambinoo force-pushed the I-69630-rust_const_unstable_check_const branch from 00ed975 to e7dfeb5 Compare July 3, 2021 13:32
@lambinoo
Copy link
Contributor Author

lambinoo commented Jul 3, 2021

I think I added support for methods and trait functions like you pointed out.
Just to be sure, to properly squashed, I had to rebase + force push, right?

@lambinoo lambinoo force-pushed the I-69630-rust_const_unstable_check_const branch 2 times, most recently from aafe09f to e6488e4 Compare July 3, 2021 13:48
@rust-log-analyzer

This comment has been minimized.

@lambinoo lambinoo force-pushed the I-69630-rust_const_unstable_check_const branch from e6488e4 to 3c49552 Compare July 3, 2021 17:15
@rust-log-analyzer

This comment has been minimized.

@lambinoo
Copy link
Contributor Author

lambinoo commented Jul 3, 2021

Apparently, implementing this checks make a bunch of other test fail. Should I go ahead and fix them at the same time in this PR?

@LeSeulArtichaut
Copy link
Contributor

A commit must pass all tests to be merged (that's what @bors does), so you have to fix the tests here.

@lambinoo
Copy link
Contributor Author

lambinoo commented Jul 3, 2021

I was more wondering if opening a secondary PR was necessary but I will do it with this one then!

@lambinoo lambinoo force-pushed the I-69630-rust_const_unstable_check_const branch from 9f609eb to 80203f4 Compare July 3, 2021 20:18
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

Okay, looks pretty good to me, but ultimately I don't really know much about const functions, so I'd prefer to pass this to someone else. Left a few nitpicks.

compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
@LeSeulArtichaut
Copy link
Contributor

r? @oli-obk

@lambinoo lambinoo force-pushed the I-69630-rust_const_unstable_check_const branch 3 times, most recently from 27d98c4 to f3f2def Compare July 4, 2021 20:02
@lambinoo lambinoo force-pushed the I-69630-rust_const_unstable_check_const branch 2 times, most recently from 710d1c2 to aadad14 Compare July 5, 2021 22:43
#[stable(feature = "potato", since = "1.0.0")]
pub struct Potato;

impl Potato {
Copy link
Contributor

Choose a reason for hiding this comment

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

please also add a test for the unstable impl const Trait for Type, the attribute works on the impl there, not on the methods. the method sigs also have no const modifier

Copy link
Contributor Author

@lambinoo lambinoo Jul 6, 2021

Choose a reason for hiding this comment

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

I think that's what is being tested on this one, no?

#[stable(feature = "potato", since = "1.27.0")]
impl const Default for Data {
#[rustc_const_unstable(feature = "data_foo", issue = "none")]
fn default() -> Data {
Data { _data: 42 }
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

apologies, I completely overlooked that

Copy link
Contributor Author

@lambinoo lambinoo Jul 6, 2021

Choose a reason for hiding this comment

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

No need to apologies! Thank you for giving me some of your time to check up on that MR!

On a side note, I'd like to get more involved as I've been a very happy user for a while. Any part of the compiler/libraries I should look at to be useful?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2021

📌 Commit aadad14 has been approved by oli-obk

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 6, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 7, 2021
…e_check_const, r=oli-obk

Checking that function is const if marked with rustc_const_unstable

Fixes rust-lang#69630

This one is still missing tests to check the behavior but I checked by hand and it seemed to work.
I would not mind some direction for writing those unit tests!
@JohnTitor
Copy link
Member

Failed in rollup: #86960 (comment)
@bors r-

@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 Jul 7, 2021
remove trailing newline

fix: test with attribute but missing const

Update compiler/rustc_passes/src/stability.rs

Co-authored-by: Léo Lanteri Thauvin <[email protected]>

Add test for extern functions

fix: using span_help instead of span_suggestion

add test for some ABIs + fmt fix

Update compiler/rustc_passes/src/stability.rs

Co-authored-by: Léo Lanteri Thauvin <[email protected]>

Refractor and add test for `impl const`

Add test to make sure no output + cleanup condition

-----------------------------

remove stdcall test, failing CI test

C abi is already tested in this, so it is not that useful to test another one.
The tested code is blind to which specific ABI for now, as long as it's not an intrinsic one
@lambinoo lambinoo force-pushed the I-69630-rust_const_unstable_check_const branch from aadad14 to 07f903e Compare July 8, 2021 05:52
@lambinoo
Copy link
Contributor Author

lambinoo commented Jul 8, 2021

Testing stdcall AND the C ABI was probably overzealous, as the tested code is blind to which ABI the function has, as long as it's not an intrinsic (for now at least).

@oli-obk
Copy link
Contributor

oli-obk commented Jul 8, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 8, 2021

📌 Commit 07f903e 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 Jul 8, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 8, 2021
…laumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#84961 (Rework SESSION_GLOBALS API)
 - rust-lang#86726 (Use diagnostic items instead of lang items for rfc2229 migrations)
 - rust-lang#86789 (Update BTreeSet::drain_filter documentation)
 - rust-lang#86838 (Checking that function is const if marked with rustc_const_unstable)
 - rust-lang#86903 (Fix small headers display)
 - rust-lang#86913 (Document rustdoc with `--document-private-items`)
 - rust-lang#86957 (Update .mailmap file)
 - rust-lang#86971 (mailmap: Add alternative addresses for myself)

Failed merges:

 - rust-lang#86869 (Account for capture kind in auto traits migration)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d85718a into rust-lang:master Jul 8, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 8, 2021
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
Development

Successfully merging this pull request may close these issues.

rustc_const_unstable should check constness
8 participants