Add a lint against never type fallback affecting unsafe code #123939

WaffleLapkin · 2024-04-14T18:14:16Z

~~I'm not very happy with the code quality... VecGraph not allowing you to get predecessors is very annoying. This should work though, so there is that.~~ (ended up updating VecGraph to support getting predecessors)

~~First few commits are from #123934 #123980~~

rustbot · 2024-04-14T18:14:23Z

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-04-14T18:14:25Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

Nilstrieb · 2024-04-14T20:24:39Z

compiler/rustc_lint_defs/src/builtin.rs

@@ -4179,6 +4180,49 @@ declare_lint! {
    "named arguments in format used positionally"
 }

+declare_lint! {
+    /// The `never_type_fallback_flowing_into_unsafe` lint detects cases where never type fallback


the lint description should mention what never type fallback actually is, I think most people are not aware of it at all

That's one of the annoying problems of all the never type stuff, fallback can't be explained without going somewhat indepth :(

I added an explanation, does it look good?

WaffleLapkin · 2024-04-15T13:54:49Z

Thinking about VecGraph more: we'll need to have predecessors for another lint anyway. One of the lints @nikomatsakis suggested checks for coercions like this:

?F -> ?1
       ^
       |
      ?N

Where ?F is a diverging variable which experiences fallback and ?N is non-diverging variable.

The algorithm to actually check this is

From the variable which is falling back do a DFS, recording all the nodes we see and sinks (variables which are not coerced to anything)
From sinks do a backwards DFS recording all nodes which we did not record in the 1-st step
The nodes recorded in the 2-nd step are the ones we need to lint against

But VecGraph does not have predecessors and so can't do a backwards DFS.

This PR (and a future one for the third lint) are struggling/will struggle from a similar problem: we need to be able to go backwards (because this lint does not care about coercion direction -- since rust infers both ways).

I will try to use just Graph, but I'm not entirely sure its interface will work here... TBQH I wish rustc would just use an off-the-shelf library for graphs, instead of rolling its own half baked and undocumented version.

WaffleLapkin · 2024-04-15T15:14:22Z

Okay. VecGraph actually exists for a reason -- having indices and edges upfront simplifies things quite a bit. Graph won't work nicely here, because we'd have to keep TyVid <-> NodeIndex map.

I'll try adding opt-in backreferences to VecGraph.

bors · 2024-04-16T20:29:13Z

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

…r, r=wesleywiser Add an opt-in to store incoming edges in `VecGraph` + misc r? `@fmease` needed for rust-lang#123939

…r, r=wesleywiser Add an opt-in to store incoming edges in `VecGraph` + misc r? ``@fmease`` needed for rust-lang#123939

…r, r=wesleywiser Add an opt-in to store incoming edges in `VecGraph` + misc r? `@fmease` needed for rust-lang#123939

…r, r=wesleywiser Add an opt-in to store incoming edges in `VecGraph` + misc r? ``@fmease`` needed for rust-lang#123939

…r, r=wesleywiser Add an opt-in to store incoming edges in `VecGraph` + misc r? ```@fmease``` needed for rust-lang#123939

Rollup merge of rust-lang#123980 - WaffleLapkin:graph-average-refactor, r=wesleywiser Add an opt-in to store incoming edges in `VecGraph` + misc r? ```@fmease``` needed for rust-lang#123939

compiler-errors · 2024-04-24T00:38:27Z

r? compiler-errors

compiler-errors

Some style nits, and then requesting that you implement the method case too.

compiler/rustc_hir_typeck/src/fallback.rs

WaffleLapkin · 2024-04-29T00:01:57Z

@compiler-errors I've added support for unsafe methods, alongside other unsafe things which may have a type infered from the environment (like pointer dereference, union access, etc). The change is annoyingly big, but yeah...

compiler-errors · 2024-04-29T22:48:58Z

compiler/rustc_hir_typeck/messages.ftl

 hir_typeck_never_type_fallback_flowing_into_unsafe =
-    never type fallback affects this call to an `unsafe` function
+    never type fallback affects this {$reason ->
+    [call] call to an `unsafe` function
+    [union_field] union access
+    [deref] raw pointer dereference
+    [path] `unsafe` function
+    [method] call to an `unsafe` method


Split this into 5 slugs and do:

#[derive(Diagnostic)] enum NeverTypeFallbackFlowingIntoUnsafe { #[diag(hir_typeck_never_type_fallback_flowing_into_unsafe_call)] Call, ... }

And then

hir_typeck_never_type_fallback_flowing_into_unsafe_call = never type fallback affects this call to an `unsafe` function

https://github.com/projectfluent/fluent/wiki/Good-Practices-for-Developers#prefer-separate-messages-over-variants-for-ui-logic

I've kept the reason enum and then convert to the diagnostic, cause I find using errors::* until the literal "emit" somewhat weird. But LMK if I should just return the diagnostic from the "find unsafe infer vars" function.

compiler-errors

r=me after splitting the fluent slugs

bors · 2024-04-30T16:17:20Z

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

(never_type_fallback_flowing_into_unsafe)

compiler-errors · 2024-05-02T02:12:39Z

that's fine

@bors r+

bors · 2024-05-02T02:12:41Z

📌 Commit b562617 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-05-02T02:41:43Z

⌛ Testing commit b562617 with merge fcc06c8...

bors · 2024-05-02T04:44:34Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing fcc06c8 to master...

rust-timer · 2024-05-02T06:00:18Z

Finished benchmarking commit (fcc06c8): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[1.1%, 1.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

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: 674.637s -> 671.059s (-0.53%)
Artifact size: 315.98 MiB -> 315.99 MiB (0.00%)

…s, r=workingjubilee Document never type fallback in `!`'s docs Pulled the documentation I've written for rust-lang#123939. I want a single place where never type fallback is explained, which can be referred in all the lints and migration materials.

rustbot assigned wesleywiser Apr 14, 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 Apr 14, 2024

WaffleLapkin mentioned this pull request Apr 14, 2024

Tracking Issue for Rust 2024: Make ! fall back to ! #123748

Open

10 tasks

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the never-fallback-unsafe-lint branch from 7dad013 to ea60ab1 Compare April 14, 2024 18:40

Nilstrieb reviewed Apr 14, 2024

View reviewed changes

WaffleLapkin mentioned this pull request Apr 15, 2024

Add an opt-in to store incoming edges in VecGraph + misc #123980

Merged

rust-cloud-vms bot force-pushed the never-fallback-unsafe-lint branch from ea60ab1 to 14f1124 Compare April 16, 2024 00:30

WaffleLapkin mentioned this pull request Apr 17, 2024

Edition 2024: Make ! fall back to ! #123508

Open

rust-cloud-vms bot force-pushed the never-fallback-unsafe-lint branch from 14f1124 to 447a08c Compare April 19, 2024 12:04

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the never-fallback-unsafe-lint branch from 447a08c to f56a71b Compare April 19, 2024 12:50

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the never-fallback-unsafe-lint branch 2 times, most recently from cee445b to 2a14501 Compare April 19, 2024 18:08

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the never-fallback-unsafe-lint branch from 2a14501 to d36b7f0 Compare April 20, 2024 16:48

rustbot assigned compiler-errors and unassigned wesleywiser Apr 24, 2024

WaffleLapkin mentioned this pull request Apr 26, 2024

Document never type fallback in !'s docs #124419

Open

compiler-errors requested changes Apr 27, 2024

View reviewed changes

compiler-errors reviewed Apr 29, 2024

View reviewed changes

compiler-errors requested changes Apr 29, 2024

View reviewed changes

compiler-errors 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2024

WaffleLapkin added 7 commits May 2, 2024 03:47

Convert if to match

ff0bfea

Add a lint against never type fallback affecting unsafe code

aa0a916

Add an explanation about never type fallback

b63cb6f

Reorganize uses

fe9c5e6

Add UnordMap::try_insert

3c815a6

Add proper support for all kinds of unsafe ops to the lint

0df43db

(never_type_fallback_flowing_into_unsafe)

Split hir_typeck_never_type_fallback_flowing_into_unsafe into 5 slugs

b562617

WaffleLapkin force-pushed the never-fallback-unsafe-lint branch from ea4473d to b562617 Compare May 2, 2024 01:50

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 May 2, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 2, 2024

bors merged commit fcc06c8 into rust-lang:master May 2, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a lint against never type fallback affecting unsafe code #123939

Add a lint against never type fallback affecting unsafe code #123939

WaffleLapkin commented Apr 14, 2024 •

edited

rustbot commented Apr 14, 2024

rustbot commented Apr 14, 2024

This comment has been minimized.

Nilstrieb Apr 14, 2024 •

edited

WaffleLapkin Apr 15, 2024

WaffleLapkin Apr 19, 2024

WaffleLapkin commented Apr 15, 2024

WaffleLapkin commented Apr 15, 2024

bors commented Apr 16, 2024

This comment has been minimized.

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

compiler-errors commented Apr 24, 2024

compiler-errors left a comment

WaffleLapkin commented Apr 29, 2024

compiler-errors Apr 29, 2024 •

edited

WaffleLapkin May 2, 2024

compiler-errors left a comment •

edited

bors commented Apr 30, 2024

compiler-errors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

rust-timer commented May 2, 2024

Add a lint against never type fallback affecting unsafe code #123939

Add a lint against never type fallback affecting unsafe code #123939

Conversation

WaffleLapkin commented Apr 14, 2024 • edited

rustbot commented Apr 14, 2024

rustbot commented Apr 14, 2024

This comment has been minimized.

Nilstrieb Apr 14, 2024 • edited

Choose a reason for hiding this comment

WaffleLapkin Apr 15, 2024

Choose a reason for hiding this comment

WaffleLapkin Apr 19, 2024

Choose a reason for hiding this comment

WaffleLapkin commented Apr 15, 2024

WaffleLapkin commented Apr 15, 2024

bors commented Apr 16, 2024

This comment has been minimized.

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

compiler-errors commented Apr 24, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

WaffleLapkin commented Apr 29, 2024

compiler-errors Apr 29, 2024 • edited

Choose a reason for hiding this comment

WaffleLapkin May 2, 2024

Choose a reason for hiding this comment

compiler-errors left a comment • edited

Choose a reason for hiding this comment

bors commented Apr 30, 2024

compiler-errors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

rust-timer commented May 2, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

WaffleLapkin commented Apr 14, 2024 •

edited

Nilstrieb Apr 14, 2024 •

edited

compiler-errors Apr 29, 2024 •

edited

compiler-errors left a comment •

edited