Avoid ICE in trait without `dyn` lint #120275

estebank · 2024-01-23T18:26:04Z

Do not attempt to provide an accurate suggestion for impl Trait in bare trait types when linting. Instead, only do the object safety check when an E0782 is already going to be emitted in the 2021 edition.

Fix #120241.

rustbot · 2024-01-23T18:26:12Z

r? @cjgillot

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

estebank · 2024-01-24T03:05:39Z

r? @fmease

fmease · 2024-01-26T02:12:42Z

Sorry for the delay, I will review this tomorrow :)

fmease · 2024-01-26T17:03:32Z

compiler/rustc_hir_analysis/src/astconv/lint.rs

-                    if self_ty.span.can_be_used_for_suggestions()
-                        && !self.maybe_lint_impl_trait(self_ty, lint)
-                    {
+                    if self_ty.span.can_be_used_for_suggestions() {


Now we should be able to remove the diag.is_error() inside maybe_lint_impl_trait again simplifying the latter.

I didn't bother removing them earlier because it didn't affect the line count at all (we already have to check for is_downgradable anyways. Regardless, done.

fmease

r=me with nit addressed

estebank · 2024-01-27T00:30:53Z

@bors r=fmease

bors · 2024-01-27T00:31:01Z

📌 Commit 3022c76 has been approved by fmease

It is now in the queue for this repository.

Avoid ICE in trait without `dyn` lint Do not attempt to provide an accurate suggestion for `impl Trait` in bare trait types when linting. Instead, only do the object safety check when an E0782 is already going to be emitted in the 2021 edition. Fix rust-lang#120241.

Rollup of 9 pull requests Successful merges: - rust-lang#111379 (Boost iterator intersperse(_with) performance) - rust-lang#118182 (Properly recover from trailing attr in body) - rust-lang#119641 (Remove feature not required by `Ipv6Addr::to_cononical` doctest) - rust-lang#119759 (Add FileCheck annotations to dataflow-const-prop tests) - rust-lang#120275 (Avoid ICE in trait without `dyn` lint) - rust-lang#120376 (Update codegen test for LLVM 18) - rust-lang#120386 (ScopeTree: remove destruction_scopes as unused) - rust-lang#120398 (Improve handling of numbers in `IntoDiagnosticArg`) - rust-lang#120399 (Remove myself from review rotation) r? `@ghost` `@rustbot` modify labels: rollup

Avoid ICE in trait without `dyn` lint Do not attempt to provide an accurate suggestion for `impl Trait` in bare trait types when linting. Instead, only do the object safety check when an E0782 is already going to be emitted in the 2021 edition. Fix rust-lang#120241.

Rollup of 9 pull requests Successful merges: - rust-lang#111379 (Boost iterator intersperse(_with) performance) - rust-lang#118182 (Properly recover from trailing attr in body) - rust-lang#119641 (Remove feature not required by `Ipv6Addr::to_cononical` doctest) - rust-lang#119957 (fix: correct suggestion arg for impl trait) - rust-lang#120275 (Avoid ICE in trait without `dyn` lint) - rust-lang#120376 (Update codegen test for LLVM 18) - rust-lang#120386 (ScopeTree: remove destruction_scopes as unused) - rust-lang#120398 (Improve handling of numbers in `IntoDiagnosticArg`) - rust-lang#120399 (Remove myself from review rotation) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-01-27T09:47:24Z

@bors r-
looks like this fails cargo fix tests
https://github.com/rust-lang-ci/rust/actions/runs/7676652858/job/20924450267#step:26:10199

#120412 (comment)

matthiaskrgr · 2024-01-27T09:47:34Z

@bors rollup=iffy

Do not attempt to provide an accurate suggestion for `impl Trait` in bare trait types when linting. Instead, only do the object safety check when an E0782 is already going to be emitted in the 2021 edition. Fix rust-lang#120241.

estebank · 2024-01-29T18:34:26Z

@bors r=fmease

bors · 2024-01-29T18:34:29Z

📌 Commit 09f16b5 has been approved by fmease

It is now in the queue for this repository.

bors · 2024-01-29T19:43:12Z

⌛ Testing commit 09f16b5 with merge af08c64...

bors · 2024-01-29T21:52:32Z

☀️ Test successful - checks-actions
Approved by: fmease
Pushing af08c64 to master...

rust-timer · 2024-01-30T00:13:31Z

Finished benchmarking commit (af08c64): 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)

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)	4.7%	[1.6%, 7.8%]	2
Regressions ❌ (secondary)	2.2%	[2.2%, 2.3%]	2
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	2.4%	[-2.2%, 7.8%]	3

Cycles

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

Binary size

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

Bootstrap: 660.078s -> 662.364s (0.35%)
Artifact size: 308.02 MiB -> 308.02 MiB (0.00%)

trevyn · 2024-01-30T17:14:16Z

tests/ui/object-safety/avoid-ice-on-warning-2.old.stderr

-LL | fn id<F>(f: impl Copy) -> usize {
-   |             ++++
+LL | fn id<F>(f: dyn Copy) -> usize {
+   |             +++


This is adding an invalid suggestion to dyn a non-object-safe trait, xref #116434

Well, that's expected. Note that this only happens in Rust <2021. From the PR description:

Do not attempt to provide an accurate suggestion […]

…rors Be less confident when `dyn` suggestion is not checked for object safety rust-lang#120275 no longer checks bare traits for object safety when making a `dyn` suggestion on Rust < 2021. In this case, qualify the suggestion with a note that the trait must be object safe, to prevent user confusion as seen in rust-lang#116434 r? `@fmease`

…rors Be less confident when `dyn` suggestion is not checked for object safety rust-lang#120275 no longer checks bare traits for object safety when making a `dyn` suggestion on Rust < 2021. In this case, qualify the suggestion with a note that the trait must be object safe, to prevent user confusion as seen in rust-lang#116434 r? ``@fmease``

…rors Be less confident when `dyn` suggestion is not checked for object safety rust-lang#120275 no longer checks bare traits for object safety when making a `dyn` suggestion on Rust < 2021. In this case, qualify the suggestion with a note that the trait must be object safe, to prevent user confusion as seen in rust-lang#116434 r? `@fmease`

…rors Be less confident when `dyn` suggestion is not checked for object safety rust-lang#120275 no longer checks bare traits for object safety when making a `dyn` suggestion on Rust < 2021. In this case, qualify the suggestion with a note that the trait must be object safe, to prevent user confusion as seen in rust-lang#116434 r? ``@fmease``

…rors Be less confident when `dyn` suggestion is not checked for object safety rust-lang#120275 no longer checks bare traits for object safety when making a `dyn` suggestion on Rust < 2021. In this case, qualify the suggestion with a note that the trait must be object safe, to prevent user confusion as seen in rust-lang#116434 r? ```@fmease```

Rollup merge of rust-lang#120530 - trevyn:issue-116434, r=compiler-errors Be less confident when `dyn` suggestion is not checked for object safety rust-lang#120275 no longer checks bare traits for object safety when making a `dyn` suggestion on Rust < 2021. In this case, qualify the suggestion with a note that the trait must be object safe, to prevent user confusion as seen in rust-lang#116434 r? ```@fmease```

rustbot assigned cjgillot Jan 23, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 23, 2024

estebank mentioned this pull request Jan 23, 2024

ICE: E0391 #120241

Closed

This comment was marked as resolved.

Sign in to view

estebank force-pushed the issue-120241 branch from c6db42f to f562032 Compare January 23, 2024 18:42

This comment was marked as resolved.

Sign in to view

estebank force-pushed the issue-120241 branch from f562032 to 3d9cb5e Compare January 23, 2024 19:06

rustbot assigned fmease and unassigned cjgillot Jan 24, 2024

trevyn mentioned this pull request Jan 25, 2024

E0412 is correct but E0782 is emitted (in trait definitions) #116434

Closed

fmease reviewed Jan 26, 2024

View reviewed changes

fmease approved these changes Jan 26, 2024

View reviewed changes

estebank force-pushed the issue-120241 branch from 3d9cb5e to 3022c76 Compare January 27, 2024 00:21

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 Jan 27, 2024

Nadrieril mentioned this pull request Jan 27, 2024

Rollup of 11 pull requests #120408

Closed

Nadrieril mentioned this pull request Jan 27, 2024

Rollup of 9 pull requests #120409

Closed

Nadrieril mentioned this pull request Jan 27, 2024

Rollup of 9 pull requests #120412

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 27, 2024

Avoid ICE in trait without dyn lint

09f16b5

Do not attempt to provide an accurate suggestion for `impl Trait` in bare trait types when linting. Instead, only do the object safety check when an E0782 is already going to be emitted in the 2021 edition. Fix rust-lang#120241.

estebank force-pushed the issue-120241 branch from 3022c76 to 09f16b5 Compare January 29, 2024 18:34

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 29, 2024

bors merged commit af08c64 into rust-lang:master Jan 29, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 29, 2024

trevyn reviewed Jan 30, 2024

View reviewed changes

trevyn mentioned this pull request Jan 31, 2024

Be less confident when dyn suggestion is not checked for object safety #120530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid ICE in trait without `dyn` lint #120275

Avoid ICE in trait without `dyn` lint #120275

estebank commented Jan 23, 2024

rustbot commented Jan 23, 2024

This comment was marked as resolved.

This comment was marked as resolved.

estebank commented Jan 24, 2024

fmease commented Jan 26, 2024

fmease Jan 26, 2024

estebank Jan 27, 2024

fmease left a comment

estebank commented Jan 27, 2024

bors commented Jan 27, 2024

matthiaskrgr commented Jan 27, 2024 •

edited

Loading

matthiaskrgr commented Jan 27, 2024

estebank commented Jan 29, 2024

bors commented Jan 29, 2024

bors commented Jan 29, 2024

bors commented Jan 29, 2024

rust-timer commented Jan 30, 2024

trevyn Jan 30, 2024

fmease Jan 30, 2024

Avoid ICE in trait without dyn lint #120275

Avoid ICE in trait without dyn lint #120275

Conversation

estebank commented Jan 23, 2024

rustbot commented Jan 23, 2024

This comment was marked as resolved.

This comment was marked as resolved.

estebank commented Jan 24, 2024

fmease commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease left a comment

Choose a reason for hiding this comment

estebank commented Jan 27, 2024

bors commented Jan 27, 2024

matthiaskrgr commented Jan 27, 2024 • edited Loading

matthiaskrgr commented Jan 27, 2024

estebank commented Jan 29, 2024

bors commented Jan 29, 2024

bors commented Jan 29, 2024

bors commented Jan 29, 2024

rust-timer commented Jan 30, 2024

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avoid ICE in trait without `dyn` lint #120275

Avoid ICE in trait without `dyn` lint #120275

matthiaskrgr commented Jan 27, 2024 •

edited

Loading