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

Expose Obligations created during type inference. #119613

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

gavinleroy
Copy link
Contributor

@gavinleroy gavinleroy commented Jan 5, 2024

This PR is a first pass at exposing the trait obligations generated and solved for during the type-check progress. Exposing these obligations allows for rustc plugins to use the public interface for proof trees (provided by the next gen trait solver).

The changes proposed track all obligations during the type-check process, this is desirable to not only look at the trees of failed obligations, but also those of successfully proved obligations. This feature is placed behind an unstable compiler option track-trait-obligations which should be used together with the next-solver option. I should note that the main interface is the function inspect_typeck made public in rustc_hir_typeck/src/lib.rs which allows the caller to provide a callback granting access to the FnCtxt.

r? @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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) labels Jan 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

Some changes occurred to the core trait solver

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

@rust-log-analyzer

This comment has been minimized.

@@ -334,6 +334,8 @@ pub struct InferCtxt<'tcx> {
pub intercrate: bool,

next_trait_solver: bool,

pub fulfilled_obligations: Lrc<Lock<Vec<traits::FulfilledObligation<'tcx>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this to only be a field of the fulfillment context (and a method on the Engine trait to get them. Do not add it to the inference context. That will miss some obligations checked while probing, but given that we also sometimes prove by just using evaluate, this shouldn't change

However, having it on the infcx stores obligations from probes, which can leak inference variables, which is bad:tm: and potentially results in ICE

Copy link
Contributor Author

@gavinleroy gavinleroy Jan 10, 2024

Choose a reason for hiding this comment

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

Leaking inference variables is obviously not good. However, there is a motivation for catching obligations from within a probe. Take the following code:

#![feature(negative_impls)]

use rayon::prelude::*;

#[derive(Debug)]
struct NotSend;

impl !Send for NotSend {}
impl !Sync for NotSend {}

fn parallel_do(vs: Vec<NotSend>) {
    // Obligation `Vec<NotSend>: IntoParallelIterator` 
    // happens inside a probe. (At least IIUC)
    vs.into_par_iter();
}

fn main() {}

If my understanding of the trait solver is correct, when type-checking the expression vs.into_par_iter() the query Vec<NotSend>: IntoParallelIterator would happen inside of a probe. For the context of the "trait debugger" we're building, catching this obligation is important so that we can explain the call ambiguity to a user.

Is there a way to get this obligation without storing the obligations on the InferCtxt?

@bors
Copy link
Contributor

bors commented Jan 14, 2024

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

@gavinleroy
Copy link
Contributor Author

Following a suggestion on Zulip, the inference context is cloned and stored alongside the captured obligations. However, I'm not sure I'm satisfied with this approach.

  1. The InferCtxt must now implement Clone, which I see as undesirable, as nowhere else in the compiler should you clone the context.
  2. The obligations still need to be stored in the InferCtxt as opposed to the TyCtxt. My understanding is because rustc_middle cannot depend on rustc_infer.

Here's an alternative proposal. Instead of using the callback on inspect_typeck to inspect the FnCtxt, we could give the the callback to the InferCtxt. When obligation tracking is enabled this callback is invoked with the InferCtxt on all root obligations in the FulfillmentCtxt. Thus making the callback of type impl Fn(infcx: &InferCtxt<'tcx>, obligation: PredicateObligation<'tcx>). No cloning necessary, and external tools should be able to get all information necessary ™️. I can of course test this claim with our tool locally.

@lcnr do you have thoughts on this approach vs what was discussed the other day?

@lcnr
Copy link
Contributor

lcnr commented Jan 16, 2024

  1. The InferCtxt must now implement Clone, which I see as undesirable, as nowhere else in the compiler should you clone the context.

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxt.html#method.fork exists and should be used instead

  1. The obligations still need to be stored in the InferCtxt as opposed to the TyCtxt. My understanding is because rustc_middle cannot depend on rustc_infer.

the type could be erased on the TyCtxt, however, I think your suggested alternative is to never store the goals anywhere but eagerly call the supplied closure? This seems preferable to me

