Fix `unwrap_or_else_default` false positive #11135

smoelius · 2023-07-09T23:26:46Z

This PR fixes a false positive in the handling of unwrap_or_else with a default value when the value is needed for type inference.

An easy example to exhibit the false positive is the following:

    let option = None;
    option.unwrap_or_else(Vec::new).push(1);

The following code would not compile, because the fact that the value is a Vec has been lost:

    let option = None;
    option.unwrap_or_default().push(1);

The fix is to:

implement a heuristic to tell whether an expression's type can be determined purely from its subexpressions, and the arguments and locals they use;
apply the heuristic to unwrap_or_else's receiver.

The heuristic returns false when applied to option in the above example, but it returns true when applied to option in either of the following examples:

    let option: Option<Vec<u64>> = None;
    option.unwrap_or_else(Vec::new).push(1);

    let option = None::<Vec<u64>>;
    option.unwrap_or_else(Vec::new).push(1);

(Aside: #10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.)

changelog: FP: [unwrap_or_else_default]: No longer lints if the default value is needed for type inference

rustbot · 2023-07-09T23:26:51Z

r? @Manishearth

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

Manishearth · 2023-07-10T14:40:16Z

r? @Centri3

rustbot · 2023-07-10T14:40:19Z

Failed to set assignee to Centri3: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

Centri3 · 2023-07-10T18:09:52Z

Will take a look soon enough ^^

Centri3

This is quite the PR, thanks for taking this on!

Centri3 · 2023-07-11T04:28:34Z

clippy_utils/src/ty/type_certainty/certainty.rs

+    pub fn clear_def_id(self) -> Certainty {
+        match self {
+            Certainty::Uncertain => Certainty::Uncertain,
+            Certainty::Certain(_) => Certainty::Certain(None),
+            Certainty::Contradiction => Certainty::Contradiction,
+        }
+    }
+
+    pub fn with_def_id(self, def_id: DefId) -> Certainty {
+        match self {
+            Certainty::Uncertain => Certainty::Uncertain,
+            Certainty::Certain(_) => Certainty::Certain(Some(def_id)),
+            Certainty::Contradiction => Certainty::Contradiction,
+        }
+    }


nit: I feel like these two should use if let/matches! instead, since the others are unchanged

Centri3 · 2023-07-11T04:29:57Z

clippy_utils/src/ty/type_certainty/mod.rs

+    expr_type_certainty(cx, expr).is_certain()
+}
+
+fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty {


nit: I think this could use a Visitor instead, though perhaps I'm wrong

I'm not sure a visitor would make sense here. When a node has multiple children, the results of the respective recursive calls need to be combined in some way (e.g., meeted or joined). IMHO, visitors are not really good for that sort of situation. This could be a failure of my imagination, though.

Centri3 · 2023-07-11T04:34:47Z

clippy_utils/src/ty/type_certainty/mod.rs

+    (0..(generics.parent_count + generics.params.len()) as u32).all(|index| {
+        fn_sig.inputs().iter().any(|input_ty| {
+            input_ty.skip_binder().walk().any(|arg| {
+                if let GenericArgKind::Type(ty) = arg.unpack() {


We could perhaps use matches! here to prevent the else branch (like, matches!(arg.unpack(), GenericArgKind::Type(ty) if ty.is_param(index)))

Centri3 · 2023-07-11T04:36:51Z

clippy_utils/src/ty/type_certainty/mod.rs

+    (0..(generics.parent_count + generics.params.len()) as u32).all(|index| {
+        fn_sig.inputs().iter().any(|input_ty| {
+            input_ty.skip_binder().walk().any(|arg| {
+                if let GenericArgKind::Type(ty) = arg.unpack() {
+                    ty.is_param(index)
+                } else {
+                    false
+                }
+            })
+        })
+    })


I'm not quite able to suggest something better rn, but this block is quite hard to read.

Your point is very well taken. I moved the .walk().any(...) code into its own function and added a comment to help explain what this block does.

Centri3 · 2023-07-11T04:45:38Z

clippy_utils/src/ty/type_certainty/mod.rs

+            let rhs = if type_is_inferrable_from_arguments(cx, expr) {
+                meet(
+                    std::iter::once(receiver_type_certainty).chain(args.iter().map(|arg| expr_type_certainty(cx, arg))),
+                )


nit: This could perhaps use then followed by conversion to Certainty using map_or. Though I'm not sure if that's more readable here.

Is this what you had in mind?

let rhs = type_is_inferrable_from_arguments(cx, expr) .then(|| meet(args.iter().map(|arg| expr_type_certainty(cx, arg)))) .unwrap_or(Certainty::Uncertain);

Do you think it is more clear?

Yeah it's not 😅 let's keep it the way it is.

Centri3 · 2023-07-11T04:47:05Z

clippy_utils/src/ty/type_certainty/mod.rs

+                {
+                    Certainty::Certain(None)
+                } else {
+                    Certainty::Uncertain


nit: This could perhaps use then followed by conversion to Certainty using map_or. Though I'm not sure if that's more readable here.

Same questions as above (is this what you had in mind and it is more clear?):

let lhs = ((parent_certainty.is_certain() || generics.parent_count == 0) && generics.params.is_empty()) .then(|| Certainty::Certain(None)) .unwrap_or(Certainty::Uncertain);

Centri3 · 2023-07-11T04:50:52Z

clippy_utils/src/ty/type_certainty/mod.rs

+    if let TyKind::Path(qpath) = &ty.kind {
+        qpath_certainty(cx, qpath, true)
+    } else {
+        let mut visitor = CertaintyVisitor::new(cx);


nit: Unnecessary else branch (afaict?)

I think the else is necessary. I expanded the comment preceding the if in attempt to better explain why.

Oh, I meant that you could inline the else, like,

if let TyKind::Path(qpath) = &ty.kind { return qpath_certainy(cx, qpath, true) } let mut visitor = CertainyVisitor::new(cx); // etc...

Sorry for misunderstanding. Fixed.

Centri3 · 2023-07-11T04:54:24Z

clippy_utils/src/ty/type_certainty/mod.rs

+//! As a heuristic, `expr_type_is_certain` may produce false negatives, but a false positive should
+//! be considered a bug.
+
+#![allow(clippy::wrong_self_convention)]


I don't like bulk-allowing a lint like this, especially one like wrong_self_convention. We should probably put this on individual methods instead, as expect (PS: Which method is actually triggering it? I can't see any out of the ordinary.)

Which method is actually triggering it?

AFAICT, none are. (I can't remember why I included this originally.)

smoelius · 2023-07-12T23:43:02Z

This is quite the PR, thanks for taking this on!

Thank you.

FWIW, I am highly sympathetic to comments like #11135 (comment). Please let me know anything I can do to make the code more clear.

Centri3 · 2023-07-12T23:45:40Z

Yeah it looks good now 👍

bors · 2023-07-13T13:09:11Z

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

Manishearth · 2023-07-14T07:38:07Z

@bors delegate=Centri3

bors · 2023-07-14T07:38:09Z

✌️ @Centri3, you can now approve this pull request!

If @Manishearth told you to "r=me" after making some further change, please make that change, then do @bors r=@Manishearth

Centri3 · 2023-07-14T08:16:39Z

clippy_utils/src/ty/type_certainty/certainty.rs

+        match (self, other) {
+            (Some(lhs), Some(rhs)) => {
+                if lhs == rhs {
+                    Some(Some(lhs))


nit: This is the perfect place to use then_some: (lhs == rhs).then_some(Some(lhs))

Centri3

Looks good ^^ Let's just use then_some in meet then I think this is done :D

Centri3 · 2023-07-19T09:07:03Z

clippy_utils/src/ty/type_certainty/certainty.rs

+        match (self, other) {
+            (None, _) | (_, None) => None,
+            (Some(lhs), Some(rhs)) => {
+                if lhs == rhs {


Same here as well (then_some); missed this one before

smoelius · 2023-07-19T10:25:36Z

Looks good ^^ Let's just use then_some in meet then I think this is done :D

Thanks, @Centri3 . Should I squash this to one commit?

Centri3 · 2023-07-19T10:28:40Z

Yeah if you could squash them that would be great

Centri3 · 2023-07-19T11:37:24Z

Thanks! @bors r+

bors · 2023-07-19T11:37:26Z

📌 Commit f583fd1 has been approved by Centri3

It is now in the queue for this repository.

bors · 2023-07-19T11:37:32Z

⌛ Testing commit f583fd1 with merge 7a34143...

bors · 2023-07-19T11:48:33Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3
Pushing 7a34143 to master...

type certainty: clear `DefId` when an expression's type changes to non-adt Fixes #12585 The root cause of the ICE in the linked issue was in the expression `one.x`, in the array literal. The type of `one` is the `One` struct: an adt with a DefId, so its certainty is `Certain(def_id_of_one)`. However, the field access `.x` can then change the type (to `i32` here) and that should update that `DefId` accordingly. It does do that correctly when `one.x` would be another adt with a DefId: https://github.com/rust-lang/rust-clippy/blob/97ba291d5aa026353ad93e48cf00e06f08c73830/clippy_utils/src/ty/type_certainty/mod.rs#L90-L91 but when it *isn't* an adt and there is no def id (which is the case in the linked issue: `one.x` is an i32), it keeps the `DefId` of `One`, even though that's the wrong type (which would then lead to a contradiction later when joining `Certainty`s): https://github.com/rust-lang/rust-clippy/blob/97ba291d5aa026353ad93e48cf00e06f08c73830/clippy_utils/src/ty/type_certainty/mod.rs#L92-L93 In particular, in the linked issue, `from_array([one.x, two.x])` would try to join the `Certainty` of the two array elements, which *should* have been `[Certain(None), Certain(None)]`, because `i32`s have no `DefId`, but instead it was `[Certain(One), Certain(Two)]`, because the DefId wasn't cleared from when it was visiting `one` and `two`. This is the "contradiction" that could be seen in the ICE message ... so this changes it to clear the `DefId` when it isn't an adt. cc `@smoelius` you implemented this initially in #11135, does this change make sense to you? changelog: none

rustbot assigned Manishearth Jul 9, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 9, 2023

smoelius force-pushed the unwrap_or_else_default-fp branch from 464ebb0 to 9751dd9 Compare July 9, 2023 23:41

Centri3 reviewed Jul 11, 2023

View reviewed changes

smoelius force-pushed the unwrap_or_else_default-fp branch from e01a8df to bc5369e Compare July 13, 2023 13:35

Centri3 reviewed Jul 14, 2023

View reviewed changes

smoelius force-pushed the unwrap_or_else_default-fp branch from 80a0301 to d475a76 Compare July 14, 2023 14:33

Centri3 approved these changes Jul 19, 2023

View reviewed changes

Fix unwrap_or_else_default false positive

f583fd1

smoelius force-pushed the unwrap_or_else_default-fp branch from bde78c6 to f583fd1 Compare July 19, 2023 10:45

bors merged commit 7a34143 into rust-lang:master Jul 19, 2023

smoelius deleted the unwrap_or_else_default-fp branch July 19, 2023 11:52

smoelius mentioned this pull request Jul 22, 2023

unwrap_or_else_default -> unwrap_or_default and improve resulting lint #10120

Merged

y21 mentioned this pull request Mar 29, 2024

type certainty: clear DefId when an expression's type changes to non-adt #12591

Merged

Fix unwrap_or_else_default false positive #11135

Fix unwrap_or_else_default false positive #11135

Conversation

smoelius commented Jul 9, 2023

rustbot commented Jul 9, 2023

Manishearth commented Jul 10, 2023

rustbot commented Jul 10, 2023

Centri3 commented Jul 10, 2023

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius commented Jul 12, 2023

Centri3 commented Jul 12, 2023

bors commented Jul 13, 2023

Manishearth commented Jul 14, 2023

bors commented Jul 14, 2023

Choose a reason for hiding this comment

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius commented Jul 19, 2023 • edited Loading

Centri3 commented Jul 19, 2023

Centri3 commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

Fix `unwrap_or_else_default` false positive #11135

Fix `unwrap_or_else_default` false positive #11135

smoelius Jul 12, 2023 •

edited

Loading

smoelius commented Jul 19, 2023 •

edited

Loading