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

[do not merge] Relicensing rewrite #3251

Merged
merged 12 commits into from Oct 5, 2018

Conversation

Projects
None yet
5 participants
@Manishearth
Member

Manishearth commented Oct 2, 2018

From the relicensing meta-issue (#3093) we have consent to relicense from all copyright holders except for:

  • #2814
  • #427
  • #3178 (this is a new PR and I hope to get consent from them now)
  • #423 (this code has since been rewritten and is minor)

There may be people we have missed, before the final relicense we'll do a proper check.

Anyway, #2814 and #427 need to be rewritten for us to be able to relicense. Ideally, they should be written by someone who has not seen their code.

This PR removes the lints introduced in those PRs. Someone who has not seen those PRs (or this one) needs to rewrite them from scratch. I'll attempt to describe them:

  • map_clone (style): catches .map(|x| x.clone()) and .map(|x| *x) and .map(|&x| x), suggests .cloned() (ensure there are no adjustments!)
  • fn_to_numeric_cast (style, ideally in CastPass): Cast a function to something other than a usize
  • fn_to_numeric_cast_with_truncation (correctness): Same as above, but the target type is smaller.

Make these changes as PRs to this branch, and we'll land it all together.

cc @oli-obk @phansch @flip1995

@oli-obk / @flip1995 reviewed the fn_to_numeric_cast* lints so shouldn't be the ones to re-add them.

Anyone up for writing these lints? :)

Manishearth added some commits Oct 2, 2018

relicensing: Remove map_clone
This removes the code added in #427
@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Oct 2, 2018

I'll do map_clone. I don't remember ever touching it and I have not looked at the changes of this PR, just at your post.

@Manishearth

This comment has been minimized.

Member

Manishearth commented on clippy_lints/src/map_clone.rs in b36bb0a Oct 2, 2018

newline at eof

@Manishearth

This comment has been minimized.

Member

Manishearth commented on clippy_lints/src/map_clone.rs in b36bb0a Oct 2, 2018

ensure that there aren't any adjustments on path

This comment has been minimized.

Collaborator

oli-obk replied Oct 2, 2018

I tried coming up with an example where this can happen, trigger the lint and not trigger a hard rustc error and failed.

This comment has been minimized.

Member

Manishearth replied Oct 2, 2018

hmm, fair

oli-obk added some commits Oct 2, 2018

@phansch

This comment has been minimized.

Collaborator

phansch commented Oct 2, 2018

I haven't seen the code of #2814. Did it add both fn_to_numeric_cast and fn_to_numeric_cast_with_truncation?
I will start with fn_to_numeric_cast later today.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Oct 2, 2018

@phansch

This comment has been minimized.

Collaborator

phansch commented Oct 3, 2018

Ok, I have a basic version of fn_to_numeric_cast ready. But I'm not sure if I need to handle only FnDef or also FnPtr and the difference between those two. I guess a code example could be helpful.

I currently have the following test cases:

fn foo() -> String { String::new() }

fn test_function_to_numeric_cast() {
    let _ = foo as i8;
    let _ = foo as i16;
    let _ = foo as i32;
    let _ = foo as i64;
    let _ = foo as i128;
    let _ = foo as isize;

    let _ = foo as u8;
    let _ = foo as u16;
    let _ = foo as u32;
    let _ = foo as u64;
    let _ = foo as u128;

    // Casting to usize is OK and should not warn
    let _ = foo as usize;
}
@Manishearth

This comment has been minimized.

Member

Manishearth commented Oct 3, 2018

You need to handle both, there's an automatic coercion from FnDef to FnPtr. You'll hit the FnPtr edge for cases where you explicitly cast a variable of type fn(...) into an int.

(FnDef is a zero-sized type that is unique for each function, letting things like .map(func) monomorphize and inline completely)

Testcase is ok.

@phansch

This comment has been minimized.

Collaborator

phansch commented Oct 3, 2018

I added the handling of fn in variable types and function arguments and pushed the commit with the fn_to_numeric_cast lint. I don't have much more time today but will get back to it tomorrow.

@@ -0,0 +1,49 @@
#![feature(tool_lints)]
#[warn(clippy::fn_to_numeric_cast)]

This comment has been minimized.

@flip1995

flip1995 Oct 3, 2018

Collaborator

#[warn()] -> #![warn()]

phansch added some commits Oct 4, 2018

@phansch

This comment has been minimized.

Collaborator

phansch commented Oct 4, 2018

I pushed a first version of the fn_to_numeric_cast_with_truncation lint (which also fixes #2981) and also improved the description of fn_to_numeric_cast (which should close #2980).

I'm not 100% clear on two things:

  1. Do the tests for fn_to_numeric_cast_with_truncation make sense? I assume they will fail if the tests are executed on platforms with different pointer sizes.
  2. On a platform with 64 bit pointer size, is it safe to cast a function pointer to i64? If not, then I think I'll have to adjust the check.
let _ = foo as u16;
let _ = foo as u32;
// TODO: Is it bad to have these tests?

This comment has been minimized.

@flip1995

flip1995 Oct 4, 2018

Collaborator

There are compile-test flags you can set to ignore tests on specific architectures.

Only run tests if pointer width is 64bit
If the pointer width of the architechture is 32bit or something else,
then the tests will most likely produce different results.
@Manishearth

This comment has been minimized.

Member

Manishearth commented Oct 5, 2018

The changes look good! I may hold off on merging since I'll be talking about this with someone from Mozilla's legal team tomorrow morning anyway as a sanity-check.

You need to update the test expectations btw.

phansch added some commits Oct 5, 2018

Fix fn_to_numeric_cast UI tests
This collapses both lint tests into one file.
Somehow allowing the other lint in the respective files did not work
correctly. Maybe that's fixed as part of fixing #3198.
@phansch

This comment has been minimized.

Collaborator

phansch commented Oct 5, 2018

For some reason (maybe #3198), allow(clippy::fn_to_numeric_cast_with_truncation) does not work inside tests/ui/fn_to_numeric_cast.rs. I will collapse both tests into one for now, which doesn't cover every case but should be good enough for a start.

(I also merged in master, let me know if I should rebase instead)

@Manishearth

This comment has been minimized.

Member

Manishearth commented Oct 5, 2018

merge is fine. Thanks!

@Manishearth Manishearth merged commit 7596503 into master Oct 5, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@Manishearth

This comment has been minimized.

Member

Manishearth commented Oct 5, 2018

Probably should have edited out the do not merge from the title, but that's fine, it was ready to be merged 😄

@matthiaskrgr

This comment has been minimized.

Collaborator

matthiaskrgr commented Oct 5, 2018

Well, this introduced a couple of dogfood failures and compiler warnings according to travis :/

   Compiling clippy_lints v0.0.212 (/home/travis/build/rust-lang-nursery/rust-clippy/clippy_lints)
warning: constant item is never used: `CLONE`
  --> clippy_lints/src/utils/paths.rs:15:1
   |
15 | pub const CLONE: [&str; 4] = ["core", "clone", "Clone", "clone"];
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(dead_code)] on by default
warning: function is never used: `get_arg_ident`
   --> clippy_lints/src/utils/mod.rs:960:1
    |
960 | pub fn get_arg_ident(pat: &Pat) -> Option<ast::Ident> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

failures:
---- [run-pass] run-pass/needless_lifetimes_impl_trait.rs stdout ----
error: compilation failed!
status: exit code: 1
command: "target/debug/clippy-driver" "tests/run-pass/needless_lifetimes_impl_trait.rs" "-L" "/home/travis/build/rust-lang-nursery/rust-clippy/target/debug/test_build_base" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/home/travis/build/rust-lang-nursery/rust-clippy/target/debug/test_build_base/needless_lifetimes_impl_trait.stage-id" "-L" "target/debug" "-L" "target/debug/deps" "-Dwarnings" "-L" "/home/travis/build/rust-lang-nursery/rust-clippy/target/debug/test_build_base/needless_lifetimes_impl_trait.stage-id.aux"
stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
error: explicit lifetimes given in parameter types where they could be elided
  --> tests/run-pass/needless_lifetimes_impl_trait.rs:17:5
   |
17 | /     fn baz<'a>(&'a self) -> impl Foo + 'a {
18 | |         Baz { bar: self }
19 | |     }
   | |_____^
   |
note: lint level defined here
  --> tests/run-pass/needless_lifetimes_impl_trait.rs:3:9
   |
3  | #![deny(clippy::needless_lifetimes)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error
------------------------------------------
thread '[run-pass] run-pass/needless_lifetimes_impl_trait.rs' panicked at 'explicit panic', /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/runtest.rs:2550:9
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:221
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:476
   5: std::panicking::begin_panic
             at libstd/panicking.rs:410
   6: compiletest_rs::runtest::ProcRes::fatal
             at /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/runtest.rs:2550
   7: compiletest_rs::runtest::TestCx::fatal_proc_rec
             at /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/runtest.rs:1649
   8: compiletest_rs::runtest::TestCx::run_rpass_test
             at /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/runtest.rs:228
   9: compiletest_rs::runtest::TestCx::run_revision
             at /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/runtest.rs:131
  10: compiletest_rs::runtest::run
             at /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/runtest.rs:81
  11: compiletest_rs::make_test_closure::{{closure}}
             at /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/lib.rs:278
  12: <F as alloc::boxed::FnBox<A>>::call_box
             at liballoc/boxed.rs:672
  13: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1461
             at liballoc/boxed.rs:672
  14: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
failures:
    [run-pass] run-pass/needless_lifetimes_impl_trait.rs
test result: FAILED. 28 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
test compile_test ... FAILED
failures:
---- compile_test stdout ----
thread 'compile_test' panicked at 'Some tests failed', /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/lib.rs:90:22
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:221
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:476
   5: std::panicking::begin_panic
             at libstd/panicking.rs:410
   6: compiletest_rs::run_tests
             at /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/compiletest_rs-0.3.14/src/lib.rs:90
   7: compile_test::run_mode
             at tests/compile-test.rs:68
   8: compile_test::compile_test
             at tests/compile-test.rs:130
   9: compile_test::compile_test::{{closure}}
             at tests/compile-test.rs:128
  10: core::ops::function::FnOnce::call_once
             at libcore/ops/function.rs:238
  11: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1468
             at libcore/ops/function.rs:238
             at liballoc/boxed.rs:672
  12: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
failures:
    compile_test
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--test compile-test'
@Manishearth

This comment has been minimized.

Member

Manishearth commented Oct 5, 2018

That was previously broken, perhaps introduced by a rust compiler change?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment