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.

Sign up for GitHub

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

Jump to bottom

move leak-check to during coherence, candidate eval #295

Closed
2 of 4 tasks
nikomatsakis opened this issue May 22, 2020 · 2 comments
Closed
2 of 4 tasks

move leak-check to during coherence, candidate eval #295

nikomatsakis opened this issue May 22, 2020 · 2 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 22, 2020 •
edited by matthewjasper
Loading

What is this issue?

This is a major change proposal, which means a proposal to make a notable change to the compiler -- one that either alters the architecture of some component, affects a lot of people, or makes a small but noticeable public change (e.g., adding a compiler flag). You can read more about the MCP process on https://forge.rust-lang.org/.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

MCP Checklist

  • MCP filed. Automatically, as a result of filing this issue:
    • The @rust-lang/wg-prioritization group will add this to the triage meeting agenda so folks see it.
    • A Zulip topic in the stream #t-compiler/major changes will be created for this issue.
  • MCP seconded. The MCP is "seconded" when a compiler team member or contributor issues the @rustbot second command. This should only be done by someone knowledgable with the area -- before seconding, it may be a good idea to cc other stakeholders as well and get their opinion.
  • Final comment period (FCP). Once the MCP is approved, the FCP begins and lasts for 10 days. This is a time for other members to review and raise concerns -- concerns that should block acceptance should be noted as comments on the thread, ideally with a link to Zulip for further discussion.
  • MCP Accepted. At the end of the FCP, a compiler team lead will review the comments and discussion and decide whether to accept the MCP.
    • At this point, the major-change-accepted label is added and the issue is closed. You can link to it for future reference.

A note on stability. If your change is proposing a new stable feature, such as a -C flag, then a full team checkoff will be required before the feature can be landed. Often it is better to start with an unstable flag, like a -Z flag, and then move to stabilize as a secondary step.

TL;DR

  • Alter the leak-check to be more precise and to work by examining all placeholders created during a snapshot
  • Remove the leak check so it occurs only during candidate evaluation + coherence

Links and Details

Mentors or Reviewers
@nikomatsakis nikomatsakis added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels May 22, 2020
This was referenced May 22, 2020
Merged
Merged
@nikomatsakis nikomatsakis mentioned this issue May 23, 2020
Open
@matthewjasper
Copy link

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label May 24, 2020
@nikomatsakis
Copy link
Contributor Author

Closing, this is accepted.

@spastorino spastorino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Jun 8, 2020
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 23, 2020
@Manishearth
Rollup merge of rust-lang#72493 - nikomatsakis:move-leak-check, r=mat…
903823c 
…thewjasper

 move leak-check to during coherence, candidate eval

Implementation of MCP rust-lang/compiler-team#295.

I'd like to do a crater run on this.

Note to @rust-lang/lang: This PR is a breaking change (bugfix). It causes tests like the following to go from a future-compatibility warning rust-lang#56105 to a hard error:

```rust
trait Trait {}
impl Trait for for<'a, 'b> fn(&'a u32, &'b u32) {}
impl Trait for for<'c> fn(&'c u32, &'c u32) {} // now rejected, used to warn
```

I am not aware of any instances of this code in the wild, but that is why we are doing a crater run. The reason for this change is that those two types are, in fact, the same type, and hence the two impls are overlapping.

There will still be impls that trigger rust-lang#56105 after this lands, however -- I hope that we will eventually just accept those impls without warning, for the most part. One example of such an impl is this pattern, which is used by wasm-bindgen and other crates as well:

```rust
trait Trait {}
impl<T> Trait for fn(&T) { }
impl<T> Trait for fn(T) { } // still accepted, but warns
```
@QuineDot QuineDot mentioned this issue Aug 11, 2021
Closed
@QuineDot QuineDot mentioned this issue Apr 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@spastorino @nikomatsakis @matthewjasper @rustbot