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

various ui tests need to be made robust w.r.t NLL migration #55533

Closed
pnkfelix opened this issue Oct 31, 2018 · 2 comments
Closed

various ui tests need to be made robust w.r.t NLL migration #55533

pnkfelix opened this issue Oct 31, 2018 · 2 comments

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Oct 31, 2018

While working on #54528 (see https://github.com/rust-lang/rust/projects/10) I found a number of tests that assume borrows to be lexically scoped.

These tests should be revised to not rely on that assumption.

(see also #52663 (comment) and https://github.com/rust-lang/rust/projects/10#column-3663841)

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 31, 2018

There's ancient beasts like this:
https://github.com/rust-lang/rust/blob/master/src/test/ui/borrowck/borrowck-lend-flow-loop.rs
which originally looked like this:
https://github.com/rust-lang/rust/blob/a896440ca1b93cc5dc6edd827ea2ae89602bfa9e/src/test/compile-fail/borrowck-lend-flow-loop.rs

the issue is that this test was written to deliberately test the lexical-ness of lifetimes. It isn't in the spirit of the test to add uses e.g. after loop. (At least, I don't think it is...)

It may be better in cases like this to use the // revisions system and then tag each error explicitly for ast vs nll, so that its clear in the test how the compiler's analysis is going to evolve.

@pnkfelix
Copy link
Member Author

pnkfelix commented Nov 2, 2018

Here is the list of tests that I derived from https://github.com/rust-lang/rust/projects/10#column-3663841 :

  • src/test/ui/borrowck/borrowck-lend-flow-loop.rs
  • src/test/ui/span/borrowck-borrow-overloaded-auto-deref-mut.rs
  • src/test/ui/issues/issue-17263.rs
  • src/test/ui/borrowck/borrowck-overloaded-index-move-index.rs
  • src/test/ui/borrowck/borrowck-reborrow-from-mut.rs
  • src/test/ui/borrowck/borrowck-unboxed-closures.rs
  • src/test/ui/borrowck/borrowck-overloaded-call.rs
  • src/test/ui/unop-move-semantics.rs
  • src/test/ui/issues/issue-52126-assign-op-invariance.rs
    • (was suggesting the experiment of removing the println! of the acc.map before +=. Seems reasonable include such a change.)
  • src/test/ui/binop/binop-move-semantics.rs
  • src/test/ui/codemap_tests/overlapping_spans.rs
  • src/test/ui/cannot-mutate-captured-non-mut-var.rs
    • This was miscategorized during the review. Not sure what I was thinking; AST-borrowck and NLL are both indeed testing the same thing.
  • src/test/ui/borrowck/borrowck-closures-mut-of-imm.rs
    • ui/borrowck/borrowck-closures-mut-of-imm.nll.stderr is supposed to be checking for error E0524: that two closures cannot simultaneously have mutable and immutable access to a variable. Issue Revamp borrowck treatment of closures #6801. Over time, it also started signaling two instances of E0596 (correctly). But NLL is now only signaling the two instances of E0596, and is not reporting E0524. We should check if we can revise the test to stop signalling E0596 while still checking for E0524.

kennytm added a commit to kennytm/rust that referenced this issue Nov 6, 2018
…s-wrt-nll, r=davidtwco

Update ui tests with respect to NLL

Fix rust-lang#55533
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant