Improve suggestions for `NonZeroT` <- `T` coercion error#99438

WaffleLapkin · 2022-07-18T20:18:16Z

Currently, when encountering a type mismatch error with NonZeroT and T (for example NonZeroU8 and u8) we errorneusly suggest wrapping expression in NonZeroT:

error[E0308]: mismatched types
 --> ./t.rs:7:35
  |
7 |     let _: std::num::NonZeroU64 = 1;
  |            --------------------   ^ expected struct `NonZeroU64`, found integer
  |            |
  |            expected due to this
  |
help: try wrapping the expression in `std::num::NonZeroU64`
  |
7 |     let _: std::num::NonZeroU64 = std::num::NonZeroU64(1);
  |                                   +++++++++++++++++++++ +

I've removed this suggestion and added suggestions to call new (for Option<NonZeroT> <- T case) or new and unwrap (for NonZeroT <- T case):

error[E0308]: mismatched types
 --> ./t.rs:7:35
  |
7 |     let _: std::num::NonZeroU64 = 1;
  |            --------------------   ^ expected struct `NonZeroU64`, found integer
  |            |
  |            expected due to this
  |
help: Consider calling `NonZeroU64::new`
  |
7 |     let _: std::num::NonZeroU64 = NonZeroU64::new(1).unwrap();
  |                                   ++++++++++++++++ ++++++++++

error[E0308]: mismatched types
 --> ./t.rs:8:43
  |
8 |     let _: Option<std::num::NonZeroU64> = 1;
  |            ----------------------------   ^ expected enum `Option`, found integer
  |            |
  |            expected due to this
  |
  = note: expected enum `Option<NonZeroU64>`
             found type `{integer}`
help: Consider calling `NonZeroU64::new`
  |
8 |     let _: Option<std::num::NonZeroU64> = NonZeroU64::new(1);
  |                                           ++++++++++++++++ +

r? @compiler-errors

rustbot · 2022-07-18T20:18:21Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

compiler-errors

Thanks @WaffleLapkin for an interesting and useful diagnostics improvement as always. This looks good as is, but I had an idea of generalizing both parts of the PR. Let me know your thoughts.

compiler/rustc_typeck/src/check/demand.rs

compiler-errors · 2022-07-18T21:25:58Z

compiler/rustc_typeck/src/check/demand.rs

+                    if !sole_field.did.is_local()
+                        && !sole_field.vis.is_accessible_from(
+                            self.tcx.parent_module(expr.hir_id).to_def_id(),
+                            self.tcx,
+                        )


Suggested change

if !sole_field.did.is_local()

&& !sole_field.vis.is_accessible_from(

self.tcx.parent_module(expr.hir_id).to_def_id(),

self.tcx,

)

if !sole_field.vis.is_accessible_from(

self.body_id.owner.to_def_id(),

self.tcx,

)

No need to restrict to only foreign def id, and we can get a more accurate def id from self.body_id.owner.to_def_id() i think.

Restriction to foreign def ids is deliberate, see the test:

rust/src/test/ui/mismatched_types/wrap-suggestion-privacy.rs

Lines 10 to 11 in da2752e

// Suggest wrapping expression because type is local

// and its privacy can be easily changed

Hm, I guess so. I think that giving an incorrect suggestion isn't as useful, especially in a large codebase, but can you at least note that in a comment?

Specifically " its privacy can be easily changed " as the justification would be nice to note

Not sure how to present it in a good way, but I've tried 5bd88df

haha I meant a comment in the source code, but this works too!

lmao

it's bed time

compiler/rustc_typeck/src/check/demand.rs

- Use `expr.hir_id.owner` instead of `self.tcx.parent_module(expr.hir_id)` - Use `.type_at()` instead of `.first()` + `.expect_ty()` - Use single `.find()` with `&&` condition Co-authored-by: Michael Goulet <michael@errs.io>

compiler-errors

This looks good for now. It can be tweaked later if anyone complains about it.

compiler-errors · 2022-07-18T22:38:39Z

r=me once CI turns green

@bors delegate+

bors · 2022-07-18T22:38:41Z

✌️ @WaffleLapkin can now approve this pull request

WaffleLapkin · 2022-07-18T23:21:02Z

@bors r=compiler-errors

bors · 2022-07-18T23:21:03Z

📌 Commit 5bd88df has been approved by compiler-errors

It is now in the queue for this repository.

…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#98028 (Add E0790 as more specific variant of E0283) - rust-lang#99384 (use body's param-env when checking if type needs drop) - rust-lang#99401 (Avoid `Symbol` to `&str` conversions) - rust-lang#99419 (Stabilize `core::task::ready!`) - rust-lang#99435 (Revert "Stabilize $$ in Rust 1.63.0") - rust-lang#99438 (Improve suggestions for `NonZeroT` <- `T` coercion error) - rust-lang#99441 (Update mdbook) - rust-lang#99453 (:arrow_up: rust-analyzer) - rust-lang#99457 (use `par_for_each_in` in `par_body_owners` and `collect_crate_mono_items`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

WaffleLapkin added 2 commits July 18, 2022 21:55

Do not suggest "wrapping the expression in std::num::NonZeroU64"

37c21d6

Suggest a fix for NonZero* <- * coercion error

7163e7f

rust-highfive assigned compiler-errors Jul 18, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 18, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2022

compiler-errors reviewed Jul 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/demand.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/check/demand.rs Show resolved Hide resolved

check accessibility before suggesting wrapping expressions

da2752e

compiler-errors reviewed Jul 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/demand.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Jul 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/demand.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Jul 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/demand.rs Show resolved Hide resolved

WaffleLapkin and others added 2 commits July 19, 2022 02:15

Apply suggestions from the review

2edad7d

- Use `expr.hir_id.owner` instead of `self.tcx.parent_module(expr.hir_id)` - Use `.type_at()` instead of `.first()` + `.expect_ty()` - Use single `.find()` with `&&` condition Co-authored-by: Michael Goulet <michael@errs.io>

Add a note about privacy to wrapping suggestion

5bd88df

compiler-errors approved these changes Jul 18, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2022

matthiaskrgr mentioned this pull request Jul 19, 2022

Rollup of 9 pull requests #99462

Merged

bors merged commit e6a100b into rust-lang:master Jul 19, 2022

rustbot added this to the 1.64.0 milestone Jul 19, 2022

WaffleLapkin deleted the dont_wrap_in_non_zero branch November 22, 2022 17:26

	// Suggest wrapping expression because type is local
	// and its privacy can be easily changed

Uh oh!

Conversation

WaffleLapkin commented Jul 18, 2022

Uh oh!

rustbot commented Jul 18, 2022

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Jul 18, 2022

Uh oh!

bors commented Jul 18, 2022

Uh oh!

WaffleLapkin commented Jul 18, 2022

Uh oh!

bors commented Jul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants