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

Downgrade interior_mutable_const lints to warn by default #6012

Closed
wants to merge 0 commits into from
Closed

Downgrade interior_mutable_const lints to warn by default #6012

wants to merge 0 commits into from

Conversation

longlb
Copy link
Contributor

@longlb longlb commented Sep 6, 2020

This change updates the two lints in the file non_copy_const.rs to be warn by default rather than deny by default. It also updates the known problems for declare_interior_mutable_const to mention some issues that are affected by the lints.

fixes #5863
changelog: none

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 6, 2020
@yaahc
Copy link
Member

yaahc commented Sep 7, 2020

Hi @longlb, ty for the PR. I'm a little confused by the changes in it though. Are the unnecessary_sort_by changes related to #5863? Looking at this it seems like you might have also gone and fixed #6001 and accidentally combined the changes for both fixes into one PR.

Another thing, do we have any ui tests for interior _mutable_const? I'd have expected a stderr file to get updated with some of the text of the error message shifting from error to warn, but I don't see that in this PR. If it's because we're missing a test could we add some basic ones that generate the warning just so we can verify that it is warning and at the correct level?

@longlb
Copy link
Contributor Author

longlb commented Sep 8, 2020

Hi @yaahc. Regarding the unnecessary_sort_by changes, I did not make them. I pulled them from the main repository when I saw that someone else had commit to it, but I did not check what they were since they did not create a conflict. They are the changes that were recently merged to resolve issue #6006, so I think that removing them from my PR should be okay.

For the UI tests, both interior_mutable_constant lints have some. The stderr files didn't update because I thought it would happen automatically, so that's my mistake. I'll fix it.

@longlb
Copy link
Contributor Author

longlb commented Sep 9, 2020

Is there a way to see all the errors or warnings that clippy produces on the tests? I've been trying to see how changing between warn and error change the output, but I can't see the output anywhere since both pass all the given tests.

@rail-rain
Copy link
Contributor

Hello @longlb. You can probably use CLIPPY_TESTS=true cargo run --bin clippy-driver -- -L ./target/debug on the test files directly although the fact that the test suite including CI is clear means the output hasn't changed by this lint level change. I think the cause of this quirk is the two test files have #![warn(borrow_interior_mutable_const)] and #![warn(declare_interior_mutable_const)] attributes for some reason. I'm not sure why these are there. The uitest should work without those.

@ebroto
Copy link
Member

ebroto commented Sep 9, 2020

@longlb to get rid of all commits that are unrelated (and the last revert), you should do an interactive rebase on top of master, keeping only your commits.

This comment may be of help.

@longlb longlb closed this Sep 9, 2020
@longlb
Copy link
Contributor Author

longlb commented Sep 9, 2020

I messed up a bit while trying to fix the git stuff and wiped out my commits. I'll rewrite and open a new pull request when I'm done.

bors added a commit that referenced this pull request Oct 2, 2020
Downgrade interior_mutable_const lints to warn by default

This change updates the two lints in the file non_copy_const.rs to be warn by default rather than deny by default. It also updates the known problems for declare_interior_mutable_const to mention some issues that are affected by the lints.

This is a repeat pull request since I botched the first one (#6012). Apart from my messing up the commits of that one, I also had a problem where the stderr of the tests didn't change despite me changing both lints to warn by default. Is this normal behaviour for some lints or do I need to adjust the tests to accommodate the change?

fixes #5863
changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

declare_interior_mutable_const should probably be downgraded
5 participants