Diagnose partial applied to final types.
#5744
Conversation
Is it worth having a distinct diagnostic or phrasing for non-class types (like tuples, structs, pointers, etc), also for declared-but-not-defined class types (where we can't tell if they're final or not)? Happy to add it, but not sure how much detail to put in here at this stage at least. I chose "non-final type" as somewhat vague wording so it sort of applies even to pointers/tuples/structs.
There was a problem hiding this comment.
Looks good, thanks!
I think it'd also be good to test:
fn F[T:! type](p: partial T*) {}
fn G[template T:! type](p: partial T*) {}... which I think will both be rejected because we claim that T "is a final type". We probably ought to have TODOs for both: in the former case, we're rejecting not because it is a final type but because it might be a final type. And the latter case we should actually accept.
| @@ -54,7 +62,12 @@ library "[[@TEST_NAME]]"; | |||
|
|
|||
| class C; | |||
|
|
|||
| // This diagnostic could be more specific - the type might be non-final, but since we only have a declaration, we don't know. | |||
There was a problem hiding this comment.
I think you should make this comment a TODO. The diagnostic is inaccurate here -- the problem is that the class is not defined, not that it is final.
| @@ -82,6 +103,10 @@ library "[[@TEST_NAME]]"; | |||
| base class C { } | |||
|
|
|||
| //@dump-sem-ir-begin | |||
| // CHECK:STDERR: todo_fail_duplicate.carbon:[[@LINE+4]]:9: error: `partial` applied to final type `partial C` [PartialOnFinal] | |||
There was a problem hiding this comment.
Hmm. Is partial C always a final type? I suppose it is!
I'd expect you get the same kind of error if you try to derive from partial C too?
| SemIR::Class::InheritanceKind::Final) { | ||
| CARBON_DIAGNOSTIC(PartialOnFinal, Error, | ||
| "`partial` applied to final type {0}", SemIR::TypeId); | ||
| context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id), |
There was a problem hiding this comment.
| context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id), | |
| context.emitter().Emit(node_id, |
Drive-by -- is it fair to assume that partial doesn't have children, and so TokenOnly doesn't need to be done here? (I believe it's mainly for nodes with children, when we don't want to include the children)
There was a problem hiding this comment.
I think the T in partial T would be a child, so this'd change the diagnostic from underlining just the partial to underlining the type as well. Maybe that's an improvement though?
There was a problem hiding this comment.
Ahhh, yeah you're right. But yeah, maybe it should because the argument may be less obviously the type in the output.
Is it worth having a distinct diagnostic or phrasing for non-class types
(like tuples, structs, pointers, etc), also for declared-but-not-defined
class types (where we can't tell if they're final or not)? Happy to add
it, but not sure how much detail to put in here at this stage at least.
I chose "non-final type" as somewhat vague wording so it sort of applies
even to pointers/tuples/structs.