Disallow creating instances of abstract classes #4381
Conversation
Taking notes here: Newly added diagnostics (+ tested, - untested): ClassAbstractHere (note, tested in several paths) AbstractTypeInAdaptDecl+ (not sure if this is correct - is it valid to adapt an abstract type?) AbstractTypeInConversion- (equivalent IncompleteTypeInConversion is also untested, so I'm not sure if/how to reach this) AbstractTypeInFunctionParam+ AbstractTypeInFunctionReturnType- (Failures about "initialization of abstract type" seem to block the return type error? That doesn't seem as useful as actually erroring on the return type) AbstractTypeInInit++ AbstractTypeInLetDecl+ AbstractTypeInValueConversion++ AbstractTypeInVarDecl++ (Field and Variable)
There was a problem hiding this comment.
The intended model is that you cannot create objects of abstract class types, but you can have values of abstract class types -- the value representation in that case is a pointer to the instance, so it doesn't need the class to be non-abstract. So I think we would want to remove some of these checks; I'll point out which ones.
toolchain/check/convert.cpp
Outdated
| CARBON_DIAGNOSTIC(AbstractTypeInValueConversion, Error, | ||
| "forming value of abstract type `{0}`", | ||
| SemIR::TypeId); | ||
| CARBON_DIAGNOSTIC(AbstractTypeInConversion, Error, | ||
| "invalid use of abstract type `{0}`", | ||
| SemIR::TypeId); |
There was a problem hiding this comment.
I think we don't want to diagnose in these two cases, only when initializing.
There was a problem hiding this comment.
Done (this also included adding null DiagnosticBuilder - open to riffing on the API (currently just uses the default ctor to create a null DiagnosticBuilder, but could use a named ctor of some kind - and has explicit bool conversion which could be some more explicitly named function))
There was a problem hiding this comment.
I think a named constructor would be nice. Maybe even a method on DiagnosticEmitter, say BuildSuppressed or something, so you don't need to repeat the template argument.
…Conversion This also adds an invalid/null/empty state to DiagnosticBuilder so this callback can then return the null DiagnosticBuilder for the cases that don't need errors.
Co-authored-by: Richard Smith <[email protected]>
There was a problem hiding this comment.
Handled feedback, and did a more major pass on the test coverage, splitting things (I feel like maybe I /should/ be writing those tests differently to, for example, share the definition of the abstract class rather than redefining it in even split - though it's so short importing it doesn't seem much more legible either - totally open to suggestions there).
Did manage to get test coverage on the return type check (at a call site to, rather than a definition of the abstract-returning function).
toolchain/check/convert.cpp
Outdated
| CARBON_DIAGNOSTIC(AbstractTypeInValueConversion, Error, | ||
| "forming value of abstract type `{0}`", | ||
| SemIR::TypeId); | ||
| CARBON_DIAGNOSTIC(AbstractTypeInConversion, Error, | ||
| "invalid use of abstract type `{0}`", | ||
| SemIR::TypeId); |
There was a problem hiding this comment.
Done (this also included adding null DiagnosticBuilder - open to riffing on the API (currently just uses the default ctor to create a null DiagnosticBuilder, but could use a named ctor of some kind - and has explicit bool conversion which could be some more explicitly named function))
| @@ -55,6 +55,7 @@ class DiagnosticEmitter { | |||
| DiagnosticBuilder(DiagnosticBuilder&&) noexcept = default; | |||
| auto operator=(DiagnosticBuilder&&) noexcept | |||
| -> DiagnosticBuilder& = default; | |||
| DiagnosticBuilder() : emitter_(nullptr) {} | |||
There was a problem hiding this comment.
A brief comment here would be handy.
toolchain/check/convert.cpp
Outdated
| CARBON_DIAGNOSTIC(AbstractTypeInValueConversion, Error, | ||
| "forming value of abstract type `{0}`", | ||
| SemIR::TypeId); | ||
| CARBON_DIAGNOSTIC(AbstractTypeInConversion, Error, | ||
| "invalid use of abstract type `{0}`", | ||
| SemIR::TypeId); |
There was a problem hiding this comment.
I think a named constructor would be nice. Maybe even a method on DiagnosticEmitter, say BuildSuppressed or something, so you don't need to repeat the template argument.
A good first-pass, at least. (abstract adapters are rejected with this change, though pending further language design discussion)