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

submodules: update clippy from 3710ec59 to ad3269c4 #60819

Merged
merged 1 commit into from
May 14, 2019

Conversation

matthiaskrgr
Copy link
Member

Changes:

Rustfmt all the things
Clippy dogfood
Update for compiletest changes
Use symbols instead of strings
Rustup to rustc 1.36.0-nightly (1764b2972 2019-05-12)
Add regression test for identity_conversion FP
UI test cleanup: Extract many_single_char_names tests
Add tests for empty_loop lint
Add in_macro again
Rename in_macro to in_macro_or_desugar

r? @oli-obk

Changes:
````
Rustfmt all the things
Clippy dogfood
Update for compiletest changes
Use symbols instead of strings
Rustup to rustc 1.36.0-nightly (1764b29 2019-05-12)
Add regression test for identity_conversion FP
UI test cleanup: Extract many_single_char_names tests
Add tests for empty_loop lint
Add in_macro again
Rename in_macro to in_macro_or_desugar
````
@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2019
@oli-obk
Copy link
Contributor

oli-obk commented May 14, 2019

@bors r+ p=1

@bors
Copy link
Contributor

bors commented May 14, 2019

📌 Commit 41184aa has been approved by oli-obk

@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 May 14, 2019
Centril added a commit to Centril/rust that referenced this pull request May 14, 2019
submodules: update clippy from 3710ec5 to ad3269c

Changes:
````
Rustfmt all the things
Clippy dogfood
Update for compiletest changes
Use symbols instead of strings
Rustup to rustc 1.36.0-nightly (1764b29 2019-05-12)
Add regression test for identity_conversion FP
UI test cleanup: Extract many_single_char_names tests
Add tests for empty_loop lint
Add in_macro again
Rename in_macro to in_macro_or_desugar
````
r? @oli-obk
bors added a commit that referenced this pull request May 14, 2019
Rollup of 9 pull requests

Successful merges:

 - #60130 (Add implementations of last in terms of next_back on a bunch of DoubleEndedIterators)
 - #60443 (as_ptr returns a read-only pointer)
 - #60444 (forego caching for all participants in cycles, apart from root node)
 - #60719 (Allow subdirectories to be tested by x.py test)
 - #60780 (fix Miri)
 - #60788 (default to $ARCH-apple-macosx10.7.0 LLVM triple for darwin targets)
 - #60799 (Allow late-bound regions in existential types)
 - #60808 (Improve the "must use" lint for `Future`)
 - #60819 (submodules: update clippy from 3710ec5 to ad3269c)

Failed merges:

r? @ghost
@ehuss
Copy link
Contributor

ehuss commented May 14, 2019

After this PR, rls's tests seem to fail with an ICE in rustc:

