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

nll: remove "warning: not reporting region error due to nll" #52768

Closed
pnkfelix opened this issue Jul 27, 2018 · 8 comments
Closed

nll: remove "warning: not reporting region error due to nll" #52768

pnkfelix opened this issue Jul 27, 2018 · 8 comments
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) NLL-diagnostics Working torwads the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@pnkfelix
Copy link
Member

As I understand it, our plan has always been to remove this warning eventually.

Certainly it cannot stay as it currently stands.

And from what I've seen, the diagnostics that we now report from MIR-borrowck tend to be good enough that the warning is just noise.

@pnkfelix pnkfelix added A-NLL Area: Non Lexical Lifetimes (NLL) WG-compiler-nll NLL-diagnostics Working torwads the "diagnostic parity" goal labels Jul 27, 2018
@pnkfelix
Copy link
Member Author

(in fact, taking care of this would bring a number of the .nll.stderr into 100% alignment with the corresponding .stderr file, which would reduce our burden for reviewing those.)

@pnkfelix
Copy link
Member Author

But on the other hand, my diagnostic review (#52663) is revealing a couple of cases where a diagnostic is missing useful info that we used to report, and I think they correspond to these region errors that we are not reporting.

@nikomatsakis nikomatsakis added this to the Rust 2018 RC milestone Aug 27, 2018
@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 27, 2018
@nikomatsakis
Copy link
Contributor

I'm bumping the priority on this and nominating it for discussion.

@pnkfelix
Copy link
Member Author

So the only issue that I'm aware of that we need to fix eventually in NLL is the diagnostic regression which I have now filed as #53771

@pnkfelix
Copy link
Member Author

but I also do not think that #53771 needs to be resolved for RC (see the end of the description for that issue for why).

Anyway, the question still remains: Are we ready to kill that "region error" warning?

@zilbuz
Copy link
Contributor

zilbuz commented Aug 29, 2018

If that's okay, I can prepare a PR with the warning removed and wait for the question to be resolved before merging it?

If the modification is just removing a warning, I think it should be fairly resilient to merge conflicts. Or is it more complicated than that?

@nikomatsakis
Copy link
Contributor

Anyway, the question still remains: Are we ready to kill that "region error" warning?

I think we are

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 3, 2018

Anyway, the question still remains: Are we ready to kill that "region error" warning?

I think we are

I agree and am going to r+ PR #53865

bors added a commit that referenced this issue Sep 3, 2018
Remove 'not reporting regions error due to nll' warning

Fix #52768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) NLL-diagnostics Working torwads the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants