-
Notifications
You must be signed in to change notification settings - Fork 13.5k
improve c-variadic errors #143546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
improve c-variadic errors #143546
Conversation
|
tests/ui/c-variadic/valid.rs
Outdated
unsafe extern "C" fn trait_method(&self, mut ap: ...) -> i32 { | ||
unsafe { ap.arg() } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This raises questions about how we should handle the codegen for its self parameter, dyn compatibility, and so on. I would prefer we not get into those for now and continue to reject this case, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So reject in traits if there is a self
parameter of some kind? reject in traits in general?
Previously also just normal associated methods/functions (like in impl S { }
) were rejected, that is unproblematic right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to continue to reject in all of the associated function cases for consistency and visit relaxation of this constraint in a deliberate fashion.
There have already been cases where people who work primarily with more frontend-oriented aspects of the compiler have misunderstood C's varargs as purely some syntactic quirk, and thus e.g. easily moved from function to function, instead of the nightmare it really is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, I'll just add a custom error message then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now generates. At least we have tests now, none of this was exercised apparently.
error: associated functions cannot have a C-variadic argument
--> $DIR/no-associated-function.rs:9:46
|
LL | unsafe extern "C" fn associated_function(mut ap: ...) -> i32 {
|
b2e08e9
to
241fdb9
Compare
// Closures cannot be c-variadic (they can't even be `extern "C"`). | ||
self.dcx().emit_err(errors::CVariadicClosure { span: variadic_param.span }); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently unreachable in practice, the parser already rejects ...
in closures. So, this could be a span_bug
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, span_bug
is only defined in rustc_middle
, so we can't use that here. panic!
or unreachable!
also don't occur in this crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AST is weird on this point, yes.
241fdb9
to
81a4a70
Compare
I think ideally we'd want to reject this in the parser itself (the same way we reject #[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {} Given that this is essentially a stability hole, I think it would probably be ok to break it, but it would need a crater run and a lang FCP. |
Sigh. I hate post-expansion syntax gating. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #143556) made this pull request unmergeable. Please resolve the merge conflicts. |
81a4a70
to
f92ba67
Compare
...
f92ba67
to
8951c42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allright, let's handle _: ...
separately then.
r? @workingjubilee now that this is no longer making any changes that are relevant for T-lang
8951c42
to
2483477
Compare
This comment has been minimized.
This comment has been minimized.
2483477
to
bfc1d8b
Compare
This comment has been minimized.
This comment has been minimized.
bfc1d8b
to
f139dd6
Compare
tracking issue: #44930
Make some errors more specific, and clean up the logic