backtrace
thread 'rustc' panicked at 'index out of bounds: the len is 2 but the index is 4294954756', /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/libcore/slice/mod.rs:2689:10
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::_print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: rustc::util::common::panic_hook
   5: std::panicking::rust_panic_with_hook
   6: std::panicking::continue_panic_fmt
   7: rust_begin_unwind
   8: core::panicking::panic_fmt
   9: core::panicking::panic_bounds_check
  10: scoped_tls::ScopedKey::with
  11: syntax_pos::symbol::Symbol::as_str
  12: clippy_lints::utils::match_def_path::{{closure}}
             at /Users/eric/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/ad3269c/clippy_lints/src/utils/mod.rs:1122
  13: ::eq
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/libcore/iter/adapters/mod.rs:589
  14: clippy_lints::utils::conf::helpers::doc_valid_idents::{{closure}}
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/libcore/slice/mod.rs:3153
  15: ::eq
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/libcore/iter/adapters/mod.rs:589
  16: std::sys_common::poison::Flag::borrow
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/libcore/iter/traits/iterator.rs:604
  17: ::drop
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/liballoc/vec.rs:1862
  18: ::drop
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/liballoc/vec.rs:1845
  19: ::drop
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/liballoc/vec.rs:1731
  20: std::sys_common::poison::Flag::borrow
             at /rustc/a9ec99f4201ec33026a468ef1289f98a95b4d71a/src/libcore/iter/traits/iterator.rs:1465
  21: clippy_lints::utils::match_def_path
             at /Users/eric/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/ad3269c/clippy_lints/src/utils/mod.rs:1122
  22: ::check_block_post::{{closure}}
             at /Users/eric/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/ad3269c/clippy_lints/src/regex.rs:116
  23: ::check_expr
  24:  as rustc::hir::intravisit::Visitor>::visit_expr
  25: rustc::hir::intravisit::walk_expr
  26:  as rustc::hir::intravisit::Visitor>::visit_expr
  27: rustc::hir::intravisit::walk_fn
  28: rustc::hir::intravisit::walk_item
  29: rustc::hir::intravisit::Visitor::visit_nested_item
  30: rustc::hir::intravisit::walk_crate
  31: rustc::lint::context::late_lint_pass_crate
  32: rustc::lint::context::late_lint_crate
  33: rustc::util::common::time
  34: rustc::util::common::time
  35: __rust_maybe_catch_panic
  36:  as core::ops::function::FnOnce<()>>::call_once
  37: __rust_maybe_catch_panic
  38: rustc_interface::passes::analysis::{{closure}}
  39: rustc::util::common::time
  40: rustc_interface::passes::analysis
  41: rustc::ty::query::__query_compute::analysis
  42: rustc::ty::query::::compute
  43: rustc::dep_graph::graph::DepGraph::with_task_impl
  44: rustc::ty::query::plumbing::::get_query
  45: rustc::ty::context::tls::enter_global
  46: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}
  47: rustc_interface::passes::create_global_ctxt::{{closure}}
  48: rustc_interface::interface::run_compiler_in_existing_thread_pool
  49: std::thread::local::LocalKey::with
  50: scoped_tls::ScopedKey::set
  51: syntax::with_globals

Is there anyone around who can help me debug it? For context, I would like to update cargo before beta, which will require updating rls, which will require getting rls working. cc @Xanewok

Also cc @Manishearth who looks like did the PR to update Symbol handling in clippy. There something fishy with the getting the interned string for a Symbol. I know very little about clippy, rustc, and rls, so I don't think I'll be able to make much progress.

Reproducing is fairly easy. Check out rls, change the clippy hash in Cargo.toml to the latest clippy, build with latest nightly. Create a cargo project with an empty lib.rs, and main.rs with fn main() {}. Run rls --cli, and it should crash with the above stack trace inside clippy match_def_path.

@Manishearth
Copy link
Member

cc @oli-obk , i didn't really do that part of the PR (just the mechanical bits)

@Manishearth
Copy link
Member

The backtrace doesn't make sense, there are no expressions in fn main() {}.

It's possible that the Symbol stuff broke some assumptions in RLS as well, and they need to update that.

@ehuss
Copy link
Contributor

ehuss commented May 14, 2019

(Sorry for the noise on this PR, I'm not sure where to move this discussion.)

Looking closer, I think I see what's wrong. Clippy is storing Symbol definitions in globals (lazy_static), but those symbol definitions are only valid for a single session/thread. When rls fires up a second build, clippy reuses those Symbol definitions, but they point to invalid values.

I'm not sure of the best way to fix that. I'm pretty sure Clippy shouldn't be holding any global references to Symbols. Maybe clippy_lints/src/utils/paths.rs could go back to using strs and create Symbols as needed? I'm not sure how much of a performance issue that would be, or if there's a better way.

I'm going to stop looking for now. Feel free to ping me here or on discord if you have any questions. It looks pretty straightforward, though.

@Manishearth
Copy link
Member

That's going to be super inefficient. The long term solution for paths.rs is to move as many of those into "diagnostic items" as possible.

Feels like a bug in libsyntax if this can be done safely, though

@bors bors merged commit 41184aa into rust-lang:master May 14, 2019
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.

6 participants