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

Improve diagnostics for let x += 1 #71976

Merged
merged 5 commits into from
Jun 19, 2020
Merged

Conversation

mibac138
Copy link
Contributor

@mibac138 mibac138 commented May 7, 2020

Fixes(?) #66736

The code responsible for the E0404 errors is here which I don't think can be easily modified to prevent emitting an error in one specific case. Because of this I couldn't get rid of E0404 and instead added E0067 along with a help message which will fix the problem.

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2020
@akraines
Copy link

akraines commented May 7, 2020

Feel free to ignore the correction, I just noticed it when I was perusing the code and figured I may as well make my first mini contribution :-)

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.

Some minor nitpicks.

src/test/ui/parser/let-binop.stderr Outdated Show resolved Hide resolved
src/librustc_parse/parser/stmt.rs Outdated Show resolved Hide resolved
src/librustc_parse/parser/stmt.rs Outdated Show resolved Hide resolved
src/librustc_parse/parser/stmt.rs Outdated Show resolved Hide resolved
LL | b += 1;
| --

error[E0067]: can't reassign to a uninitialized variable
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer different wording here, but I'm not sure what :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the wording isn't ideal but I don't have any better ideas right now. The E0067 error description is: An invalid left-hand side expression was used on an assignment operation. however I think it's harder to understand. I might be biased though :)

src/librustc_parse/parser/stmt.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

This cannot be an error at parse time.
Syntactically, PATH + is a valid trait object type.

This should probably be combined with 870a7de and reported somewhere at resolution or HIR lowering time.

@estebank
Copy link
Contributor

estebank commented May 7, 2020

A trait object cannot be part of a pattern, can it?

@petrochenkov
Copy link
Contributor

@estebank
This PR changes let a: i8 += 1; as well, not only let a += 1;.

@petrochenkov
Copy link
Contributor

I would be ok if this PR only recovered op= as = and nothing else.
(Even then the cost/benefit looks pretty questionable.)

@mibac138
Copy link
Contributor Author

mibac138 commented May 7, 2020

only recovered op= as =

@petrochenkov do you mean leaving initialize the variable suggestion and removing reassign to a previously initialized variable suggestion?

@petrochenkov
Copy link
Contributor

@mibac138
I mean doing something like this petrochenkov@2209a09, not changing specific wording.

(Also if the recovery logic is larger than a few lines, then it's preferable to move it to diagnostics.rs.)

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2020
When parsing `let x: i8 += 1` the compiler interprets `i8` as a trait
which makes it more complicated to do error recovery. More advanced
error recovery is not implemented in this commit.
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-8 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
##[section]Starting: Linux x86_64-gnu-llvm-8
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 4'
Agent machine name: 'fv-az659'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200512.2
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200512.2/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.2)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/d5d426e5-d974-446c-8829-8ea1fb6dea55.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/71976/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/71976/merge:refs/remotes/pull/71976/merge
---
 ---> cb2676f08729
Step 5/8 : ENV RUST_CONFIGURE_ARGS       --build=x86_64-unknown-linux-gnu       --llvm-root=/usr/lib/llvm-8       --enable-llvm-link-shared       --set rust.thin-lto-import-instr-limit=10
 ---> Using cache
 ---> df25ce111862
Step 6/8 : ENV SCRIPT python2.7 ../x.py test --exclude src/tools/tidy &&            python2.7 ../x.py test src/test/mir-opt --pass=build                                   --target=armv5te-unknown-linux-gnueabi &&            python2.7 ../x.py test src/tools/tidy
 ---> 599b9ac96b27
Step 7/8 : ENV NO_DEBUG_ASSERTIONS=1
 ---> Using cache
 ---> 091087e35a36
---
   Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
   Compiling chalk-rust-ir v0.10.0
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling chalk-solve v0.10.0
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
   Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
   Compiling chalk-rust-ir v0.10.0
   Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
   Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
   Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
   Compiling chalk-solve v0.10.0
   Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
   Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