Comment on lines 55 to 61
if let Some(inspector) = infcx.obligation_inspector.get() {
let result = match result {
Ok((_, c, _)) => Ok(*c),
Err(NoSolution) => Err(NoSolution),
};
(inspector)(infcx, &obligation, result);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't the Some(inspector) check enough 🤔 why did you add a separate flag here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In it's current state, it's technically possible to have a Some when the track-trait-obligation flag isn't set. I'll change it to check the flag before setting the option.

Copy link
Contributor

Choose a reason for hiding this comment

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

my quesiton would be: why have the track-trait-obligation flag in the first place? It seems like always using an inspector, if one is provided, should be good enough 🤔 are you worried about accidentally relying on the behavior of an inspector in ordinary compilations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. The motivation to have the unstable flag was to make the changes seem less intrusive :) especially because using the inspector relies on using the next trait solver, which is behind a flag. But functionally there's no reason to have the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the unstable flag and added a comment to inspect_typeck which states that it only has an effect when using the next trait solver. That should be clear enough for its use. I'm not particularly worried that someone will rely on its behavior during normal compilation.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

r=me after nits

.collect()
.collect();

errors
Copy link
Contributor

Choose a reason for hiding this comment

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

spurious change, please revert

}

#[instrument(level = "debug", skip(tcx, fallback), ret)]
/// Same as `typeck` but `inspect` is invoked on evaluation of each root obligation.
/// Inspecting obligations only works with the new trait solver.
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 state that:

  • only used by external tools
  • not cached as it's not a query

…tc_infer using callback.

Pass each obligation to an fn callback with its respective inference context. This avoids needing to keep around copies of obligations or inference contexts.

Specify usability of inspect_typeck in comment.
@gavinleroy
Copy link
Contributor Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Jan 19, 2024

@gavinleroy: 🔑 Insufficient privileges: Not in reviewers

@lcnr
Copy link
Contributor

lcnr commented Jan 19, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 19, 2024

📌 Commit 130b7e7 has been approved by lcnr

It is now in the queue for this repository.

@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 Jan 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#103730 (Added NonZeroXxx::from_mut(_unchecked)?)
 - rust-lang#113142 (optimize EscapeAscii's Display  and CStr's Debug)
 - rust-lang#118799 (Stabilize single-field offset_of)
 - rust-lang#119613 (Expose Obligations created during type inference.)
 - rust-lang#119752 (Avoid ICEs in trait names without `dyn`)
 - rust-lang#120132 (Teach tidy about line/col information for malformed features)
 - rust-lang#120135 (SMIR: Make the remaining "private" fields actually private)
 - rust-lang#120148 (`single_use_lifetimes`: Don't suggest deleting lifetimes with bounds)
 - rust-lang#120150 (Stabilize `round_ties_even`)
 - rust-lang#120155 (Don't use `ReErased` to detect type test promotion failed)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#103730 (Added NonZeroXxx::from_mut(_unchecked)?)
 - rust-lang#113142 (optimize EscapeAscii's Display  and CStr's Debug)
 - rust-lang#118799 (Stabilize single-field offset_of)
 - rust-lang#119613 (Expose Obligations created during type inference.)
 - rust-lang#119752 (Avoid ICEs in trait names without `dyn`)
 - rust-lang#120132 (Teach tidy about line/col information for malformed features)
 - rust-lang#120135 (SMIR: Make the remaining "private" fields actually private)
 - rust-lang#120148 (`single_use_lifetimes`: Don't suggest deleting lifetimes with bounds)
 - rust-lang#120150 (Stabilize `round_ties_even`)
 - rust-lang#120155 (Don't use `ReErased` to detect type test promotion failed)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2de5ca2 into rust-lang:master Jan 20, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
Rollup merge of rust-lang#119613 - gavinleroy:expose-obligations, r=lcnr

Expose Obligations created during type inference.

This PR is a first pass at exposing the trait obligations generated and solved for during the type-check progress. Exposing these obligations allows for rustc plugins to use the public interface for proof trees (provided by the next gen trait solver).

The changes proposed track *all* obligations during the type-check process, this is desirable to not only look at the trees of failed obligations, but also those of successfully proved obligations. This feature is placed behind an unstable compiler option `track-trait-obligations` which should be used together with the `next-solver` option. I should note that the main interface is the function `inspect_typeck` made public in `rustc_hir_typeck/src/lib.rs` which allows the caller to provide a callback granting access to the `FnCtxt`.

r? `@lcnr`
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. 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants