Skip to content
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

Disallow abstract or base on class declarations (that are not definitions) #4378

Merged
23 changes: 7 additions & 16 deletions toolchain/check/handle_class.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,6 @@ static auto MergeClassRedecl(Context& context, SemIRLoc new_loc,
return false;
}

// The introducer kind must match the previous declaration.
// TODO: The rule here is not yet decided. See #3384.
if (prev_class.inheritance_kind != new_class.inheritance_kind) {
CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducer, Error,
"class redeclared with different inheritance kind");
CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducerPrevious, Note,
"previously declared here");
context.emitter()
.Build(new_loc, ClassRedeclarationDifferentIntroducer)
.Note(prev_loc, ClassRedeclarationDifferentIntroducerPrevious)
.Emit();
}

if (new_is_definition) {
prev_class.MergeDefinition(new_class);
prev_class.scope_id = new_class.scope_id;
Expand Down Expand Up @@ -194,9 +181,13 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id,
auto introducer =
context.decl_introducer_state_stack().Pop<Lex::TokenKind::Class>();
CheckAccessModifiersOnDecl(context, introducer, parent_scope_inst);
LimitModifiersOnDecl(context, introducer,
KeywordModifierSet::Class | KeywordModifierSet::Access |
KeywordModifierSet::Extern);
auto accepted_modifiers =
KeywordModifierSet::Access | KeywordModifierSet::Extern;
auto invalid_unless_definition_modifiers = KeywordModifierSet::None;
(is_definition ? accepted_modifiers : invalid_unless_definition_modifiers) |=
KeywordModifierSet::Class;
LimitModifiersOnDecl(context, introducer, accepted_modifiers,
invalid_unless_definition_modifiers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this a bit hard to read. I wonder if it would be simpler to introduce another function for this case? Maybe something like:

Suggested change
auto accepted_modifiers =
KeywordModifierSet::Access | KeywordModifierSet::Extern;
auto invalid_unless_definition_modifiers = KeywordModifierSet::None;
(is_definition ? accepted_modifiers : invalid_unless_definition_modifiers) |=
KeywordModifierSet::Class;
LimitModifiersOnDecl(context, introducer, accepted_modifiers,
invalid_unless_definition_modifiers);
LimitModifiersOnDecl(context, introducer,
KeywordModifierSet::Access | KeywordModifierSet::Extern |
KeywordModifierSet::Class);
if (!is_definition) {
LimitModifiersNotDefinition(context, introducer,
KeywordModifierSet::Access | KeywordModifierSet::Extern);
}

The only difference between the two would be the diagnostic issued, so they could mostly share implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, yeah - that does simplify things greatly! (done)

RestrictExternModifierOnDecl(context, introducer, parent_scope_inst,
is_definition);

Expand Down
24 changes: 19 additions & 5 deletions toolchain/check/modifiers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,22 @@
namespace Carbon::Check {

static auto DiagnoseNotAllowed(Context& context, Parse::NodeId modifier_node,
bool allowed_in_definition,
Lex::TokenKind decl_kind,
llvm::StringRef context_string,
SemIR::LocId context_loc_id) -> void {
CARBON_DIAGNOSTIC(ModifierNotAllowedOn, Error,
"`{0}` not allowed on `{1}` declaration{2}", Lex::TokenKind,
Lex::TokenKind, std::string);
auto diag = context.emitter().Build(modifier_node, ModifierNotAllowedOn,
context.token_kind(modifier_node),
decl_kind, context_string.str());
CARBON_DIAGNOSTIC(
ModifierNotAllowedOnDeclOnly, Error,
"`{0}` not allowed on `{1}` declaration, only definition{2}",
Lex::TokenKind, Lex::TokenKind, std::string);
auto diag = context.emitter().Build(
modifier_node,
allowed_in_definition ? ModifierNotAllowedOnDeclOnly
: ModifierNotAllowedOn,
context.token_kind(modifier_node), decl_kind, context_string.str());
if (context_loc_id.is_valid()) {
CARBON_DIAGNOSTIC(ModifierNotInContext, Note, "containing definition here");
diag.Note(context_loc_id, ModifierNotInContext);
Expand All @@ -39,8 +46,12 @@ static auto ModifierOrderAsSet(ModifierOrder order) -> KeywordModifierSet {

auto ForbidModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
KeywordModifierSet forbidden,
KeywordModifierSet allowed_on_definition,
llvm::StringRef context_string,
SemIR::LocId context_loc_id) -> void {
CARBON_CHECK((allowed_on_definition & ~forbidden).empty(),
"allowed_on_definition modifiers must only be a subset of those "
"already forbidden");
auto not_allowed = introducer.modifier_set & forbidden;
if (not_allowed.empty()) {
return;
Expand All @@ -50,8 +61,10 @@ auto ForbidModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
order_index <= static_cast<int8_t>(ModifierOrder::Last); ++order_index) {
auto order = static_cast<ModifierOrder>(order_index);
if (not_allowed.HasAnyOf(ModifierOrderAsSet(order))) {
DiagnoseNotAllowed(context, introducer.modifier_node_id(order),
introducer.kind, context_string, context_loc_id);
DiagnoseNotAllowed(
context, introducer.modifier_node_id(order),
allowed_on_definition.HasAnyOf(ModifierOrderAsSet(order)),
introducer.kind, context_string, context_loc_id);
introducer.set_modifier_node_id(order, Parse::NodeId::Invalid);
}
}
Expand All @@ -71,6 +84,7 @@ auto CheckAccessModifiersOnDecl(Context& context,
// scope.
ForbidModifiersOnDecl(
context, introducer, KeywordModifierSet::Protected,
KeywordModifierSet::None,
" at file scope, `protected` is only allowed on class members");
return;
}
Expand Down
25 changes: 20 additions & 5 deletions toolchain/check/modifiers.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,35 @@ auto CheckMethodModifiersOnFunction(

// Like `LimitModifiersOnDecl`, except says which modifiers are forbidden, and a
// `context_string` (and optional `context_loc_id`) specifying the context in
// which those modifiers are forbidden.
// which those modifiers are forbidden. `allowed_on_definition` specifies
// modifiers that are forbidden, but would be valid if the declaration was a
// definition.
// TODO: Take another look at diagnostic phrasing for callers.
auto ForbidModifiersOnDecl(Context& context, DeclIntroducerState& introducer,
KeywordModifierSet forbidden,
KeywordModifierSet allowed_on_definition,
llvm::StringRef context_string,
SemIR::LocId context_loc_id = SemIR::LocId::Invalid)
-> void;

inline auto ForbidModifiersOnDecl(
Context& context, DeclIntroducerState& introducer,
KeywordModifierSet forbidden, llvm::StringRef context_string,
SemIR::LocId context_loc_id = SemIR::LocId::Invalid) -> void {
ForbidModifiersOnDecl(context, introducer, forbidden,
KeywordModifierSet::None, context_string,
context_loc_id);
}

// Reports a diagnostic (using `decl_kind`) if modifiers on this declaration are
// not in `allowed`. Updates `introducer`.
inline auto LimitModifiersOnDecl(Context& context,
DeclIntroducerState& introducer,
KeywordModifierSet allowed) -> void {
ForbidModifiersOnDecl(context, introducer, ~allowed, "");
inline auto LimitModifiersOnDecl(
Context& context, DeclIntroducerState& introducer,
KeywordModifierSet allowed,
KeywordModifierSet allowed_on_definition = KeywordModifierSet::None)
-> void {
ForbidModifiersOnDecl(context, introducer, ~allowed, allowed_on_definition,
"");
}

// Restricts the `extern` modifier to only be used on namespace-scoped
Expand Down
71 changes: 41 additions & 30 deletions toolchain/check/testdata/class/fail_abstract.carbon
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,22 @@ class Derived {

fn Make() -> Derived {
// TODO: This should be valid, and should construct an instance of `partial Abstract` as the base.
// CHECK:STDERR: fail_abstract.carbon:[[@LINE+3]]:19: error: cannot construct instance of abstract class; consider using `partial Abstract` instead
// CHECK:STDERR: fail_abstract.carbon:[[@LINE+4]]:19: error: cannot construct instance of abstract class; consider using `partial Abstract` instead
// CHECK:STDERR: return {.base = {.a = 1}, .d = 7};
// CHECK:STDERR: ^~~~~~~~
// CHECK:STDERR:
return {.base = {.a = 1}, .d = 7};
}

fn Access(d: Derived) -> (i32, i32) {
return (d.d, d.base.a);
}

// CHECK:STDERR: fail_abstract.carbon:[[@LINE+3]]:1: error: `abstract` not allowed on `class` declaration, only definition
// CHECK:STDERR: abstract class AbstractDecl;
// CHECK:STDERR: ^~~~~~~~
abstract class AbstractDecl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in another file split. The ideal here is that each should-fail test is in its own split, so accidentally succeeding is considered an error. In this case, the previous test should really be a "fail_todo" test, that is expected to succeed when the feature is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks - sure, had a go at adding some splits.


// CHECK:STDOUT: --- fail_abstract.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
Expand Down Expand Up @@ -59,6 +65,7 @@ fn Access(d: Derived) -> (i32, i32) {
// CHECK:STDOUT: %Access.type: type = fn_type @Access [template]
// CHECK:STDOUT: %Access: %Access.type = struct_value () [template]
// CHECK:STDOUT: %.18: type = ptr_type %.17 [template]
// CHECK:STDOUT: %AbstractDecl: type = class_type @AbstractDecl [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: imports {
Expand All @@ -83,6 +90,7 @@ fn Access(d: Derived) -> (i32, i32) {
// CHECK:STDOUT: .Derived = %Derived.decl
// CHECK:STDOUT: .Make = %Make.decl
// CHECK:STDOUT: .Access = %Access.decl
// CHECK:STDOUT: .AbstractDecl = %AbstractDecl.decl
// CHECK:STDOUT: }
// CHECK:STDOUT: %Core.import = import Core
// CHECK:STDOUT: %Abstract.decl: type = class_decl @Abstract [template = constants.%Abstract] {} {}
Expand All @@ -97,16 +105,17 @@ fn Access(d: Derived) -> (i32, i32) {
// CHECK:STDOUT: %Derived.ref: type = name_ref Derived, file.%Derived.decl [template = constants.%Derived]
// CHECK:STDOUT: %d.param: %Derived = param d, runtime_param0
// CHECK:STDOUT: %d: %Derived = bind_name d, %d.param
// CHECK:STDOUT: %int.make_type_32.loc29_27: init type = call constants.%Int32() [template = i32]
// CHECK:STDOUT: %int.make_type_32.loc29_32: init type = call constants.%Int32() [template = i32]
// CHECK:STDOUT: %.loc29_35.1: %.16 = tuple_literal (%int.make_type_32.loc29_27, %int.make_type_32.loc29_32)
// CHECK:STDOUT: %.loc29_35.2: type = value_of_initializer %int.make_type_32.loc29_27 [template = i32]
// CHECK:STDOUT: %.loc29_35.3: type = converted %int.make_type_32.loc29_27, %.loc29_35.2 [template = i32]
// CHECK:STDOUT: %.loc29_35.4: type = value_of_initializer %int.make_type_32.loc29_32 [template = i32]
// CHECK:STDOUT: %.loc29_35.5: type = converted %int.make_type_32.loc29_32, %.loc29_35.4 [template = i32]
// CHECK:STDOUT: %.loc29_35.6: type = converted %.loc29_35.1, constants.%.17 [template = constants.%.17]
// CHECK:STDOUT: %int.make_type_32.loc30_27: init type = call constants.%Int32() [template = i32]
// CHECK:STDOUT: %int.make_type_32.loc30_32: init type = call constants.%Int32() [template = i32]
// CHECK:STDOUT: %.loc30_35.1: %.16 = tuple_literal (%int.make_type_32.loc30_27, %int.make_type_32.loc30_32)
// CHECK:STDOUT: %.loc30_35.2: type = value_of_initializer %int.make_type_32.loc30_27 [template = i32]
// CHECK:STDOUT: %.loc30_35.3: type = converted %int.make_type_32.loc30_27, %.loc30_35.2 [template = i32]
// CHECK:STDOUT: %.loc30_35.4: type = value_of_initializer %int.make_type_32.loc30_32 [template = i32]
// CHECK:STDOUT: %.loc30_35.5: type = converted %int.make_type_32.loc30_32, %.loc30_35.4 [template = i32]
// CHECK:STDOUT: %.loc30_35.6: type = converted %.loc30_35.1, constants.%.17 [template = constants.%.17]
// CHECK:STDOUT: %return: ref %.17 = var <return slot>
// CHECK:STDOUT: }
// CHECK:STDOUT: %AbstractDecl.decl: type = class_decl @AbstractDecl [template = constants.%AbstractDecl] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @Abstract {
Expand Down Expand Up @@ -137,37 +146,39 @@ fn Access(d: Derived) -> (i32, i32) {
// CHECK:STDOUT: extend name_scope2
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @AbstractDecl;
// CHECK:STDOUT:
// CHECK:STDOUT: fn @Int32() -> type = "int.make_type_32";
// CHECK:STDOUT:
// CHECK:STDOUT: fn @Make() -> %return: %Derived {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: %.loc26_25: i32 = int_literal 1 [template = constants.%.13]
// CHECK:STDOUT: %.loc26_26: %.3 = struct_literal (%.loc26_25)
// CHECK:STDOUT: %.loc26_34: i32 = int_literal 7 [template = constants.%.14]
// CHECK:STDOUT: %.loc26_35: %.15 = struct_literal (%.loc26_26, %.loc26_34)
// CHECK:STDOUT: %.loc27_25: i32 = int_literal 1 [template = constants.%.13]
// CHECK:STDOUT: %.loc27_26: %.3 = struct_literal (%.loc27_25)
// CHECK:STDOUT: %.loc27_34: i32 = int_literal 7 [template = constants.%.14]
// CHECK:STDOUT: %.loc27_35: %.15 = struct_literal (%.loc27_26, %.loc27_34)
// CHECK:STDOUT: return <error> to %return
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @Access(%d: %Derived) -> %return: %.17 {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: %d.ref.loc30_11: %Derived = name_ref d, %d
// CHECK:STDOUT: %d.ref.loc30_12: %.7 = name_ref d, @Derived.%.loc18_8 [template = @Derived.%.loc18_8]
// CHECK:STDOUT: %.loc30_12.1: ref i32 = class_element_access %d.ref.loc30_11, element1
// CHECK:STDOUT: %.loc30_12.2: i32 = bind_value %.loc30_12.1
// CHECK:STDOUT: %d.ref.loc30_16: %Derived = name_ref d, %d
// CHECK:STDOUT: %d.ref.loc31_11: %Derived = name_ref d, %d
// CHECK:STDOUT: %d.ref.loc31_12: %.7 = name_ref d, @Derived.%.loc18_8 [template = @Derived.%.loc18_8]
// CHECK:STDOUT: %.loc31_12.1: ref i32 = class_element_access %d.ref.loc31_11, element1
// CHECK:STDOUT: %.loc31_12.2: i32 = bind_value %.loc31_12.1
// CHECK:STDOUT: %d.ref.loc31_16: %Derived = name_ref d, %d
// CHECK:STDOUT: %base.ref: %.6 = name_ref base, @Derived.%.loc16 [template = @Derived.%.loc16]
// CHECK:STDOUT: %.loc30_17.1: ref %Abstract = class_element_access %d.ref.loc30_16, element0
// CHECK:STDOUT: %.loc30_17.2: %Abstract = bind_value %.loc30_17.1
// CHECK:STDOUT: %.loc31_17.1: ref %Abstract = class_element_access %d.ref.loc31_16, element0
// CHECK:STDOUT: %.loc31_17.2: %Abstract = bind_value %.loc31_17.1
// CHECK:STDOUT: %a.ref: %.2 = name_ref a, @Abstract.%.loc12_8 [template = @Abstract.%.loc12_8]
// CHECK:STDOUT: %.loc30_22.1: ref i32 = class_element_access %.loc30_17.2, element0
// CHECK:STDOUT: %.loc30_22.2: i32 = bind_value %.loc30_22.1
// CHECK:STDOUT: %.loc30_24.1: %.17 = tuple_literal (%.loc30_12.2, %.loc30_22.2)
// CHECK:STDOUT: %.loc30_24.2: ref i32 = tuple_access %return, element0
// CHECK:STDOUT: %.loc30_24.3: init i32 = initialize_from %.loc30_12.2 to %.loc30_24.2
// CHECK:STDOUT: %.loc30_24.4: ref i32 = tuple_access %return, element1
// CHECK:STDOUT: %.loc30_24.5: init i32 = initialize_from %.loc30_22.2 to %.loc30_24.4
// CHECK:STDOUT: %.loc30_24.6: init %.17 = tuple_init (%.loc30_24.3, %.loc30_24.5) to %return
// CHECK:STDOUT: %.loc30_25: init %.17 = converted %.loc30_24.1, %.loc30_24.6
// CHECK:STDOUT: return %.loc30_25 to %return
// CHECK:STDOUT: %.loc31_22.1: ref i32 = class_element_access %.loc31_17.2, element0
// CHECK:STDOUT: %.loc31_22.2: i32 = bind_value %.loc31_22.1
// CHECK:STDOUT: %.loc31_24.1: %.17 = tuple_literal (%.loc31_12.2, %.loc31_22.2)
// CHECK:STDOUT: %.loc31_24.2: ref i32 = tuple_access %return, element0
// CHECK:STDOUT: %.loc31_24.3: init i32 = initialize_from %.loc31_12.2 to %.loc31_24.2
// CHECK:STDOUT: %.loc31_24.4: ref i32 = tuple_access %return, element1
// CHECK:STDOUT: %.loc31_24.5: init i32 = initialize_from %.loc31_22.2 to %.loc31_24.4
// CHECK:STDOUT: %.loc31_24.6: init %.17 = tuple_init (%.loc31_24.3, %.loc31_24.5) to %return
// CHECK:STDOUT: %.loc31_25: init %.17 = converted %.loc31_24.1, %.loc31_24.6
// CHECK:STDOUT: return %.loc31_25 to %return
// CHECK:STDOUT: }
// CHECK:STDOUT:
Loading
Loading