Deny more ~const trait bounds#117817
Conversation
|
Does this fix any of these? :D |
fee1-dead
left a comment
There was a problem hiding this comment.
personally speaking I'd rather make Some(DisalloweTildeConstContext) the default
sure, let's do that!
e345737 to
2fe9827
Compare
No, it doesn't :| |
2fe9827 to
8ce5d78
Compare
|
|
||
| self.visit_vis(&item.vis); | ||
| self.visit_ident(item.ident); | ||
| self.with_tilde_const(None, |this| this.visit_generics(generics)); |
There was a problem hiding this comment.
Currently permitting ~const trait bounds on inherent impls not to regress rfc-2632-const-trait-impl/tilde_const_on_impl_bound.rs. Not sure if the following code should compile at some point:
struct Struct;
#[const_trait] trait Trait {}
impl<T: ~const Trait> Struct {
const fn f(_: T) {}
}If I were to straight up deny ~const here, it'd obviously fix #117004. I haven't tested yet if the ~const lowering code can properly lower the code above.
There was a problem hiding this comment.
We should probably deny this first. We don't really need this to work and making it work certainly adds to the overhead. Perhaps a followup PR? Or I can do that. Thanks for working on this! :)
There was a problem hiding this comment.
I think we should deny ~const here. If we want to be able to use Trait in const fn f, I think the user should need to write:
struct Struct;
#[const_trait] trait Trait {}
impl<T: Trait> Struct {
const fn f(_: T) where T: ~const Trait {}
}
at least in my opinion.
| .trait = this trait is not a `#[const_trait]`, so it cannot have `~const` trait bounds | ||
| .impl = this impl is not `const`, so it cannot have `~const` trait bounds | ||
| .object = trait objects cannot have `~const` trait bounds | ||
| .item = this item cannot have `~const` trait bounds |
There was a problem hiding this comment.
This is quite ambiguous and we'd probably always want to have a reason.. OTOH, we don't want to allow tilde const for these "unknown" cases. Would be nice if this was turned into a span_bug! so that a more specific context is always defined.
No need for this PR to have this though.
|
@bors r+ |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a04d56b): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.671s -> 675.185s (0.08%) |
|
perf regression looks kinda spurious |
…headers, r=fee1-dead Deny `~const` trait bounds in inherent impl headers Follow-up to rust-lang#117817. Implements rust-lang#117817 (comment). Fixes rust-lang#117004. r? fee1-dead or compiler
Rollup merge of rust-lang#119059 - fmease:no-tilde-const-in-inh-impl-headers, r=fee1-dead Deny `~const` trait bounds in inherent impl headers Follow-up to rust-lang#117817. Implements rust-lang#117817 (comment). Fixes rust-lang#117004. r? fee1-dead or compiler
…r=fee1-dead Deny `~const` trait bounds in inherent impl headers Follow-up to #117817. Implements rust-lang/rust#117817 (comment). Fixes #117004. r? fee1-dead or compiler
thereby fixing a family of ICEs (delayed bugs) for
feature(const_trait_impl, effects)code.As discussed
r? @fee1-dead