..........................................................................i......................... 1800/10203
.................................................................................................... 1900/10203
.............................................................................................i..i... 2000/10203
.................................................................................................... 2100/10203
...................................................................................iiiii............ 2200/10203
.................................................................................................... 2400/10203
.................................................................................................... 2500/10203
.................................................................................................... 2600/10203
.................................................................................................... 2700/10203
---
...............i.................................................................................... 5200/10203
.................................................................................................... 5300/10203
..............................................i..................................................... 5400/10203
.......................................i............................................................ 5500/10203
................................................ii.ii........i...i.................................. 5600/10203
..................................................................................................i. 5800/10203
.................................................................................................... 5900/10203
..................................................ii.....................................i.......... 6000/10203
.................................................................................................... 6100/10203
.................................................................................................... 6100/10203
.................................................................................................... 6200/10203
.................................................................................................... 6300/10203
...........ii...i..ii...........i................................................................... 6400/10203
.................................................................................................... 6600/10203
.................................................................................................... 6700/10203
.................................................................................................... 6700/10203
............................................i..ii................................................... 6800/10203
.................................................................................................... 7000/10203
..................................................................................................i. 7100/10203
.................................................................................................... 7200/10203
.................................................................................................F.. 7300/10203
---
.................................................................................................... 8100/10203
.................................................................................................... 8200/10203
.................................................................................................... 8300/10203
.......................i............................................................................ 8400/10203
.............................................................................iiiiii.iiiiii.i........ 8500/10203
................................i................................................................... 8700/10203
.................................................................................................... 8800/10203
.................................................................................................... 8900/10203
.................................................................................................... 9000/10203
---
error: /checkout/src/test/ui/parser/let-binop-plus.rs:4: expected error not found: can't reassign to an uninitialized variable

