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

[WIP] Run the unused variables lint on the MIR #72164

Closed

Conversation

ecstatic-morse
Copy link
Contributor

Resolves #51003.

This removes the HIR liveness pass that is currently responsible for the unused variables lint. The idea is that this will reduce the overall difficulty of updating the HIR. Unfortunately, because we are moving this analysis further away from the AST, we need to do some tricks to map MIR locals back the variables they are tied to. One source of friction is bindings in match patterns, which are simultaneously associated with two distinct locals, one in the guard and one in the match arm body.

Due to a shortcoming of the incremental tests, I cannot add a HirId to VarBindingForm. As a result, we are storing way too much stuff in VarBindingForm, and properly handling multiple bindings with the same name (e.g., Ok(x) | Err(x)) will require more still. I'd like to fix the underlying issue and start storing HirIds in the VarBindingForm, but I think this will require changing the problematic tests? Until we can store a HirId on VarBindingForm, I likely won't prioritize full diagnostic parity with the HIR liveness pass, since my current approach seems clearly wrong.

I don't think it's clear that this approach will be easier to maintain than the HIR liveness analysis. I'm posting this draft so that people can weigh in on that question. If it is resolved in the affirmative, I'll continue working on this, but until then it's not ready for a detailed review.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 May 13, 2020
@ecstatic-morse
Copy link
Contributor Author

Forgot the r? @ghost. Sorry

@petrochenkov petrochenkov removed their assignment May 13, 2020
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented May 13, 2020

Some more TODOs for myself:

  • Add FakeRead::ForLetUnderscore to all places where we do no-op matches. For example, let () = expr;. Possibly change the name?
  • Investigate behavior for async fn parameters, which seem to always be used by the internal generator.

@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 3'
Agent machine name: 'fv-az578'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200430.2
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200430.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.1)
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/f5ea1143-b921-4e08-af2d-ce17dbf786f3.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72164/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/72164/merge:refs/remotes/pull/72164/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)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
   Compiling rustc_ast_passes v0.0.0 (/checkout/src/librustc_ast_passes)
   Compiling rustc_expand v0.0.0 (/checkout/src/librustc_expand)
   Compiling rustc_builtin_macros v0.0.0 (/checkout/src/librustc_builtin_macros)
   Compiling rustc_infer v0.0.0 (/checkout/src/librustc_infer)
   Compiling rustc_save_analysis v0.0.0 (/checkout/src/librustc_save_analysis)
   Compiling rustc_trait_selection v0.0.0 (/checkout/src/librustc_trait_selection)
error: variable `convert_to_ambiguous` is assigned to, but never used
   |
90 |         let convert_to_ambiguous;
   |             ^^^^^^^^^^^^^^^^^^^^
   |
   |
   = note: `-D unused-variables` implied by `-D warnings`
   = note: consider using `_convert_to_ambiguous` instead
error: aborting due to previous error

error: could not compile `rustc_trait_selection`.

---
  local time: Wed May 13 17:02:46 UTC 2020
  network time: Wed, 13 May 2020 17:02:46 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72164/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/72164/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3435) (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)

@bors
Copy link
Contributor

bors commented May 19, 2020

☔ The latest upstream changes (presumably #69171) made this pull request unmergeable. Please resolve the merge conflicts.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jun 11, 2020

This will require a lot more work, and I'm still not sure if it's the correct approach. I don't anticipate I'll get back to it anytime soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rewrite liveness analysis to be based on MIR
4 participants