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: must review all .nll.stderr files #49862

Closed
pnkfelix opened this issue Apr 10, 2018 · 5 comments
Closed

NLL: must review all .nll.stderr files #49862

pnkfelix opened this issue Apr 10, 2018 · 5 comments
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-diagnostics Working torwads the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

#49861 checkpoints the state of NLL blindly. I made almost no attempt to check whether the content of each .nll.stderr file makes sense.

(I did sanity check a couple to just make sure that --compile-mode=nll seems like it is working, but even then I wouldn't call that skimming any sort of formal review.)

So, here's the issue: each instance of foo.nll.stderr represents a difference in the output from the AST-borrowck versus NLL borrowck.

Here are some reasons we might see different output:

  1. The input file might be an error under lexical lifetimes but be accepted by rustc under NLL. Such cases are marked by either an empty foo.nll.stderr or a #[rustc_error] diagnostic output (see compiletest: compare-mode cannot handle mixed success + failure #49855 for an explanation of that).
  • Sometimes such a case just means that the test should be removed or converted to a run-pass test.
  • But most often, preserving the spirit of the original test in this case will require revising it in some way. Much of the Rust test suite was written assuming lexical lifetimes and test authors took short-cuts making use of this, e.g. by just taking a mutable borrow and never using it. We often should just fix such tests by adding a use of the mutable borrow at a "correct" point in the source code.
  1. Both modes might reject the input, but NLL may emit a different set of error codes from AST borrowck. We need to review such cases to make sure the change in codes makes sense; and if it doesn't, then we should file a corresponding bug against NLL.
  2. Both modes might reject the input, with the same error codes, but NLL may have different spans or other details in its diagnostic output. We need to review such cases and determine if the change in output includes an unacceptable degradation in quality; if so, then we need to file a bug against NLL (or, quite possibly, note the particular test in a pre-existing bug, since we already know that NLL has a large number of cases where its diagnostic output is not yet up-to-snuff).

In this issue, I am going to make a list of all of the .nll.stderr files added in #49861. The idea is that people can then volunteer to review that stderr output under NLL mode, comparing it to the stderr output under AST borrowck, and figure out which case above the test falls into.

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 10, 2018

(I'm going to throw the list into a gist for the short term. I'm trying to figure out what the best format for a quest issue is; I might use #35233 as a model, but its possible that now that we're using Paper for so many tasks that maybe that is a better place to host a mutable table to hold that state.)

@pnkfelix
Copy link
Member Author

And of course we still need better instructions on what to actually do with each test before this will be a proper quest issue.

(In any case people shouldn't attempt to take tasks from the list of tests until #49861 has landed, since the whole point is to do this work relative to some checkpoint.)

@pnkfelix pnkfelix added A-NLL Area: Non Lexical Lifetimes (NLL) WG-compiler-nll labels Apr 10, 2018
@pnkfelix
Copy link
Member Author

(whatever the instructions are, they probably should involve actually running the compiler in NLL mode on the given source file. I am learning that its not a good idea to attempt to blindly interpret the output that you see in the stderr file(s). At least, I thought an empty .stderr was a sign of a successful compile when I was looking at #49860, but it was actually a sign of an ICE, at least in that case.)

@pnkfelix pnkfelix added the NLL-diagnostics Working torwads the "diagnostic parity" goal label Apr 17, 2018
@jkordish jkordish added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 8, 2018
@pnkfelix
Copy link
Member Author

Okay I've actually done the review.

See this paper document for now:
https://paper.dropbox.com/doc/NLL-stderr-diagnostic-deviations-KzroSvNgQ7lip3WQyiLTa

(I don't actually want that document to be too long-lived. I'm hoping we either address most of the items on the list quickly; if we fail to do that, then the table in that document should be transcribed elsewhere, perhaps to this issue.)

@nikomatsakis
Copy link
Contributor

Thanks to @pnkfelix's heroic efforts, this is done.

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) C-enhancement Category: An issue proposing an enhancement or a PR with one. 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