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

Change the criteria of interior_mutable_const #6046

Merged
merged 3 commits into from
Sep 18, 2020

Conversation

rail-rain
Copy link
Contributor

This implements my suggestion here, and so hopefully fixes #5050.

  • stop linting associated types and generic type parameters
  • start linting ones in trait impls
    whose corresponding definitions in the traits are generic
  • remove the is_copy check
    as presumably the only purpose of it is to allow
    generics with Copy bounds as Freeze is internal
    and generics are no longer linted
  • remove the term 'copy' from the tests
    as being Copy no longer have meaning

changelog: Change the criteria of declare_interior_mutable_const and borrow_interior_mutable_const to narrow the lints to only lint things that defenitly is a interior mutable type, not potentially.

@rust-highfive
Copy link

r? @phansch

(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 Sep 15, 2020
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. Linting trait impls over trait definitions is definitely the better approach here. The tests are a little mind boggling with the spaghetti traits, but they also looked good to me.

clippy_lints/src/non_copy_const.rs Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 15, 2020
@flip1995
Copy link
Member

r? @flip1995

@rust-highfive rust-highfive assigned flip1995 and unassigned phansch Sep 15, 2020
* stop linting associated types and generic type parameters
* start linting ones in trait impls
  whose corresponding definitions in the traits are generic
* remove the `is_copy` check
  as presumably the only purpose of it is to allow
  generics with `Copy` bounds as `Freeze` is internal
  and generics are no longer linted
* remove the term 'copy' from the tests
  as being `Copy` no longer have meaning
@rail-rain rail-rain force-pushed the change_criteria_non_copy_const branch from 2e158f4 to 74c2a71 Compare September 17, 2020 07:39
* rewrite the test for `declare_interior_mutable_const from scratch`

* fix a minor false positive where `Cell<"const T>` gets linted twice
@rail-rain rail-rain force-pushed the change_criteria_non_copy_const branch from 74c2a71 to 2fc9064 Compare September 17, 2020 07:48
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test restructure. This looks really good to me now and is way easier to understand.

Can you add the example from #5050 to the tests? I don't see why it should still get linted, but Self in a generics position made problems in the past for other lints. So basically the same as the part of the test with the SelfType trait, just with Option wrapped around Self. Also add a comment referring to the issue #5050, please.

tests/ui/declare_interior_mutable_const.rs Outdated Show resolved Hide resolved
@rail-rain
Copy link
Contributor Author

Of cource. Do you prefer Option<Self> in an new trait? I've put it in SelfType, but if you do let me know. I will change it so.

@flip1995
Copy link
Member

Perfect, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 18, 2020

📌 Commit d5af360 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Sep 18, 2020

⌛ Testing commit d5af360 with merge 1039376...

bors added a commit that referenced this pull request Sep 18, 2020
…p1995

Change the criteria of `interior_mutable_const`

This implements my suggestion [here](#5050 (comment)), and so hopefully fixes #5050.

* stop linting associated types and generic type parameters
* start linting ones in trait impls
  whose corresponding definitions in the traits are generic
* remove the `is_copy` check
  as presumably the only purpose of it is to allow
  generics with `Copy` bounds as `Freeze` is internal
  and generics are no longer linted
* remove the term 'copy' from the tests
  as being `Copy` no longer have meaning

---

changelog: Change the criteria of `declare_interior_mutable_const` and `borrow_interior_mutable_const` to narrow the lints to only lint things that defenitly is a interior mutable type, not potentially.
@bors
Copy link
Contributor

bors commented Sep 18, 2020

💔 Test failed - checks-action_test

@flip1995
Copy link
Member

@bors retry

A failure in the "Set up job" step seems really weird. I hope we don't have to debug this...

@bors
Copy link
Contributor

bors commented Sep 18, 2020

⌛ Testing commit d5af360 with merge d88b9b7...

@bors
Copy link
Contributor

bors commented Sep 18, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing d88b9b7 to master...

@bors bors merged commit d88b9b7 into rust-lang:master Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

declare_interior_mutable_const for Option
5 participants