eventual goal: re-remove leak-check from compiler #59490

pnkfelix · 2019-03-28T13:54:38Z

PR #58592 re-added the so-called leak check.

Re-adding the leak-check masked a number of bugs, where you need to use -Z no-leak-check to observe them again.

We plan to eventually re-remove the leak check. But before doing so, we need to double-check that all of the aforementioned masked bugs are either fixed, or at the very least revisited in terms of determining their priority in a leak-check-free world.

The text was updated successfully, but these errors were encountered:

vi · 2019-03-29T17:08:57Z

What is a leak check? How it masks the bugs? What Github label do those leak-check-masked bugs carry?

Google search does not seem to show relevant things; asking on IRC lead to answerer suggesting to ask here.

Do I understand the chain correctly:

fixing those bugs unlocks
removing re-added leak-check, which unlocks
Chalk integration, which unlocks
generic associated types, which unlocks
better abstraction abilities for Rust, which unlocks
better and safer APIs?

mark-i-m · 2019-03-29T18:12:31Z

@vi perhaps @pnkfelix can correct me if i am mistaken. I think they are referring to Placeholder leaks, which was the old way of handling HRTB lifetimes. My guess is that there is some difference in how Universes (the new way) work that caused edge case bugs or something....

pnkfelix · 2019-04-03T11:12:06Z

I think they are referring to Placeholder leaks, which was the old way of handling HRTB lifetimes.

Yes, this matches my own understanding.

The check that was re-added in PR #58592 has a big comment above its core function, here:

rust/src/librustc/infer/region_constraints/leak_check.rs

Lines 7 to 30 in 33d3598

    
           /// Searches region constraints created since `snapshot` that 
        
           /// affect one of the placeholders in `placeholder_map`, returning 
        
           /// an error if any of the placeholders are related to another 
        
           /// placeholder or would have to escape into some parent universe 
        
           /// that cannot name them. 
        
           /// 
        
           /// This is a temporary backwards compatibility measure to try and 
        
           /// retain the older (arguably incorrect) behavior of the 
        
           /// compiler. 
        
           /// 
        
           /// NB. The use of snapshot here is mostly an efficiency thing -- 
        
           /// we could search *all* region constraints, but that'd be a 
        
           /// bigger set and the data structures are not setup for that. If 
        
           /// we wind up keeping some form of this check long term, it would 
        
           /// probably be better to remove the snapshot parameter and to 
        
           /// refactor the constraint set. 
        
           pub fn leak_check( 
        
               &mut self, 
        
               tcx: TyCtxt<'_, '_, 'tcx>, 
        
               overly_polymorphic: bool, 
        
               placeholder_map: &PlaceholderMap<'tcx>, 
        
               _snapshot: &CombinedSnapshot<'_, 'tcx>, 
        
           ) -> RelateResult<'tcx, ()> { 
        
               debug!("leak_check(placeholders={:?})", placeholder_map);

I think the rustc guide mark-i-m linked is the best place for a new-comer to start; but I also do not know if the strategy outlined for handling leaks corresponds to our ideal-but-not-yet-the-default solution, or if the ideal solution is not yet documented there.

I am not an expert here. PR #58592 says the main reason to re-remove the leak check is that the leak-check does not handle higher-ranked types correctly in all cases.

What I do not know is whether the remaining buggy cases motivating removal of the leak check are all instances of expressiveness bugs (programs that we want to accept but currently reject), or if some of them are soundness issues. I think it is just an expressiveness issue, but I am not sure of that.

pnkfelix · 2019-04-03T11:20:08Z

What is a leak check? How it masks the bugs? What Github label do those leak-check-masked bugs carry?

I say that the leak check is "masking bugs" because it is causing the rustc compiler to reject programs relatively early (due to them violating the leak check), but those same programs, if we do get rid of the leak check, end up hitting some bug downstream that causes an internal compiler error.

Currently all such bugs that I know of are things that arise from interactions with non-lexical lifetimes, perhaps in part due to the MIR typeck pass that is in charge of checking the correctness of higher-ranked types (and without the leak check, a larger class of potential higher-ranked types ends up being fed into the MIR typeck pass).

It is possible that there are other bugs that belong in this category that are not due to interaction with non-lexical lifetime. I am just not yet aware of any such cases; it is an unknown quantity.

