Skip to content

Conversation

matthri
Copy link
Contributor

@matthri matthri commented Nov 3, 2023

fixes: #11752

changelog: [iter_kv_map]: fix false positive: Don't suggest into_keys() and into_values() if the MSRV is to low

@rustbot
Copy link
Collaborator

rustbot commented Nov 3, 2023

r? @Alexendoo

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 3, 2023
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

This should probably also update the lint documentation:

/// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD, MANUAL_HASH_ONE.

@matthri
Copy link
Contributor Author

matthri commented Nov 4, 2023

Thanks for pointing that out, I updated the documentation

@bors
Copy link
Contributor

bors commented Nov 10, 2023

☔ The latest upstream changes (presumably #11750) made this pull request unmergeable. Please resolve the merge conflicts.

@matthri matthri force-pushed the iter-kv-map-msrv-fix branch from 840481f to d77b9be Compare November 12, 2023 14:48
@matthri matthri requested a review from y21 November 17, 2023 00:00
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Changes look good to me 👍
I have one comment but that's not really concerned with the PR here, rather the lint itself, so feel free to ignore.

) {
if map_type == "into_iter" && !msrv.meets(msrvs::INTO_KEYS) {
return;
}
if !expr.span.from_expansion()
&& let ExprKind::Closure(c) = m_arg.kind
Copy link
Member

Choose a reason for hiding this comment

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

This lint should probably check that the closure doesn't have parameter- or return type annotations since that can help type inference or make the map closure act as a coercion site.
Not your problem though, that could be a PR on its own, unless you also want to try to fix that here ^^.

Examples where the machine applicable suggestion breaks: coercions, inference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will have a try fixing this, but I think I will open a separate PR so this one doesn't get too cluttered ^^

@Alexendoo
Copy link
Member

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Nov 22, 2023

📌 Commit d77b9be has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 22, 2023

⌛ Testing commit d77b9be with merge ac92899...

bors added a commit that referenced this pull request Nov 22, 2023
Fix iter_kv_map false positive into_keys and into_values suggestion

fixes: #11752

changelog: [`iter_kv_map`]: fix false positive: Don't suggest `into_keys()` and `into_values()` if the MSRV is to low
@bors
Copy link
Contributor

bors commented Nov 22, 2023

💔 Test failed - checks-action_test

@matthri matthri force-pushed the iter-kv-map-msrv-fix branch from d77b9be to a20f61b Compare November 22, 2023 14:19
@matthri
Copy link
Contributor Author

matthri commented Nov 22, 2023

I ran the cargo collect-metadata command to update book/src/lint_configuration.md.
Should this maybe get mentioned in the documentation?

@matthri matthri requested a review from Alexendoo November 22, 2023 14:24
@Alexendoo
Copy link
Member

Yeah a note would be good, would be good for us to see if we can make it automatic also

@Alexendoo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 22, 2023

📌 Commit a20f61b has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 22, 2023

⌛ Testing commit a20f61b with merge c24784e...

@bors
Copy link
Contributor

bors commented Nov 22, 2023

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

@bors bors merged commit c24784e into rust-lang:master Nov 22, 2023
@matthri matthri deleted the iter-kv-map-msrv-fix branch November 22, 2023 22:17
bors added a commit that referenced this pull request Nov 23, 2023
Add documentation update hint using `cargo collect-metadata`

This adds a little reminder to update the documentation in the book using `cargo collect-metadata` after changing the lint configuration since this can easily be missed (been there done that 🙈).

> Yeah a note would be good, would be good for us to see if we can make it automatic also

_Originally posted by `@Alexendoo` in #11757 (comment)

Regarding the automation Im not sure whats the best option here. I thought about a kind of a "semi-automated" way, e.g. a `cargo dev` command which runs the "is the documentation updated" check (and maybe other useful checks) locally and reminds the user of updating the documentation so this gets caught before pushing.

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.

iter_kv_map needs to be MSRV gated on 1.54
5 participants