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

Add tests for ensuring we have the right impls for arithmetic traits #49660

Closed
clarfonthey opened this issue Apr 4, 2018 · 14 comments · Fixed by #63692
Closed

Add tests for ensuring we have the right impls for arithmetic traits #49660

clarfonthey opened this issue Apr 4, 2018 · 14 comments · Fixed by #63692
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Apr 4, 2018

The other day I noticed that Wrapping only implements Shl and co. for usize, which contradicts that without Wrapping the shift amount can be any type. So, I think it may be useful to create tests which use macros to shove together all the types we expect to have impls for and check that we're consistent there.

What I've noticed:

  • For every operation three extra impls are added, with the combinations of single-level references on both sides, e.g. &T + T, T + &T, &T + &T for Add.
  • Assignment ops do the same as the above, with only one extra impl, e.g. T += &T for Add.
  • Add, Sub, Mul, Div, Rem exist for all int and float types with themselves
  • BitAnd, BitOr, BitXor exist for all int types and bool
  • Shl and Shr allow any integer type for both the left and the right side
  • Neg exists for all ints and floats
  • Not exists for all ints and bool
  • Wrapping should mimic all of the above, where references are of the form &Wrapping<T> and not Wrapping<&T>.

Like, I wouldn't be surprised if rearranging some macros might add or remove impls for these and adding comprehensive tests might help make sure we covered everything.

@scottmcm scottmcm added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 5, 2018
@XAMPPRocky XAMPPRocky added A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 29, 2018
@varkor varkor added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Dec 21, 2018
@bytesnail
Copy link

This seems like a simple but useful task, I will try to finish it.

@TheSirC
Copy link

TheSirC commented Jun 19, 2019

I am reformulating the task to make sure it is clear (to myself at least) :

The goal

Create tests using a set of macros to check that every numerical instances of Wrapping<T> for the following operations Op are implemented for every combination of numerical instances (i32 <-> i64, i32 <-> bool , ...) :

Op T Op T &T Op T T Op &T &T Op &T
Add*
Sub*
Mul*
Div*
Rem*
BitAnd¤
BitOr¤
BitXor¤
Shl
Shr
Neg*
Not¤

with :

  • Op* having the integer and float types with themselves,
  • Op¤ having the integer and bool types with themselves,
  • Op⟊ having the integer and float types with themselves,
  • T = {Integer,Float,Wrapping<T>} and &T = &{Integer,Float,Wrapping<T>}

Am I correct ?

Link to other issues

How does this relate to #23545 ?(e.g. is it possible to bypass the bug and still implement the test ? I could not see from the issue if a solution had been found)

@iluuu1994
Copy link
Contributor

@rustbot claim

@TheSirC Wrapping isn't implemented for bool and f32/f64. So this is probably only useful for integer types.

@clarfonthey
Copy link
Contributor Author

This doesn't actually fix the issue; it should be tested for all types, not just Wrapping.

@shivan1b
Copy link
Contributor

Hi @clarfon ! Are you or anyone else working on adding the missed tests?

@clarfonthey
Copy link
Contributor Author

They're free for anyone to implement right now!

@iluuu1994
Copy link
Contributor

iluuu1994 commented Sep 10, 2019

Nope, not right now. Feel free to take over. It should be pretty similar to #63692, just add the missing types (bool, fp) and operators (bit wise operators).

@shivan1b
Copy link
Contributor

Thank you very much @clarfon and @iluuu1994 , I'll send a PR.

@Dylan-DPC-zz
Copy link

@rustbot unassign

@mental32
Copy link
Contributor

@rustbot claim

I'll try to get a PR in by next weekend, If nothing happens past that I'm afraid I may have been too busy so [anyone] feel free to overtake 😄

@rustbot rustbot self-assigned this Apr 13, 2020
@ryzokuken
Copy link
Contributor

@rustbot claim

@rustbot rustbot assigned rustbot and unassigned rustbot Apr 27, 2020
@Alexendoo
Copy link
Member

Hi, are you still working on this issue @ryzokuken?

@ryzokuken
Copy link
Contributor

@Alexendoo I'm sorry, I couldn't do it. Please feel free to reassign this.

@Alexendoo
Copy link
Member

No worries @ryzokuken

@rustbot release-assignment

@rustbot rustbot removed their assignment Aug 5, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 14, 2021
Fix rust-lang#49660 - Adds checks to ensure existence of arithmetic trait implementations

The first 2 commits fix an issue with the existing `wrapping.rs` tests. It wasn't referred to from the module, so the file was being ignored. This is fixed in rust-lang@872dc60 This surfaced a bug in its macro which is fixed in rust-lang@8ddad18

Lastly, commit rust-lang@64d695b is the actual tests for fixing rust-lang#49660

The following checks are done:

* `Add`, `Sub`, `Mul`, `Div`, `Rem`
  * `T op T`, `T op &T`, `&T op T` and `&T op &T`
  * for all integer and floating point types
* `AddAssign`, `SubAssign`, `MulAssign`, `DivAssign`, `RemAssign`
  * `&mut T op T` and `&mut T op &T`
  * for all integer and floating point types
* `Neg`
  * `op T` and `op &T`
  * for all signed integer and floating point types
* `Not`
  * `op T` and `op &T`
  * for `bool`
* `BitAnd`, `BitOr`, `BitXor`
  * `T op T`, `T op &T`, `&T op T` and `&T op &T`
  * for all integer types and bool
* `BitAndAssign`, `BitOrAssign`, `BitXorAssign`
  * `&mut T op T` and `&mut T op &T`
  * for all integer types and bool
* `Shl`, `Shr`
  * `L op R`, `L op &R`, `&L op R` and `&L op &R`
  * for all pairs of integer types
* `ShlAssign`, `ShrAssign`
  * `&mut L op R`, `&mut L op &R`
  * for all pairs of integer types

NOTE: I'd like some feedback on improving the macros. I'm not familiar with the idioms and patterns there and composing them has been a challenge for me.

[EDIT]: updated links to commits after rebase.
@bors bors closed this as completed in 64d695b Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.