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

Stop using Reveal::All before borrowck #101478

Closed
wants to merge 13 commits into from
Closed

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 6, 2022

Using Reveal::All before borrowck is incorrect, both because that means that we peek into opaque types at a point which is still observable by the user, and more importantly, because peaking into opaque types causes query cycles if it happens this early.

I completely remove param_env.with_reveal_all_normalized. This function changes the context of a param env from "I am used for userfacing stuff" to "nm, actually I am not", which should always be incorrect or at least dubious enough to require an explicit impl with its own comment.

This is a breaking change in a few subtle ways, so it definitely needs a crater run. Still have to add some more tests where this is impactful.

r? @rust-lang/types @rust-lang/wg-const-eval

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 6, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2022
== "Sorts(ExpectedFound { expected: impl std::iter::Iterator<Item = ()>, found: std::iter::FlatMap<std::option::IntoIter<()>, std::vec::Vec<()>, [closure@src/main.rs:8:35: 8:38]> })"
{
bug!()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops, debug code 😁

@lcnr
Copy link
Contributor Author

lcnr commented Sep 6, 2022

@bors try

@bors
Copy link
Contributor

bors commented Sep 6, 2022

⌛ Trying commit 734d9d3 with merge 9b4fd9cf46c3c08bce1c5ed60b929af1c4aaed01...

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor Author

lcnr commented Sep 6, 2022

ctfe returing TooGeneric for opaque types in otherwise concrete constants causes ICE rn, have to fix this. changing to draft

@lcnr lcnr marked this pull request as draft September 6, 2022 11:33
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

span,
format!("Missing value for constant, but no error reported?"),
format!("unable to use constant with a hidden value in the type system"),
Copy link
Member

Choose a reason for hiding this comment

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

What's a "constant with a hidden value"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that error message is bad but i wasn't able to think of a better one.

This can be hit in two cases:

  • when encountering an opaque type during ctfe
  • when encountering a default associated item (with feature(specialization))

we should definitely change that message before merging this PR but I am not yet sure about the best way to do so

@lcnr
Copy link
Contributor Author

lcnr commented Sep 7, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 7, 2022
@bors
Copy link
Contributor

bors commented Sep 7, 2022

⌛ Trying commit 41066e2 with merge 0004f98fdd309b1940390200ae28913f7fbd736f...

@bors
Copy link
Contributor

bors commented Sep 7, 2022

☀️ Try build successful - checks-actions
Build commit: 0004f98fdd309b1940390200ae28913f7fbd736f (0004f98fdd309b1940390200ae28913f7fbd736f)

@rust-timer
Copy link
Collaborator

Queued 0004f98fdd309b1940390200ae28913f7fbd736f with parent 0568b0a, future comparison URL.

@lcnr lcnr marked this pull request as ready for review September 7, 2022 14:17
@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@lcnr
Copy link
Contributor Author

lcnr commented Sep 7, 2022

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-101478 created and queued.
🤖 Automatically detected try build 0004f98fdd309b1940390200ae28913f7fbd736f
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@lcnr
Copy link
Contributor Author

lcnr commented Sep 16, 2022

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Sep 16, 2022

@lcnr proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 16, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Sep 16, 2022

I lowered some of the changes to a future compat lint which should hopefully allow us to merge this PR quickly without figuring out our exact goals for opaque types.


This pull request changes the compiler to stop using Reveal::All at any point until borrowck is completed. Using Reveal::All means that it is possible to inspect

  • the underlying type of opaque types
  • the value of a "defaulted" associated item (requires feature(specialization))

In cases where this is known to break existing code, we retry the failing code with Reveal::All, emitting a future compatibility lint in case that succeeds. The cases where we emit a future compatibility lint are const evaluation for type system constants and transmute checking. We currently do not retry for patterns, this is a breaking change, even if not discovered using crater. See in-ctfe/match-arm-exhaustive.rs for an example. In case this causes an actual regression, we could extend the future compatibility lint to this area as well.

This PR is necessary as using Reveal::All before borrowck is incorrect and blocks work on implied bounds. Even if we want to fully embrace opaque types in these places - which is not trivial, see #101478 (comment), the approach then should not use Reveal::All. So even then, this PR will be "correct". I do not want to wait with my implied bounds work until this is figured out.

A crater run found 2 impacted crates:

mijit

Transmute in a test between an opaque type and the underlying concrete type. Opened PR fixing this in apt1002/mijit#51.

name-it

This crate cannot really be fixed to avoid the lint, as it fundamentally requires this pattern:

use std::mem;
fn returns_opaque() -> impl Sized {
    0u8
}

struct NamedOpaqueType {
    data: [mem::MaybeUninit<u8>; size_of_fut(returns_opaque)]
}

const fn size_of_fut<FUT>(x: fn() -> FUT) -> usize {
   mem::size_of::<FUT>()
}

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Sep 16, 2022

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 16, 2022
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
doc tests for: /checkout/src/doc/rustc/src/tests/index.md
doc tests for: /checkout/src/doc/rustc/src/what-is-rustc.md
 finished in 2.034 seconds
Generating lint docs (x86_64-unknown-linux-gnu)
error: failed to test example in lint docs for `hidden_type_of_opaque_types_in_type_system` in /checkout/compiler/rustc_lint_defs/src/builtin.rs:3985: lint docs should start with the text "The `hidden_type_of_opaque_types_in_type_system` lint" to introduce the lint
This error was generated by the lint-docs tool.
This tool extracts documentation for lints from the source code and places
This tool extracts documentation for lints from the source code and places
them in the rustc book. See the declare_lint! documentation
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint_defs/macro.declare_lint.html


To re-run these tests, run: ./x.py test --keep-stage=0 src/tools/lint-docs
The --keep-stage flag should be used if you have already built the compiler

Build completed unsuccessfully in 0:33:01

@bors
Copy link
Contributor

bors commented Sep 17, 2022

☔ The latest upstream changes (presumably #98588) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

This crate cannot really be fixed to avoid the lint, as it fundamentally requires this pattern:

Wasn't the plan to allow querying the size of an opaque type during CTFE? Or does that not work / lead to the same query cycles?

Similar to the second example of my previous comment: playground. With different variations you may end up getting ICE at some other places here. Generally you have non-wf type at places which can't deal with them.

I am trying to understand why this is non-wf. We have a usize: CDefault assumption so in that context, everything should be WF, right? We just must not try to normalize that instance, just like we would if this was a generic T: CDefault.

@lcnr
Copy link
Contributor Author

lcnr commented Sep 19, 2022

Wasn't the plan to allow querying the size of an opaque type during CTFE? Or does that not work / lead to the same query cycles?

Yes, but I am not comfortable with that change because it requires LayoutOf to work with non-wf types to be correct. My hope was that going with a future compat warning and explicitly stating "please show us your usecase, some of the code that's currently will be allowed without lints in the future" allows me to merge this PR without having to think about that because its blocking other work which is far more important to me.

I am trying to understand why this is non-wf. We have a usize: CDefault assumption so in that context, everything should be WF, right? We just must not try to normalize that instance, just like we would if this was a generic T: CDefault.

(talking about this example) Yeah, that should actually be fine, bad example 😅 this currently causes an ICE because Reveal::All erases global bounds which isn't really correct considering how it is used 😁

Only revealing the opaque type in the value - and not the param_env - is sound as long as there are no opaque types in the param_env, which is the case there. So as long as we use a conversion which doesn't drop trivial bounds, this should actually be alright 🤔

Doing that with opaque types in where-bounds does result in non-wf types like this previous example.

@RalfJung
Copy link
Member

RalfJung commented Sep 20, 2022

Doing that with opaque types in where-bounds does result in non-wf types like this previous example.

You'll have to spell this out in more detail for me, I don't see it.^^ (Also is there a better place to discuss this?)

In a context where Foo implements Trait, what is wrong with <Foo as Trait>::ASSOC? Sure that context might be inconsistent, but no more inconsistent than a context that has usize: CDefault (as in the first example) Box<_>: Copy or other where bounds that I can add to a function.

So as long as we use a conversion which doesn't drop trivial bounds, this should actually be alright thinking

I don't know what makes a bound trivial, but if that includes trivially false bounds like usize: CDefault then yes dropping them sounds like a big disaster to me -- it can turn a well-formed context into an ill-formed one. Dropping a truly trivial bound like usize: Copy should be fine though. (Speaking just from general type system intuition here without any knowledge about how this works in Rust.)

FWIW this does sound like the kind of conceptual question that I mentioned in our discussion a month or so ago, the kind of question that would benefit from some theory formalization and drafting.

@lcnr
Copy link
Contributor Author

lcnr commented Sep 20, 2022

I don't know what makes a bound trivial, but if that includes trivially false bounds like usize: CDefault then yes dropping them sounds like a big disaster to me

jup, trivially false bounds are the ones which are dangerous to drop. We currently drop all global ones I think, i.e. bounds which don't refer to any generic parameters 🤔

@lcnr
Copy link
Contributor Author

lcnr commented Sep 20, 2022

Also is there a better place to discuss this?

moved this to zulip: https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/.60Reveal.3A.3AAll.60.20borrowck.20yeet

@lcnr
Copy link
Contributor Author

lcnr commented Sep 29, 2022

talked about this with @nikomatsakis and @oli-obk in a recent meeting and we're still unhappy with the current solution. Going to experiment with a different approach where we lazily reveal opaque types on use and will open a new PR for that.

@lcnr lcnr closed this Sep 29, 2022
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 29, 2022
@jackh726 jackh726 removed the I-types-nominated Nominated for discussion during a types team meeting. label Oct 5, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
Always use RevealAll for const eval queries

implements what is described in rust-lang#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang#101478
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jan 21, 2024
Always use RevealAll for const eval queries

implements what is described in rust-lang/rust#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang/rust#101478
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
Always use RevealAll for const eval queries

implements what is described in rust-lang/rust#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang/rust#101478
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Always use RevealAll for const eval queries

implements what is described in rust-lang/rust#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang/rust#101478
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.