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

Disallow non-monomorphic calls to needs_drop in interpreter #85994

Merged
merged 2 commits into from
Jun 11, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jun 4, 2021

otherwise evaluation could change after further substitutions.

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 Jun 4, 2021
@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2021

Thanks, this makes sense.
Is there any reason not to do this for all eval_nullary_intrinsic?

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2021

Cc @oli-obk

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 4, 2021

Is there any reason not to do this for all eval_nullary_intrinsic?

I could be useful for optimizations to evaluate variant_count::<Option<T>>() or needs_drop::<ManuallyDrop<T>>() as soon as possible.

I do have a test case, but wasn't sure if we want one that depends on const_evaluatable_unchecked; following compiles on nightly:

#![feature(const_generics)]

struct Value<const B: bool> {}
impl Value<true> { fn is_true() {} }

fn f<T>() {
    Value::<{std::mem::needs_drop::<T>()}>::is_true();
}
fn main() {
    f::<u32>();
}

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2021

I could be useful for optimizations to evaluate variant_count::<Option>() or needs_drop::<ManuallyDrop>() as soon as possible.

Well, for the latter that won't happen any more with this PR, right?
If we want this, we need a needs_drop inside rustc that can answer "yes", "no", and "don't know".

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 4, 2021

Well, for the latter that won't happen any more with this PR, right?

For variant_count we already have suitable implementation, for needs_drop we would need to change it.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2021

Ah, fair.

Then maybe add comments to the other branches (the ones that do not call "ensure_monomorphic") explicitly saying "this code will correctly handle non-monomorphic calls" or so. I want to make sure that when/if another intrinsic is added, we don't introduce the same bug again...

@varkor
Copy link
Member

varkor commented Jun 5, 2021

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned varkor Jun 5, 2021
@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2021

Code change looks good to me; is there any way to have a test for this? r=me otherwise.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 5, 2021

Added a test case mentioned earlier.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2021

Awesome, thanks a lot. :)
@bors r+

@bors
Copy link
Contributor

bors commented Jun 5, 2021

📌 Commit 07a03b0 has been approved by RalfJung

@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 Jun 5, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 5, 2021
…alfJung

Disallow non-monomorphic calls to `needs_drop` in interpreter

otherwise evaluation could change after further substitutions.
@@ -0,0 +1,20 @@
error[E0599]: no function or associated item named `assert` found for struct `Bool<{ std::mem::needs_drop::<T>() }>` in the current scope
Copy link
Member

Choose a reason for hiding this comment

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

This error is useless - it would be good to suppress it. Is there already an issue for this kind of error?

@bors
Copy link
Contributor

bors commented Jun 11, 2021

⌛ Testing commit 07a03b0 with merge 66ba810...

@bors
Copy link
Contributor

bors commented Jun 11, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 66ba810 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2021
@bors bors merged commit 66ba810 into rust-lang:master Jun 11, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 11, 2021
@tmiasko tmiasko deleted the monomorphic-needs-drop branch June 11, 2021 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants