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

Recover mut $pat and other improvements #63945

Merged
merged 5 commits into from
Aug 29, 2019
Merged

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 27, 2019

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2019
@Centril
Copy link
Contributor Author

Centril commented Aug 27, 2019

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned eddyb Aug 27, 2019
@Centril Centril changed the title Recover mut $pat and other improvements [WIP] Recover mut $pat and other improvements Aug 27, 2019
@Centril Centril changed the title [WIP] Recover mut $pat and other improvements Recover mut $pat and other improvements Aug 27, 2019
@Centril
Copy link
Contributor Author

Centril commented Aug 27, 2019

This turned out to add quite a bit of additional logic but I think it's a working implementation.
cc also @petrochenkov

AddMut.visit_pat(&mut pat);

// Unwrap; If we don't have `mut $ident`, error.
let pat = pat.into_inner();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could avoid this allocation by extracting a new function fn parse_pat_with_range_pat_kind. This would also avoid the need for the NtPat logic above but it would also not recover on let mut $p where $p:pat.

@@ -1,5 +1,5 @@
// This file was auto-generated using 'src/etc/generate-keyword-tests.py pub'

fn main() {
let pub = "foo"; //~ error: expected pattern, found keyword `pub`
let pub = "foo"; //~ error: expected identifier, found keyword `pub`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of these tests could be one file, don't you think? (not to be addressed in this PR necessarily)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want to file a follow up issue?

self.check_path()
// Just for recovery (see `can_be_ident`).
|| self.token.is_ident() && !self.token.is_bool_lit() && !self.token.is_keyword(kw::In)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this made me realize that we should have some check for "expected Fn() -> _, found bool and expr is foo\n|| bar" to detect a missing ; on the first pattern and returning a closure was intended 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a concrete example of a pitfall here? I'd like to avoid complicating things until someone reports a an issue.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like to handle the _ and Self cases as well to avoid being misleading (don't add or change the suggestion in those two cases). r=me

@Centril
Copy link
Contributor Author

Centril commented Aug 27, 2019

@estebank I made some more changes; can you have another quick look at the code?

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 27, 2019

📌 Commit 42e895d has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 29, 2019
Recover `mut $pat` and other improvements

- Recover on e.g. `mut Foo(x, y)` and suggest `Foo(mut x, mut y)`. Fixes rust-lang#63764.
- Recover on e.g. `let mut mut x;`
- Recover on e.g. `let keyword` and `let keyword(...)`.
- Cleanups in `token.rs` with `fn is_non_raw_ident_where` and friends.
bors added a commit that referenced this pull request Aug 29, 2019
Rollup of 11 pull requests

Successful merges:

 - #63811 (Correctly suggest adding bounds to `impl Trait` argument)
 - #63933 (Resolve some small issues related to #63580)
 - #63938 (or-pattern: fix typo in error message)
 - #63945 (Recover `mut $pat` and other improvements)
 - #63958 (const_prop: only call error_to_const_error if we are actually showing something)
 - #63961 (Add Option<Span> to `require_lang_item`)
 - #63963 (remove the reference to __cxa_thread_atexit_impl)
 - #63965 (Prevent syntax error in LD linker version script)
 - #63968 (rustc_apfloat: make the crate #![no_std] explicitly.)
 - #63970 (Notify me (flip1995) when Clippy toolstate changes)
 - #63980 (add missing `#[repr(C)]` on the Slices union)

Failed merges:

 - #63989 (Add Yaah to clippy toolstain notification list)

r? @ghost
@bors bors merged commit 42e895d into rust-lang:master Aug 29, 2019
@Centril Centril deleted the recover-mut-pat branch August 29, 2019 08:48
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 11, 2019
Pkgsrc changes:
 * Remove patch which no longer applies (but what about RPATH?)
 * Adapt a few patches to changed files upstream.

Upstream changes:

Version 1.39.0 (2019-11-07)
===========================

Language
--------
- [You can now create `async` functions and blocks with `async fn`,
  `async move {}`, and `async {}` respectively, and you can now call
  `.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
  pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
  `allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
  by procedural macro attributes applied to items. e.g.
  ```rust
  fn len(
      #[cfg(windows)] slice: &[u16],
      #[cfg(not(windows))] slice: &[u8],
  ) -> usize {
      slice.len()
  }
  ```
- [You can now take shared references to bind-by-move patterns in the
  `if` guards of `match` arms.][63118] e.g.
  ```rust
  fn main() {
      let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

      match array {
          nums
  //      ---- `nums` is bound by move.
              if nums.iter().sum::<u8>() == 10
  //                 ^------ `.iter()` implicitly takes a reference to `nums`.
          => {
              drop(nums);
  //          ----------- Legal as `nums` was bound by move and so we have ownership.
          }
          _ => unreachable!(),
      }
  }
  ```

Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
  [63402] **Note** Cargo currently doesn't use this feature. Refer to
  [cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
  output of successful tests.][62600]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
  now `const`.][63786]

Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]

Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
  [cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
  `--all` is now deprecated.

Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
  for compiling doctests.][63834]

Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
  the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
  previously a warning, and will also become a hard error in the Rust 2015
  edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
  run tests.][63827] This should improve performance when running a large
  amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
  recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
  returned `0.0`.

[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
Centril added a commit to Centril/rust that referenced this pull request Feb 18, 2020
…stebank

parse: recover `mut (x @ y)` as `(mut x @ mut y)`.

Follow up to rust-lang#68992 (comment) and rust-lang#63945.

Specifically, when given `let mut (x @ y)` we recover with `let (mut x @ mut y)` as the suggestion:

```rust
error: `mut` must be attached to each individual binding
  --> $DIR/mut-patterns.rs:12:9
   |
LL |     let mut (x @ y) = 0;
   |         ^^^^^^^^^^^ help: add `mut` to each binding: `(mut x @ mut y)`
   |
   = note: `mut` may be followed by `variable` and `variable @ pattern`
```

r? @matthewjasper @estebank
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom diagnostic for bad mut in pattern with sub-bindings
5 participants