Split TyBareFn into TyFnDef and TyFnPtr. #26284
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
I'm going to have to tweak this; external functions (like intrinsics) having FnPtrTy is way too confusing. |
|
Okay, should be good now; intrinsic declarations now have type FnDefTy. |
There's a lot of stuff wrong with the representation of these types: TyFnDef doesn't actually uniquely identify a function, TyFnPtr is used to represent method calls, TyFnDef in the sub-expression of a cast isn't correctly reified, and probably some other stuff I haven't discovered yet. Splitting them seems like the right first step, though.
|
It was pointed out on IRC that changing extern declarations from FnPtrTy to FnDefTy is a breaking change. You'd have to be writing sort of weird-looking code to actually hit an error message, though: it's impossible to explicitly write an FnDefTy, so you could only trigger it by doing something like "let mut a = memcpy; a = memmove;". |
|
@eefriedman it's more likely to happen with a conditional, e.g. let my_fn = if debug_enabled {
my_fn_debug
} else {
my_fn_fast
};
my_fn(...); |
|
☔ The latest upstream changes (presumably #26351) made this pull request unmergeable. Please resolve the merge conflicts. |
|
RFC 1122 discusses the handling of breaking changes, but I do not know whether to categorize this as a "small change" or not. I suspect that examples such as the one described by @eddyb come up more often than you might think... As a reference, @nikomatsakis went through a fair amount of future-proofing to lay the foundation for the the object defaults change in #26370 In my gut, I'm inclined to recommend that we do the same here... i.e. that we adopt a strategy of first trying to detect cases that will eventually be errors and emit warnings for them in the short term. But its possible others think that would be overkill. cc @rust-lang/lang |
|
There enough weirdness here that it should probably get cleaned up before I try to rename anything, with both the handling of extern fns and method type handling. I'm not sure how I would even about reliably detecting the unification of the types of different extern functions. I guess I'll leave this open for now, but I'm really not sure what to do with it. |
|
A couple of things: (1) I think this is good work, but this PR doesn't really go as far as I'd like. That is, I'd like to actually implement the zero-sized types change. Doing the work that this PR does is a good first step though. This change is (slightly) breaking in some cases. (2) I think we should definitely evaluate the impact; I am not 100% certain whether we should go forward with a warning period or what. Unlike the default object lifetime bounds, this work falls under the category of bug or soundness fix (in particular, issue #19925.) But of course this doesn't make much difference to the end-user experience if it causes your code to break. Probably it would be best to phase in the changes with a warning cycle, but at minimum we should ensure we get very clear error messages. |
|
@nikomatsakis we could also unify the results of conditionals (and maybe array elements) by coercing to a "common reified form", which should alleviate most of the possible fallout. |
|
@nikomatsakis I thought your comments on #19925 implied that this would not be a soundness fix, since under the current implementation, the hole (pseudo-hole?) in the type system is not actually exploitable since we are still passing the actual functions to invoke (post-monomorphization) Is there a different example that actually demonstrates unsoundness, as opposed to code that we would like to reject today but do not (and doesn't actually behave unsoundly when it is run) ? |
|
This doesn't seem like a soundness issue to me - "TyFnDef" includes the function def-id. Also, "TyFnPtr" is mostly like a trait object, but it has some different representability properties so I don't think we can merge them. |
| @@ -612,7 +612,7 @@ fn check_expr<'a, 'tcx>(v: &mut CheckCrateVisitor<'a, 'tcx>, | |||
| v.add_qualif(ConstQualif::NON_ZERO_SIZED); | |||
| } | |||
| Some(def::DefStruct(_)) => { | |||
| if let ty::TyBareFn(..) = node_ty.sty { | |||
| if let ty::TyFnDef(..) = node_ty.sty { | |||
I was using "soundness" a bit generously -- that is, I think that the current treatment is a legitimate bug in the type-checker, causing it to accept some programs that ought not to be accepted. I am not saying that those programs can cause undefined behavior. |
|
What's the status of this PR? |
|
This is on hold because it's a breaking change with regards to foreign functions, and the discussion of how to deal with that never settled on a solution. |
|
@eefriedman ping? Should this be moved forward? Closed? |
|
I'll close it for now, given that there isn't likely to be any movement in the near future. |
Distinguish fn item types to allow reification from nothing to fn pointers. The first commit is a rebase of #26284, except for files that have moved since. This is a [breaking-change], due to: * each FFI function has a distinct type, like all other functions currently do * all generic parameters on functions are recorded in their item types, e.g.: `size_of::<u8>` & `size_of::<i8>`'s types differ despite their identical signature. * function items are zero-sized, which will stop transmutes from working on them The first two cases are handled in most cases with the new coerce-unify logic, which will combine incompatible function item types into function pointers, at the outer-most level of if-else chains, match arms and array literals. The last case is specially handled during type-checking such that transmutes from a function item type to a pointer or integer type will continue to work for another release cycle, but are being linted against. To get rid of warnings and ensure your code will continue to compile, cast to a pointer before transmuting.
There's a lot of stuff wrong with the representation of these types:
TyFnDef doesn't actually uniquely identify a function, TyFnPtr is used to
represent method calls, TyFnDef in the sub-expression of a cast isn't
correctly reified, and probably some other stuff I haven't discovered yet.
Splitting them seems like the right first step, though.