We have not allocated a github-label dedicated solely to these masked bugs (and I do not think that we should allocate such a label). I think it will suffice for such bugs to mention this isue (#59490) so that they end up being linked from this page.

nikomatsakis · 2020-03-26T22:03:36Z

So #65232 made good progress towards this goal by adding universes to the 'empty regions and thus ensuring that we don't have any region R1 where forall<R2> { R2: R1 } is true. However, this work did not extend to the NLL checker. Therefore, I think the major remaining work items are as follows:

extend the NLL check to avoid a notion of a 'empty value
resolve the confusing error messages that may result
integrate empty into the canonicalizer (see FIXME)
figure out how to resolve the wasm-bindgen regression

nikomatsakis · 2020-03-26T22:04:03Z

I'm looking some into that first point and will probably create a spin-out issue for it and put more details there.

@matthewjasper

…tthewjasper extend NLL checker to understand `'empty` combined with universes This PR extends the NLL region checker to understand `'empty` combined with universes. In particular, it means that the NLL region checker no longer considers `exists<R2> { forall<R1> { R1: R2 } }` to be provable. This is work towards rust-lang#59490, but we're not all the way there. One thing in particular it does not address is error messages. The modifications to the NLL region inference code turned out to be simpler than expected. The main change is to require that if `R1: R2` then `universe(R1) <= universe(R2)`. This constraint follows from the region lattice (shown below), because we assume then that `R2` is "at least" `empty(Universe(R2))`, and hence if `R1: R2` (i.e., `R1 >= R2` on the lattice) then `R1` must be in some universe that can name `'empty(Universe(R2))`, which requires that `Universe(R1) <= Universe(R2)`. ``` static ----------+-----...------+ (greatest) | | | early-bound and | | free regions | | | | | scope regions | | | | | empty(root) placeholder(U1) | | / | | / placeholder(Un) empty(U1) -- / | / ... / | / empty(Un) -------- (smallest) ``` I also made what turned out to be a somewhat unrelated change to add a special region to represent `'empty(U0)`, which we use (somewhat hackily) to indicate well-formedness checks in some parts of the compiler. This fixes rust-lang#68550. I did some investigation into fixing the error message situation. That's a bit trickier: the existing "nice region error" code around placeholders relies on having better error tracing than NLL currently provides, so that it knows (e.g.) that the constraint arose from applying a trait impl and things like that. I feel like I was hoping *not* to do such fine-grained tracing in NLL, and it seems like we...largely...got away with that. I'm not sure yet if we'll have to add more tracing information or if there is some sort of alternative. It's worth pointing out though that I've not kind of shifted my opinion on whose job it should be to enforce lifetimes: I tend to think we ought to be moving back towards *something like* the leak-check (just not the one we *had*). If we took that approach, it would actually resolve this aspect of the error message problem, because we would be resolving 'higher-ranked errors' in the trait solver itself, and hence we wouldn't have to thread as much causal information back to the region checker. I think it would also help us with removing the leak check while not breaking some of the existing crates out there. Regardless, I think it's worth landing this change, because it was relatively simple and it aligns the set of programs that NLL accepts with those that are accepted by the main region checker, and hence should at least *help* us in migration (though I guess we still also have to resolve the existing crates that rely on leak check for coherence). r? @matthewjasper

@matthewjasper

…tthewjasper extend NLL checker to understand `'empty` combined with universes This PR extends the NLL region checker to understand `'empty` combined with universes. In particular, it means that the NLL region checker no longer considers `exists<R2> { forall<R1> { R1: R2 } }` to be provable. This is work towards rust-lang#59490, but we're not all the way there. One thing in particular it does not address is error messages. The modifications to the NLL region inference code turned out to be simpler than expected. The main change is to require that if `R1: R2` then `universe(R1) <= universe(R2)`. This constraint follows from the region lattice (shown below), because we assume then that `R2` is "at least" `empty(Universe(R2))`, and hence if `R1: R2` (i.e., `R1 >= R2` on the lattice) then `R1` must be in some universe that can name `'empty(Universe(R2))`, which requires that `Universe(R1) <= Universe(R2)`. ``` static ----------+-----...------+ (greatest) | | | early-bound and | | free regions | | | | | scope regions | | | | | empty(root) placeholder(U1) | | / | | / placeholder(Un) empty(U1) -- / | / ... / | / empty(Un) -------- (smallest) ``` I also made what turned out to be a somewhat unrelated change to add a special region to represent `'empty(U0)`, which we use (somewhat hackily) to indicate well-formedness checks in some parts of the compiler. This fixes rust-lang#68550. I did some investigation into fixing the error message situation. That's a bit trickier: the existing "nice region error" code around placeholders relies on having better error tracing than NLL currently provides, so that it knows (e.g.) that the constraint arose from applying a trait impl and things like that. I feel like I was hoping *not* to do such fine-grained tracing in NLL, and it seems like we...largely...got away with that. I'm not sure yet if we'll have to add more tracing information or if there is some sort of alternative. It's worth pointing out though that I've not kind of shifted my opinion on whose job it should be to enforce lifetimes: I tend to think we ought to be moving back towards *something like* the leak-check (just not the one we *had*). If we took that approach, it would actually resolve this aspect of the error message problem, because we would be resolving 'higher-ranked errors' in the trait solver itself, and hence we wouldn't have to thread as much causal information back to the region checker. I think it would also help us with removing the leak check while not breaking some of the existing crates out there. Regardless, I think it's worth landing this change, because it was relatively simple and it aligns the set of programs that NLL accepts with those that are accepted by the main region checker, and hence should at least *help* us in migration (though I guess we still also have to resolve the existing crates that rely on leak check for coherence). r? @matthewjasper

lcnr · 2022-07-15T13:20:52Z

discussed in the t-types backlog bonanza

While the plan for how we want to remove the leak check has changed, we still intend to remove the current version, so we're keeping this open to track that for now.

The goal is to pretty much add a param_env aware leak check directly into trait solving and remove the current impl.

pnkfelix · 2022-08-12T14:06:26Z

Discussed in T-compiler backlog bonanza

@rustbot label: S-tracking-unimplemented

instantiate higher ranked goals outside of candidate selection, remove the `coherence_leak_check` future compat lint - rust-lang#59490 - rust-lang#56105 --- and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34. See https://hackmd.io/gstwC83ORaG7V29w54nMdA for more details about this change r? `@nikomatsakis`

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 28, 2019

This was referenced Mar 28, 2019

[NLL] Bad higher ranked subtype error #57374

Open

NLL accepts higher-ranked subtype that non-NLL rejects #57642

Closed

nikomatsakis mentioned this issue Mar 29, 2019

tracking issue for "chalkification" #48049

Closed

5 tasks

pnkfelix added the WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 label Apr 3, 2019

pnkfelix mentioned this issue Apr 4, 2019

Function pointer docs may need updating #46989

Open

lqd mentioned this issue Oct 9, 2019

replace the leak check with universes, take 2 #65232

Merged

nikomatsakis mentioned this issue Apr 9, 2020

extend NLL checker to understand 'empty combined with universes #70950

Merged

crlf0710 added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jun 10, 2020

nikomatsakis mentioned this issue Jun 11, 2020

A FunctionPointer trait to represent all fn types rust-lang/lang-team#23

Closed

ecstatic-morse mentioned this issue Jul 30, 2021

Check constraints arising from higher-rank trait bounds (HRTBs) rust-lang/polonius#172

Open

spastorino mentioned this issue Aug 6, 2021

Consolidate HKTBs, leak-check and universes information around with what we have in the guide rust-lang/rustc-dev-guide#1184

Open

lcnr added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 labels Jul 15, 2022

rustbot added the S-tracking-unimplemented Status: The feature has not been implemented. label Aug 12, 2022

lcnr mentioned this issue Jan 26, 2024

instantiate higher ranked goals outside of candidate selection #119820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eventual goal: re-remove leak-check from compiler #59490

eventual goal: re-remove leak-check from compiler #59490

pnkfelix commented Mar 28, 2019

vi commented Mar 29, 2019 •

edited

Loading

mark-i-m commented Mar 29, 2019

pnkfelix commented Apr 3, 2019

pnkfelix commented Apr 3, 2019 •

edited

Loading

nikomatsakis commented Mar 26, 2020 •

edited by lcnr

Loading

nikomatsakis commented Mar 26, 2020

lcnr commented Jul 15, 2022

pnkfelix commented Aug 12, 2022

eventual goal: re-remove leak-check from compiler #59490

eventual goal: re-remove leak-check from compiler #59490

Comments

pnkfelix commented Mar 28, 2019

vi commented Mar 29, 2019 • edited Loading

mark-i-m commented Mar 29, 2019

pnkfelix commented Apr 3, 2019

pnkfelix commented Apr 3, 2019 • edited Loading

nikomatsakis commented Mar 26, 2020 • edited by lcnr Loading

nikomatsakis commented Mar 26, 2020

lcnr commented Jul 15, 2022

pnkfelix commented Aug 12, 2022

vi commented Mar 29, 2019 •

edited

Loading

pnkfelix commented Apr 3, 2019 •

edited

Loading

nikomatsakis commented Mar 26, 2020 •

edited by lcnr

Loading