Const-to-pattern-to-MIR cleanup by RalfJung · Pull Request #127687 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation27 Commits5 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Now that all uses of constants without structural equality are hard errors, there's a bunch of cleanup we can do in the code that handles patterns: we can always funnel patterns through valtrees first (rather than having a fallback path for when valtree construction fails), and we can make sure that if we emit a PartialEq call it is not calling anything user-defined.

To keep the error messages the same, I made valtree construction failures return the information of which type it is that cannot be valtree'd. search_for_structural_match_violation is now not needed any more at all, so I removed it.

r? @oli-obk

Some changes occurred in exhaustiveness checking

cc @Nadrieril

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to the core trait solver

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

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in match checking

cc @Nadrieril

let c = ty::Const::new_unevaluated(
self.tcx,
ty::UnevaluatedConst { def: instance.def_id(), args: instance.args },
);

Choose a reason for hiding this comment

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

This code looks a bit odd... we create an instance only to immediately take it apart again. Can this be simplified further? We have def_id and args already before the let instance = ..., can we just use those directly? But there are some specific error cases up there that I don't know how to handle then, in particular AssocConstInPattern.

Choose a reason for hiding this comment

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

If I remove the Instance::try_resolve, then the only change in the tests is that "associated consts cannot be referenced in patterns" becomes "constant pattern depends on a generic parameter". Which seems okay? It does depend on a generic parameter:

pub fn test<A: Foo, B: Foo>(arg: EFoo, A::X: EFoo) { //~~^ ERROR associated consts cannot be referenced in patterns let A::X = arg; //~~^ ERROR associated consts cannot be referenced in patterns }

If the associated const can be resolved, I am fairly sure we already accept that code, so the general claim "associated consts cannot be referenced in patterns" is already not true I think.

Any objections to changing the error message for that case?

EDIT: I have now done this in the 4th commit.

Choose a reason for hiding this comment

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

I think that change is fine 👍

This comment has been minimized.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

let param_env_reveal_all = self.param_env.with_reveal_all_normalized(self.tcx);
// N.B. There is no guarantee that args collected in typeck results are fully normalized,
// so they need to be normalized in order to pass to `Instance::resolve`, which will ICE
// if given unnormalized types.

Choose a reason for hiding this comment

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

I was not able to reproduce any ICEs here with the new version that directly constructs an unevaluated const from the args. So I removed the normalization step. If that can cause ICEs I am sure matthiaskrgr's fuzzer will find them quickly, and then we'll have a test. ;)

This comment has been minimized.

rustbot added A-testsuite

Area: The testsuite used to check the correctness of rustc

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

labels

Jul 13, 2024

This comment has been minimized.

[u8; size_of::<Result<Option<ty::ValTree<'static>>, mir::interpret::ErrorHandled>>()];
impl EraseType for Result<Result<ty::ValTree<'_>, Ty<'_>>, mir::interpret::ErrorHandled> {
type Result = [u8; size_of::<
Result<Result<ty::ValTree<'static>, Ty<'static>>, mir::interpret::ErrorHandled>,

Choose a reason for hiding this comment

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

use the mir::interpret::EvalToValTreeResult type alias here 🤔

Choose a reason for hiding this comment

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

Is that a good idea? This seems like a very dangerous trait to implement, it just transmutes some lifetimes around, so maybe it'd be dangerous to just change the type alias without realizing that this also changes this here.

Unfortunately the EraseType trait has no comment at all, and to my great surprise it is a safe trait, so I really don't understand what happens here.


if self.saw_const_match_error.get().is_none() {

Choose a reason for hiding this comment

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

can this check inlined_const_as_pat.references_error() instead? I think we only set saw_const_match_error when also replacing that pattern with Pat::Error

Choose a reason for hiding this comment

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

I had no idea that function existed.^^ Yes, that's a great way to get rid of this annoying Cell.

Choose a reason for hiding this comment

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

Hm, it doesn't seem to work though. The error is shown now even if we already previously showed other errors, e.g.

error: to use a constant of type `MyType` in a pattern, `MyType` must be annotated with `#[derive(PartialEq)]`
  --> $DIR/const-partial_eq-fallback-ice.rs:14:12
   |
LL |     if let CONSTANT = &&MyType {
   |            ^^^^^^^^
   |
   = note: the traits must be derived, manual `impl`s are not sufficient
   = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details

error: to use a constant of type `&&MyType` in a pattern, the type must implement `PartialEq`
  --> $DIR/const-partial_eq-fallback-ice.rs:14:12
   |
LL |     if let CONSTANT = &&MyType {
   |            ^^^^^^^^

error: aborting due to 2 previous errors

Choose a reason for hiding this comment

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

that is very confusing to me and I can't see how that would happen 🤔 all the cases which previously set saw_const_match_error returns as PatKind::Error and afaict we never discard subpattern 🤔 could you revert the commit and test with assert_eq!(!inlined_const_as_pat.references_error(), saw_const_match_error.get().is_none(), "pat={inlined_const_as_pat:?}");

Choose a reason for hiding this comment

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

The problem is that TypeVisitable does not have a dedicated method for ErrorGuaranteed, so only errors in types, constants and lifetimes are actually found.

Choose a reason for hiding this comment

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

ah 🤔 should we add that method? it feels useful and is surprising otherwise 😊

Choose a reason for hiding this comment

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

really happy with these changes 😊 I haven't interacted with this area recently so I don't feel fully confident in signing this off myself, would like oli to do a final review

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 16, 2024

Make ErrorGuaranteed discoverable outside types, consts, and lifetimes

types like PatKind could contain ErrorGuaranteed, but not return them via tainted_by_errors or error_reported (see rust-lang#127687 (comment)). Now this happens, but it's a bit fragile as you can see with the TypeSuperVisitable for Ty impl.

We will catch any problems around Ty, Region or Const at runtime with an assert, and everything using derives will not have such issues, as it will just invoke the TypeVisitable for ErrorGuaranteed impl

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jul 16, 2024

…,nnethercote

Make ErrorGuaranteed discoverable outside types, consts, and lifetimes

Choose a reason for hiding this comment

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

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Jul 16, 2024

Rollup merge of rust-lang#127808 - oli-obk:tainting_visitors2, r=lcnr,nnethercote

Make ErrorGuaranteed discoverable outside types, consts, and lifetimes

#127808 landed and tests now pass locally, so all looking good. :)

@bors r=oli-obk,lcnr

📌 Commit 303a2db has been approved by oli-obk,lcnr

It is now in the queue for this repository.

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

Jul 18, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 18, 2024

Rollup of 7 pull requests

Successful merges:

rust-lang#127491 (Migrate 8 very similar FFI run-make tests to rmake)
rust-lang#127687 (Const-to-pattern-to-MIR cleanup)
rust-lang#127822 (Migrate issue-85401-static-mir, missing-crate-dependency and unstable-flag-required run-make tests to rmake)
rust-lang#127842 (Remove TrailingToken.)
rust-lang#127864 (cleanup: remove support for 3DNow! cpu features)
rust-lang#127899 (Mark myself as on leave)
rust-lang#127901 (Add missing GHA group for building llvm-bitcode-linker)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Jul 18, 2024

Rollup merge of rust-lang#127687 - RalfJung:pattern-cleanup, r=oli-obk,lcnr

Const-to-pattern-to-MIR cleanup

r? @oli-obk

Labels

A-testsuite

Area: The testsuite used to check the correctness of rustc

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

WG-trait-system-refactor

The Rustc Trait System Refactor Initiative (-Znext-solver)