When encountering struct fn call literal with private fields, suggest all builders by estebank · Pull Request #117683 · rust-lang/rust

estebank · 2023-11-07T19:33:49Z

When encountering code like Box(42), suggest Box::new(42) and all other associated functions that return -> Box<T>.

Add a way to give pre-sorted suggestions.

rustbot · 2023-11-07T19:33:57Z

r? @cjgillot

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

rustbot · 2023-11-07T19:34:00Z

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

compiler/rustc_errors/src/diagnostic.rs

compiler/rustc_resolve/src/late/diagnostics.rs

tests/ui/hygiene/extern-prelude-from-opaque-fail-2018.stderr

rustbot · 2023-11-08T18:25:01Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

WaffleLapkin

Couple nits.

Also, are all those sorts required? A bit surprising we have so much non-determinism...

compiler/rustc_errors/src/diagnostic.rs

compiler/rustc_resolve/src/late/diagnostics.rs

compiler/rustc_hir_typeck/src/expr.rs

cjgillot · 2023-11-13T23:48:37Z

compiler/rustc_hir_typeck/src/expr.rs

+                    }
+                })
+                .collect::<Vec<_>>();
+            items.sort_by_key(|(order, _, _)| *order);


Should we give higher priority to methods with fewer inputs?

I feel like definition order is a more reliable heuristic

compiler/rustc_hir_typeck/src/expr.rs

cjgillot · 2023-11-13T23:54:34Z

compiler/rustc_hir_typeck/src/expr.rs

                        .join(".")
                })
                .collect::<Vec<_>>();
+            candidate_fields.sort();


Could you add a FIXME so we don't forget to track down the non determinism?

all places where you had to call sort.

I sorted in all these to maintain the current behavior, I haven't validated that all of them needed to be sorted.

compiler/rustc_resolve/src/late/diagnostics.rs

cjgillot · 2023-11-14T00:01:20Z

compiler/rustc_resolve/src/late/diagnostics.rs

        true
    }

+    fn suggest_alternative_construction_methods(


I'm not fond of having twice (almost) identical code in two parts of the compiler. Which version has the most effect? The two tests I see exercise the non-local version, so iiuc this one.

Yeah, it's not ideal, but we have different amount of context on each stage.

This one affects only the first cases in tests/ui/privacy/suggest-box-new.rs, while the other affects the latter cases in that file. We have no way of knowing what the prevalence of these errors are in the wild. I you'd prefer I moved the logic to a central place I can do that. I didn't because there was no natural place where these wouldn't feel out of place.

bors · 2023-11-16T16:07:34Z

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

bors · 2023-11-18T23:50:34Z

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

… all builders When encountering code like `Box(42)`, suggest `Box::new(42)` and *all* other associated functions that return `-> Box<T>`.

cjgillot · 2023-11-19T20:40:18Z

Let's go with this version, to be refactored later.
@bors r+

bors · 2023-11-19T20:40:50Z

📌 Commit ac56b06 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-11-19T20:58:19Z

⌛ Testing commit ac56b06 with merge 9a66e44...

bors · 2023-11-19T22:54:08Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 9a66e44 to master...

rust-timer · 2023-11-20T00:15:18Z

Finished benchmarking commit (9a66e44): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[3.1%, 3.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.906s -> 676.7s (0.12%)
Artifact size: 313.79 MiB -> 313.73 MiB (-0.02%)

riking · 2023-11-29T01:24:26Z

compiler/rustc_hir_typeck/src/expr.rs

+                        span.shrink_to_hi().with_hi(expr_span.hi()),
+                        format!("you might have meant to use the `{name}` associated function"),
+                        suggestion(name, *args),
+                        Applicability::MaybeIncorrect,


shouldn't this be if args.len() > 0 { HasPlaceholders } else { MaybeIncorrect }? I was under the impression that was more severe than MaybeIncorrect

MaybeIncorrect is actually more severe than HasPlaceholders

rustbot assigned cjgillot Nov 7, 2023

WaffleLapkin reviewed Nov 7, 2023

View reviewed changes

estebank force-pushed the priv-builder-sugg branch from c7623f5 to 4fc13d4 Compare November 8, 2023 18:24

WaffleLapkin reviewed Nov 8, 2023

View reviewed changes

compiler/rustc_errors/src/diagnostic.rs Outdated Show resolved Hide resolved

compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

cjgillot reviewed Nov 14, 2023

View reviewed changes

estebank force-pushed the priv-builder-sugg branch from 75b0f08 to 9959160 Compare November 16, 2023 16:42

estebank added 14 commits November 19, 2023 17:47

When encountering struct fn call literal with private fields, suggest…

42aa127

… all builders When encountering code like `Box(42)`, suggest `Box::new(42)` and *all* other associated functions that return `-> Box<T>`.

Don't sort span_suggestions, leave that to caller

f1ae02f

On private tuple struct, suggest Default::default when possible

a4f47de

Add test for public struct with private fields

e0e379b

Do not suggest struct literal when fields are private

12a8bb8

Suggest using builder on curly brace struct called as fn

987155f

Remove unnecessary .collect()

1dfec45

Suggest builder functions on struct literal with private fields

be0958f

Suggest Default::default() for struct literals with private fields

69edf8e

fix tidy

00265f0

Account for number of arguments in suggestion

4d16171

review comments

a519c9b

fmt

b0d7ccb

fix rebase

ac56b06

estebank force-pushed the priv-builder-sugg branch from 9959160 to ac56b06 Compare November 19, 2023 18:07

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 Nov 19, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2023

bors merged commit 9a66e44 into rust-lang:master Nov 19, 2023

rustbot added this to the 1.76.0 milestone Nov 19, 2023

bors mentioned this pull request Nov 19, 2023

On borrow return type, suggest borrowing from arg or owned return type #117914

Merged

riking reviewed Nov 29, 2023

View reviewed changes

Uh oh!

Conversation

estebank commented Nov 7, 2023

Uh oh!

rustbot commented Nov 7, 2023

Uh oh!

rustbot commented Nov 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustbot commented Nov 8, 2023

Uh oh!

WaffleLapkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

bors commented Nov 16, 2023

Uh oh!

bors commented Nov 18, 2023

Uh oh!

cjgillot commented Nov 19, 2023

Uh oh!

bors commented Nov 19, 2023

Uh oh!

bors commented Nov 19, 2023

Uh oh!

bors commented Nov 19, 2023

Uh oh!

rust-timer commented Nov 20, 2023

Overall result: no relevant changes - no action needed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants