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

Enable recursion for visit_ty in lint visitor #22943

Merged
merged 1 commit into from
Mar 3, 2015
Merged

Enable recursion for visit_ty in lint visitor #22943

merged 1 commit into from
Mar 3, 2015

Conversation

ipetkov
Copy link
Contributor

@ipetkov ipetkov commented Mar 1, 2015

  • The lint visitor's visit_ty method did not recurse, and had a
    reference to the now closed Enabling recursion in Visitor.visit_ty makes the compiler reject fixed-length arrays #10894
  • The newly enabled recursion has only affected the deprectated lint
    which now detects uses of deprecated items in trait impls and
    function return types
  • Renamed some references to CowString and CowVec to Cow<str> and
    Cow<[T]>, respectively, which appear outside of the crate which
    defines them
  • Replaced a few instances of InvariantType<T> with
    PhantomData<Cell<T>>
  • Disabled the deprecated lint in several places that
    reference/implement traits on deprecated items which will get cleaned
    up in the future
  • Unfortunately, this means that if a library declares
    #![deny(deprecated)] and marks anything as deprecated, it will have
    to disable the lint for any uses of said item, e.g. any impl the now
    deprecated item

For any library that denies deprecated items but has deprecated items
of its own, this is a [breaking-change]

I had originally intended for the lint to ignore uses of deprecated items that are declared in the same crate, but this goes against some previous test cases that expect the lint to capture all uses of deprecated items, so I maintained the previous approach to avoid changing the expected behavior of the lint.

Tested locally on OS X, so hopefully there aren't any deprecated item uses behind a cfg that I may have missed.

@rust-highfive
Copy link
Collaborator

r? @pcwalton

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

@alexcrichton
Copy link
Member

@bors: r+ 3ad59a0

Thanks!

@bors
Copy link
Contributor

bors commented Mar 2, 2015

⌛ Testing commit 3ad59a0 with merge 0a2bc7f...

@bors
Copy link
Contributor

bors commented Mar 2, 2015

⌛ Testing commit 3ad59a0 with merge f78333e...

@Manishearth
Copy link
Member

@bors: r-

@Manishearth
Copy link
Member

make check-stage1-std fails for me locally

time, please rewrite code that relies on it
/opt/rust/src/libstd/lib.rs:116 #![feature(old_impl_check)]
                                           ^~~~~~~~~~~~~~
/opt/rust/src/libstd/thread_local/scoped.rs:122:51: 122:70 error: chained comparison operators require parentheses
/opt/rust/src/libstd/thread_local/scoped.rs:122                 marker: ::std::marker::PhantomData<::std::cell::Cell<$t>>,
                                                                                                  ^~~~~~~~~~~~~~~~~~~
/opt/rust/src/libstd/thread_local/scoped.rs:122:70: 122:72 error: unexpected token: `u32`
/opt/rust/src/libstd/thread_local/scoped.rs:122                 marker: ::std::marker::PhantomData<::std::cell::Cell<$t>>,

Looks like that macro is only used internally in the tests, and the parsing is off for those. I guess adding some parens might help?

@bors
Copy link
Contributor

bors commented Mar 2, 2015

💔 Test failed - auto-linux-64-nopt-t

@Manishearth
Copy link
Member

Probably needs d3cc32f (not tested yet)

@Manishearth
Copy link
Member

I got this failure as part of the rollup

------------------------------------------
stderr:
------------------------------------------
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/test/compile-fail/huge-array-simple.rs:14:8: 14:11 warning: unused variable: `fat`, #[warn(unused_variables)] on by default
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/test/compile-fail/huge-array-simple.rs:14    let fat : [u8; (1<<61)+(1<<31)] = [0; (1u64<<61) as usize +(1u64<<31) as usize];
                                                                                                                    ^~~
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/test/compile-fail/huge-array-simple.rs:14:20: 14:25 error: bitshift exceeds the type's number of bits, #[deny(exceeding_bitshifts)] on by default
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/test/compile-fail/huge-array-simple.rs:14    let fat : [u8; (1<<61)+(1<<31)] = [0; (1u64<<61) as usize +(1u64<<31) as usize];
                                                                                                                                ^~~~~
error: aborting due to previous error

------------------------------------------

@nagisa
Copy link
Member

nagisa commented Mar 2, 2015

Relevant IRC logs

@ipetkov
Copy link
Contributor Author

ipetkov commented Mar 2, 2015

@Manishearth didn't get the error on my 64bit machine, so it's definitely a machine dependent error since it the exceeding_bitshifts lint is now correctly triggering on 32bit machines. I think it would be appropriate to disable it for this particular test so it doesn't shadow the expected too big for the current architecture error.

* The lint visitor's visit_ty method did not recurse, and had a
  reference to the now closed #10894
* The newly enabled recursion has only affected the `deprectated` lint
  which now detects uses of deprecated items in trait impls and
  function return types
* Renamed some references to `CowString` and `CowVec` to `Cow<str>` and
  `Cow<[T]>`, respectively, which appear outside of the crate which
  defines them
* Replaced a few instances of `InvariantType<T>` with
  `PhantomData<Cell<T>>`
* Disabled the `deprecated` lint in several places that
  reference/implement traits on deprecated items which will get cleaned
  up in the future
* Disabled the `exceeding_bitshifts` lint for
  compile-fail/huge-array-simple test so it doesn't shadow the expected
  error on 32bit systems
* Unfortunately, this means that if a library declares
  `#![deny(deprecated)]` and marks anything as deprecated, it will have
  to disable the lint for any uses of said item, e.g. any impl the now
  deprecated item

For any library that denies deprecated items but has deprecated items
of its own, this is a [breaking-change]
@ipetkov
Copy link
Contributor Author

ipetkov commented Mar 2, 2015

@alexcrichton updated the PR branch with a fix for that failing test case and incorporated @Manishearth syntax fix

@alexcrichton
Copy link
Member

@bors: r+ 2b03718

Thanks!

@alexcrichton alexcrichton assigned alexcrichton and unassigned pcwalton Mar 3, 2015
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 3, 2015
 * The lint visitor's visit_ty method did not recurse, and had a
  reference to the now closed rust-lang#10894
* The newly enabled recursion has only affected the `deprectated` lint
  which now detects uses of deprecated items in trait impls and
  function return types
* Renamed some references to `CowString` and `CowVec` to `Cow<str>` and
  `Cow<[T]>`, respectively, which appear outside of the crate which
  defines them
* Replaced a few instances of `InvariantType<T>` with
  `PhantomData<Cell<T>>`
* Disabled the `deprecated` lint in several places that
  reference/implement traits on deprecated items which will get cleaned
  up in the future
* Unfortunately, this means that if a library declares
  `#![deny(deprecated)]` and marks anything as deprecated, it will have
  to disable the lint for any uses of said item, e.g. any impl the now
  deprecated item

For any library that denies deprecated items but has deprecated items
of its own, this is a [breaking-change]

I had originally intended for the lint to ignore uses of deprecated items that are declared in the same crate, but this goes against some previous test cases that expect the lint to capture *all* uses of deprecated items, so I maintained the previous approach to avoid changing the expected behavior of the lint.

Tested locally on OS X, so hopefully there aren't any deprecated item uses behind a `cfg` that I may have missed.
@bors bors merged commit 2b03718 into rust-lang:master Mar 3, 2015
@ipetkov ipetkov deleted the lint-recursion branch March 3, 2015 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling recursion in Visitor.visit_ty makes the compiler reject fixed-length arrays
7 participants