Improve C-variadic error messages: part 2 #146342
Conversation
there is no reason this should not work, really, we're just cutting some scope for now
rustbot
added
S-waiting-on-review
|
Thank you, aside from the nitpicking this is a pretty good PR. |
folkertdev
force-pushed
the
c-variadic-errors-take-3
branch
from
9f3e568 to
9196844
Compare
|
|
||
| #[suggestion( | ||
| ast_passes_suggestion, | ||
| applicability = "maybe-incorrect", |
There was a problem hiding this comment.
This seems machine-applicable? It is a direct fixup. The only other working solution is to remove the varargs.
It may lead to the code not compiling because the callers aren't in unsafe {}, but that's fine. Intentional, even.
Unless the main justification is to prevent the risk of such calls silently appearing in an unsafe {}? But I'm not sure how much that is a risk, as the code already doesn't compile. I suppose after refactoring? Someone could be refactoring a fixed args function to variable arguments, and it's used in unsafe {} incidentally, then they get a fixup like this without looking... hmm, yeah, I guess that's a risk. I suppose I talked myself into justifying the current state!
There was a problem hiding this comment.
Also, adding unsafe to a function declaration should likely be accompanied by the addition of a safety doc comment, and having to manually fix the error makes it more likely that you will do that too
There was a problem hiding this comment.
That's the sort of linting that clippy is good at enforcing (and despite my occasional grumbling in clippy's direction, lints on unsafety are one of the clear winners for clippy usage, IMO), so I'm less concerned at that level.
|
Yeah, that works! Thinking: We're not doing any manual manipulation of spans still aside from picking edges, so this shouldn't run afoul of the classic problems with spans, as far as I am aware. I'm expecting issues, if any, to appear at the integration edges with tooling that works on suggestions, and not in rustc. So this code is fine in rustc, I believe. Thanks! @bors r+ rollup |
bors
added
S-waiting-on-bors
| async unsafe extern "C" fn multiple_named_lifetimes<'a, 'b>(_: u8, ...) {} | ||
| //~^ ERROR hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds | ||
| //~^ ERROR functions cannot be both `async` and C-variadic | ||
| //~| ERROR hidden type for `impl Future<Output = ()>` captures lifetime that does not appear in bounds |
There was a problem hiding this comment.
This is an interesting quirk: varargs appear to interact with lifetimes in such a way that, plus async, this creates a bunch of hidden captured lifetimes? I think this might depend on varargs and not require async but I think all the other ways to generate this interaction with hidden types are unstable anyways.
There was a problem hiding this comment.
Interesting, I believe that is what is actually happening here too #132142, where we actually run into an ICE. Anyhow the fix there is to disallow function definitions with that ABI, which should fix it I think?
But it does look like some part of the type checker doesn't properly account for c-variadics. It also interacts specifically with the async machinery, because this desugared version does not emit errors.
unsafe extern "C" fn multiple_named_lifetimes2<'a, 'b>(_: u8, ...) -> impl std::future::Future<Output = ()> {
async {}
}There was a problem hiding this comment.
#125431 has some extra info
There was a problem hiding this comment.
Yeah the desugared version doesn't have the same lifetime-capturing rules, I believe. It seems to Need to be an opaque type.
There was a problem hiding this comment.
"but Jubilee, the opaque type is right there, impl Something, you even tried to do the same with a different trait!"
...uhm, hmm.
Right, my brain started churning over "something with use<'???> maybe?" but then I couldn't find it and eventually concluded it was the compiler interacting with something we couldn't name as a programmer.
There was a problem hiding this comment.
Something like that, because this also just works
unsafe extern "C" fn multiple_named_lifetimes2<'a, 'b>(_: u8, ...) -> impl std::future::Future<Output = ()> + 'a + 'b {
async {}
}so I'm guessing the desugared version has the async {} itself capture the lifetime?
…3, r=workingjubilee Improve C-variadic error messages: part 2 tracking issue: rust-lang#44930 a reimplementation of rust-lang#143546 that builds on rust-lang#146165. This PR - disallows coroutines (e.g. `async fn`) from having a `...` argument - disallows associated functions (both in traits and standard impl blocks) from having a `...` argument - splits up a generic "ill-formed C-variadic function" into specific errors about using an incorrect ABI, not specifying an ABI, or missing the unsafe keyword C-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird. For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message. Made to be reviewed commit-by-commit. cc `@workingjubilee` r? compiler
…3, r=workingjubilee Improve C-variadic error messages: part 2 tracking issue: rust-lang#44930 a reimplementation of rust-lang#143546 that builds on rust-lang#146165. This PR - disallows coroutines (e.g. `async fn`) from having a `...` argument - disallows associated functions (both in traits and standard impl blocks) from having a `...` argument - splits up a generic "ill-formed C-variadic function" into specific errors about using an incorrect ABI, not specifying an ABI, or missing the unsafe keyword C-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird. For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message. Made to be reviewed commit-by-commit. cc ``@workingjubilee`` r? compiler
Rollup of 6 pull requests Successful merges: - #146311 (Minor symbol comment fixes.) - #146340 (Strip frontmatter in fewer places) - #146342 (Improve C-variadic error messages: part 2) - #146347 (report duplicate symbols added by the driver) - #146374 (Update `browser-ui-test` version to `0.22.2`) - #146379 (Fix `compare_against_sw_vers` test) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 8 pull requests Successful merges: - #145327 (std: make address resolution weirdness local to SGX) - #145879 (default auto traits: use default supertraits instead of `Self: Trait` bounds on associated items) - #146123 (Suggest examples of format specifiers in error messages) - #146311 (Minor symbol comment fixes.) - #146322 (Make Barrier RefUnwindSafe again) - #146327 (Add tests for deref on pin) - #146340 (Strip frontmatter in fewer places) - #146342 (Improve C-variadic error messages: part 2) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #146342 - folkertdev:c-variadic-errors-take-3, r=workingjubilee Improve C-variadic error messages: part 2 tracking issue: #44930 a reimplementation of #143546 that builds on #146165. This PR - disallows coroutines (e.g. `async fn`) from having a `...` argument - disallows associated functions (both in traits and standard impl blocks) from having a `...` argument - splits up a generic "ill-formed C-variadic function" into specific errors about using an incorrect ABI, not specifying an ABI, or missing the unsafe keyword C-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird. For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message. Made to be reviewed commit-by-commit. cc `@workingjubilee` r? compiler
…hods, r=workingjubilee c-variadic: allow c-variadic inherent and trait methods tracking issue: rust-lang#44930 Continuing the work of rust-lang#146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible. There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a `dyn` object are cast to a function pointer, a `ReifyShim` is created. If that shim is c-variadic, it would need to forward the C variable argument list. That does appear to work, because the `va_list` is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation. Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with `dyn` objects at all? We can revisit this limitation if a need arises. r? `@workingjubilee`
…kingjubilee c-variadic: allow c-variadic inherent and trait methods tracking issue: #44930 Continuing the work of #146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible. There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a `dyn` object are cast to a function pointer, a `ReifyShim` is created. If that shim is c-variadic, it would need to forward the C variable argument list. That does appear to work, because the `va_list` is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation. Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with `dyn` objects at all? We can revisit this limitation if a need arises. r? `@workingjubilee`
…hods, r=workingjubilee c-variadic: allow c-variadic inherent and trait methods tracking issue: rust-lang#44930 Continuing the work of rust-lang#146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible. There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a `dyn` object are cast to a function pointer, a `ReifyShim` is created. If that shim is c-variadic, it would need to forward the C variable argument list. That does appear to work, because the `va_list` is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation. Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with `dyn` objects at all? We can revisit this limitation if a need arises. r? `@workingjubilee`
Rollup merge of #146434 - folkertdev:c-variadic-inherent-methods, r=workingjubilee c-variadic: allow c-variadic inherent and trait methods tracking issue: #44930 Continuing the work of #146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible. There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a `dyn` object are cast to a function pointer, a `ReifyShim` is created. If that shim is c-variadic, it would need to forward the C variable argument list. That does appear to work, because the `va_list` is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation. Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with `dyn` objects at all? We can revisit this limitation if a need arises. r? `@workingjubilee`
tracking issue: #44930
a reimplementation of #143546 that builds on #146165.
This PR
async fn) from having a...argument...argumentC-variadic coroutines probably don't make sense? C-variadic functions are for FFI purposes, combining that with async functions seems weird.
For associated functions, we're just cutting scope. It's probably fine, but it's probably better to explicitly allow it. So for now, at least give a more targeted error message.
Made to be reviewed commit-by-commit.
cc @workingjubilee
r? compiler