a fn pointer doesn't implement `Fn`/`FnMut`/`FnOnce` if its return type isn't sized #100096

compiler-errors · 2022-08-03T05:22:57Z

I stumbled upon #83915 which hasn't received much attention recently, and I wanted to revive it since this is one existing soundness hole that seems pretty easy to fix.

I'm not actually sure that the alternative approach described here is sufficient, given the src/test/ui/function-pointer/unsized-ret.rs example I provided below. Rebasing the branch mentioned in that comment and testing that UI test, it seems that we actually end up only observing that str: !Sized during monomorphization, whereupon we ICE. Even if we were to fix that ICE, ideally we'd be raising an error that a fn pointer is being used badly during typecheck instead of monomorphization, hence adapting the original approach in #83915.

I am happy to close this if people would prefer we rebase the original PR and land that -- I am partly opening to be annoying and get people thinking about this unsoundness again ❤️ 😸

cc: @estebank and @nikomatsakis
r? types

Here's a link to the thread: https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/PR.20.2383915/near/235421351 for more context.

estebank · 2022-08-03T11:11:03Z

src/test/ui/function-pointer/unsized-ret.stderr

+LL |     foo::<fn() -> str, _>(None, ());
+   |     ^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
+   |
+   = help: within `fn() -> str`, the trait `Sized` is not implemented for `str`


Random thought: I wonder if we could get the span for the str and use it for a span_help here, opportunistically (because for things like type aliases in a foreign crate, we might not be able to recover them).

estebank · 2022-08-03T11:12:03Z

src/test/ui/function-pointer/unsized-ret.stderr

+note: required by a bound in `foo`
+  --> $DIR/unsized-ret.rs:4:11
+   |
+LL | fn foo<F: Fn<T>, T>(f: Option<F>, t: T) {
+   |           ^^^^^ required by this bound in `foo`


The implicit Sized requirements should have their own obligation cause so that we could mention why it was required (as the code doesn't mention it, this would be mystifying the first time you encounter it).

estebank · 2022-08-03T11:13:44Z

Could we add this test to this PR as well?
https://github.com/rust-lang/rust/pull/83915/files#diff-8c64c576ccaceb816e71d2279a6ee4bf79211bc06f55c46dda3f98a18748ad7a

estebank · 2022-08-03T11:18:27Z

I'm ok with landing this instead of my PR. I'll let the types team decide whether this is the right approach (although it looks reasonable to me).

compiler-errors · 2022-08-10T03:26:02Z

i guess this really should be r? @nikomatsakis since he reviewed the last one

pnkfelix · 2022-09-08T14:40:11Z

Discussed in T-compiler triage meeting. Nominating for T-types to handle forward movement.

@rustbot label: I-types-nominated

compiler-errors · 2022-09-08T16:15:17Z

r? @jackh726

jackh726

r=me with testing for fn ptrs with bound vars

jackh726 · 2022-09-12T02:23:38Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

+        // The binder on the Fn obligation is "less" important than the one on
+        // the signature, as evidenced by how we treat it during projection.
+        // The safe thing to do here is to liberate it, though, which should
+        // have no worse effect than skipping the binder here.
+        let liberated_fn_ty = self.infcx.replace_bound_vars_with_placeholders(obligation.self_ty());
+        let output_ty = self
+            .infcx
+            .replace_bound_vars_with_placeholders(liberated_fn_ty.fn_sig(self.tcx()).output());
+        let output_ty = normalize_with_depth_to(
+            self,
+            obligation.param_env,
+            cause.clone(),
+            obligation.recursion_depth,
+            output_ty,
+            &mut nested,
+        );
+        let tr = ty::Binder::dummy(ty::TraitRef::new(
+            self.tcx().require_lang_item(LangItem::Sized, None),
+            self.tcx().mk_substs_trait(output_ty, &[]),
+        ));


Just for completeness, can you update the test to include some fn ptrs with bound vars?

… is unsized

compiler-errors · 2022-09-12T03:25:20Z

Added some check-pass tests with higher-ranked lifetimes, and a failure to the unsized-return test with a higher-ranked lifetime

compiler-errors · 2022-09-12T03:26:29Z

src/test/ui/function-pointer/unsized-ret.stderr

+   |     |
+   |     required by a bound introduced by this call
+   |
+   = help: within `for<'a> fn(&'a ()) -> (dyn std::fmt::Display + 'a)`, the trait `for<'a> Sized` is not implemented for `(dyn std::fmt::Display + 'a)`


Interestingly this mentions for<'a> Sized, but I think that's because we print placeholder regions as if they were in binders.

jackh726 · 2022-09-20T19:19:25Z

@bors r+

IIUC, this doesn't change stable behavior, only fixes unstable behavior.

bors · 2022-09-20T19:19:27Z

📌 Commit 3b573c9 has been approved by jackh726

It is now in the queue for this repository.

bors · 2022-09-21T04:35:23Z

⌛ Testing commit 3b573c9 with merge 1de00d1...

bors · 2022-09-21T07:41:53Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 1de00d1 to master...

rust-timer · 2022-09-21T08:59:54Z

Finished benchmarking commit (1de00d1): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	23.0%	[21.3%, 25.2%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	-	-	0
Regressions ❌ (secondary)	9.1%	[7.4%, 10.6%]	6
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

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)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	21.0%	[2.6%, 26.6%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[2.7%, 2.7%]	1

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

compiler-errors · 2022-09-21T16:35:24Z

Oh my goodness, I will look into those perf regressions ASAP.

Mark-Simulacrum · 2022-09-27T13:12:19Z

FWIW, the regressions are on a single secondary benchmark, and while they're real this is a soundness fix so if we can't mitigate them it makes sense to move forward I think. (That said the change here seems like it could definitely affect other code, wg-grammar probably just has more fn ptrs or something).

We do see a similar-ish regression in some of the bootstrap benchmarks, though to a lesser extent, so it's not entirely limited I guess.

compiler-errors · 2022-10-04T21:45:10Z

So I haven't had time to fix this, nor do I think this is an easily mitigated regression. I'm not sure if @rust-lang/wg-prioritization deems this a bad enough perf regression that we should roll it back.

This is technically an issue that is only triggered with #![feature(fn_traits, unboxed_closures)], so it doesn't represent a risk on stable Rust, for example. Not sure if @rust-lang/types has an opinion on whether we should roll back/table this and revisit it before (theoretically) we were to stabilize the raw Fn traits.

lcnr · 2022-10-04T22:36:35Z

I don't think we should revert this PR, the regression is limited to only one secondary benchmark and while it may make sense to look into how we can reduce that regression there, it doesn't feel bad enough to warrant a complete revert of this soundness fix

…safety, r=lcnr Check that built-in callable types validate their output type is `Sized` (in new solver) Working on parity with old solver. Putting this up for consideration, it's not *really* needed or anything just yet. Maybe it's better to approach this from another direction (like always checking the item bounds when calling `consider_assumption`? we may need that for coinduction to be sound though?) This basically implements rust-lang#100096 for the new solver.

rust-highfive assigned lcnr Aug 3, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 3, 2022

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

estebank reviewed Aug 3, 2022

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned lcnr Aug 10, 2022

rustbot added the I-types-nominated Nominated for discussion during a types team meeting. label Sep 8, 2022

rust-highfive assigned jackh726 and unassigned nikomatsakis Sep 8, 2022

jackh726 requested changes Sep 12, 2022

View reviewed changes

jackh726 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 Sep 12, 2022

compiler-errors force-pushed the fn-return-must-be-sized branch from 98b5f4e to 29ace82 Compare September 12, 2022 03:20

fn pointers don't implement Fn/FnMut/FnOnce traits if its return type…

3b573c9

… is unsized

compiler-errors force-pushed the fn-return-must-be-sized branch from 29ace82 to 3b573c9 Compare September 12, 2022 03:24

compiler-errors commented Sep 12, 2022

View reviewed changes

compiler-errors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2022

jackh726 approved these changes Sep 20, 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 Sep 20, 2022

jackh726 removed the I-types-nominated Nominated for discussion during a types team meeting. label Sep 20, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 21, 2022

bors merged commit 1de00d1 into rust-lang:master Sep 21, 2022

rustbot added this to the 1.66.0 milestone Sep 21, 2022

rustbot added the perf-regression Performance regression. label Sep 21, 2022

compiler-errors mentioned this pull request Sep 21, 2022

Only register Sized in fn pointer confirmation if return is not trivially Sized #102108

Closed

compiler-errors mentioned this pull request Sep 26, 2022

ice: combine tests: Ty::fn_sig() called on non-fn type: _ #102289

Closed

This was referenced Oct 4, 2022

Enforce Sized return types on Fn* bounds #83915

Closed

fn() -> Out is a valid type for unsized types Out, and it implements FnOnce<(), Output = Out> #82633

Closed

jruderman mentioned this pull request Dec 18, 2022

maybe regression related to GATs #105878

Open

compiler-errors mentioned this pull request Feb 10, 2023

Check that built-in callable types validate their output type is Sized (in new solver) #107867

Merged

compiler-errors deleted the fn-return-must-be-sized branch August 11, 2023 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a fn pointer doesn't implement `Fn`/`FnMut`/`FnOnce` if its return type isn't sized #100096

a fn pointer doesn't implement `Fn`/`FnMut`/`FnOnce` if its return type isn't sized #100096

compiler-errors commented Aug 3, 2022 •

edited

Loading

estebank Aug 3, 2022

estebank Aug 3, 2022

estebank commented Aug 3, 2022

estebank commented Aug 3, 2022

compiler-errors commented Aug 10, 2022

pnkfelix commented Sep 8, 2022

compiler-errors commented Sep 8, 2022

jackh726 left a comment

jackh726 Sep 12, 2022

compiler-errors commented Sep 12, 2022

compiler-errors Sep 12, 2022

jackh726 commented Sep 20, 2022

bors commented Sep 20, 2022

bors commented Sep 21, 2022

bors commented Sep 21, 2022

rust-timer commented Sep 21, 2022

compiler-errors commented Sep 21, 2022

Mark-Simulacrum commented Sep 27, 2022

compiler-errors commented Oct 4, 2022

lcnr commented Oct 4, 2022

a fn pointer doesn't implement Fn/FnMut/FnOnce if its return type isn't sized #100096

a fn pointer doesn't implement Fn/FnMut/FnOnce if its return type isn't sized #100096

Conversation

compiler-errors commented Aug 3, 2022 • edited Loading

estebank Aug 3, 2022

Choose a reason for hiding this comment

estebank Aug 3, 2022

Choose a reason for hiding this comment

estebank commented Aug 3, 2022

estebank commented Aug 3, 2022

compiler-errors commented Aug 10, 2022

pnkfelix commented Sep 8, 2022

compiler-errors commented Sep 8, 2022

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Sep 12, 2022

Choose a reason for hiding this comment

compiler-errors commented Sep 12, 2022

compiler-errors Sep 12, 2022

Choose a reason for hiding this comment

jackh726 commented Sep 20, 2022

bors commented Sep 20, 2022

bors commented Sep 21, 2022

bors commented Sep 21, 2022

rust-timer commented Sep 21, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

compiler-errors commented Sep 21, 2022

Mark-Simulacrum commented Sep 27, 2022

compiler-errors commented Oct 4, 2022

lcnr commented Oct 4, 2022

a fn pointer doesn't implement `Fn`/`FnMut`/`FnOnce` if its return type isn't sized #100096

a fn pointer doesn't implement `Fn`/`FnMut`/`FnOnce` if its return type isn't sized #100096

compiler-errors commented Aug 3, 2022 •

edited

Loading