error: 0 unexpected errors found, 1 expected errors not found
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/parser/let-binop-plus.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/parser/let-binop-plus" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/parser/let-binop-plus/auxiliary"
    Error {
        line_num: 4,
        kind: Some(
            Error,
---

thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:348:22


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-8/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "8.0.0" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --exclude src/tools/tidy
Build completed unsuccessfully in 1:01:32
Build completed unsuccessfully in 1:01:32
== clock drift check ==
  local time: Wed May 20 21:38:21 UTC 2020
  network time: Wed, 20 May 2020 21:38:22 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/71976/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/71976/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3860) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2020
@crlf0710 crlf0710 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2020
@crlf0710 crlf0710 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2020
@Dylan-DPC-zz
Copy link

@estebank @petrochenkov this looks good for a merge?

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 16, 2020

📌 Commit 98532a3 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 Jun 16, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
Improve diagnostics for `let x += 1`

Fixes(?) rust-lang#66736

The code responsible for the `E0404` errors is [here](https://github.com/rust-lang/rust/blob/master/src/librustc_parse/parser/ty.rs#L399-L424) which I don't think can be easily modified to prevent emitting an error in one specific case. Because of this I couldn't get rid of `E0404` and instead added `E0067` along with a help message which will fix the problem.

r? @estebank
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 17, 2020
Improve diagnostics for `let x += 1`

Fixes(?) rust-lang#66736

The code responsible for the `E0404` errors is [here](https://github.com/rust-lang/rust/blob/master/src/librustc_parse/parser/ty.rs#L399-L424) which I don't think can be easily modified to prevent emitting an error in one specific case. Because of this I couldn't get rid of `E0404` and instead added `E0067` along with a help message which will fix the problem.

r? @estebank
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 17, 2020
Improve diagnostics for `let x += 1`

Fixes(?) rust-lang#66736

The code responsible for the `E0404` errors is [here](https://github.com/rust-lang/rust/blob/master/src/librustc_parse/parser/ty.rs#L399-L424) which I don't think can be easily modified to prevent emitting an error in one specific case. Because of this I couldn't get rid of `E0404` and instead added `E0067` along with a help message which will fix the problem.

r? @estebank
_ => self.eat(&token::Eq),
};

Ok(if eq_consumed || eq_optional { Some(self.parse_expr()?) } else { None })
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ok(if eq_consumed || eq_optional { Some(self.parse_expr()?) } else { None })
if eq_consumed || eq_optional { self.parse_expr().map(Some) } else { Ok(None) }

tmandry added a commit to tmandry/rust that referenced this pull request Jun 17, 2020
Improve diagnostics for `let x += 1`

Fixes(?) rust-lang#66736

The code responsible for the `E0404` errors is [here](https://github.com/rust-lang/rust/blob/master/src/librustc_parse/parser/ty.rs#L399-L424) which I don't think can be easily modified to prevent emitting an error in one specific case. Because of this I couldn't get rid of `E0404` and instead added `E0067` along with a help message which will fix the problem.

r? @estebank
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 18, 2020
Improve diagnostics for `let x += 1`

Fixes(?) rust-lang#66736

The code responsible for the `E0404` errors is [here](https://github.com/rust-lang/rust/blob/master/src/librustc_parse/parser/ty.rs#L399-L424) which I don't think can be easily modified to prevent emitting an error in one specific case. Because of this I couldn't get rid of `E0404` and instead added `E0067` along with a help message which will fix the problem.

r? @estebank
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2020
…arth

Rollup of 17 pull requests

Successful merges:

 - rust-lang#70551 (Make all uses of ty::Error delay a span bug)
 - rust-lang#71338 (Expand "recursive opaque type" diagnostic)
 - rust-lang#71976 (Improve diagnostics for `let x += 1`)
 - rust-lang#72279 (add raw_ref macros)
 - rust-lang#72628 (Add tests for 'impl Default for [T; N]')
 - rust-lang#72804 (Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position)
 - rust-lang#72814 (remove visit_terminator_kind from MIR visitor)
 - rust-lang#72836 (Complete the std::time documentation to warn about the inconsistencies between OS)
 - rust-lang#72968 (Only highlight doc search results via mouseover if mouse has moved)
 - rust-lang#73034 (Export `#[inline]` fns with extern indicators)
 - rust-lang#73315 (Clean up some weird command strings)
 - rust-lang#73320 (Make new type param suggestion more targetted)
 - rust-lang#73361 (Tweak "non-primitive cast" error)
 - rust-lang#73425 (Mention functions pointers in the documentation)
 - rust-lang#73428 (Fix typo in librustc_ast docs)
 - rust-lang#73447 (Improve document for `Result::as_deref(_mut)` methods)
 - rust-lang#73476 (Added tooltip for should_panic code examples)

Failed merges:

r? @ghost
@bors bors merged commit 39f8784 into rust-lang:master Jun 19, 2020
@mibac138 mibac138 deleted the let-recovery branch June 19, 2020 11:32
let _ = a;
let b += 1; //~ ERROR can't reassign to an uninitialized variable
let _ = b;
let c *= 1; //~ ERROR can't reassign to an uninitialized variable
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if it's already initialized?

let d = 1;
let d += 2;

Copy link
Contributor

Choose a reason for hiding this comment

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

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=e06e05de1651f41857588ffad51deaa8:

error: can't reassign to an uninitialized variable
 --> src/main.rs:3:7
  |
3 | let d += 2;
  |       ^^ help: initialize the variable

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but one could argue I just initialized it above (same name).

Copy link
Contributor

@pickfire pickfire Jul 1, 2020

Choose a reason for hiding this comment

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

Maybe change the wording for initialized variables? Can't reassign to initialized variable, it should only be either let d = 2 or d += 2, can we even use let with +=?

Copy link
Contributor

@Lonami Lonami Jul 1, 2020

Choose a reason for hiding this comment

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

+= cannot be used with let to my knowledge. Perhaps we could radically change the wording approach:

note: only `=` can be used in `let` bindings
help: change this `+=` for `=`
help: remove `let` (if it was initialized before)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 14, 2021
…avidtwco

Improve the wording for the `can't reassign` error

Follow-up for rust-lang#71976 (comment).
Fixes rust-lang#66736
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2021
…idtwco

Improve the wording for the `can't reassign` error

Follow-up for rust-lang#71976 (comment).
Fixes rust-lang#66736
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.