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.

Sign up for GitHub

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

Error on impl functions prior to extends base #4648

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Error on impl functions prior to extends base #4648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions toolchain/check/handle_class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,21 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
return true;
}

// TODO: base must appear before virtual functions too
for (auto inst_id : context.inst_block_stack().PeekCurrentBlockContents()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry that re-traversing the current block could be excessively costly. Do you think it might be sufficient to rephrase ImplWithoutBase to say there hasn't been a base declaration yet, and not have a separate diagnostic when we actually reach the base declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose - it's sort of awkward that that's different from the field-before-base error (though that's meaningfully different than the impl-before-base, since impl-before-base can be identified as invalid at the impl, without having to wait for the base)

But if we want to error on virtual/abstract-before-base (for consistency? for detecting whether the virtual functions shadows something in the base (& should be impl instead), etc?), we can't do it at the virtual function and would need to do this second pass, or some other mechanism?

This will only traverse the members that appear before the base decl, right? Which should be relatively few? (though not guaranteed - since we're currently saying you can have any number of non-virtual functions declared before the base decl)

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose - it's sort of awkward that that's different from the field-before-base error (though that's meaningfully different than the impl-before-base, since impl-before-base can be identified as invalid at the impl, without having to wait for the base)

I see it as consistent: in both cases, we're diagnosing the error at the earliest point where it can be detected (and only there). The asymmetry between the two just reflects the asymmetry in the language rules: "impl must be preceded by base" is structurally different from "base must not be preceded by a field".

But if we want to error on virtual/abstract-before-base (for consistency? for detecting whether the virtual functions shadows something in the base (& should be impl instead), etc?), we can't do it at the virtual function and would need to do this second pass, or some other mechanism?

I'm not sure of the best way to implement that. Possibly there's somewhere ephemeral where we could track whether we've seen any virtual/abstract methods yet?

This will only traverse the members that appear before the base decl, right? Which should be relatively few? (though not guaranteed - since we're currently saying you can have any number of non-virtual functions declared before the base decl)

Yeah, I assume the prevailing style will be to put the base decl as close to the top of the class as possible, so the cost might not be that bad in practice. But even if performance isn't an issue, there's still the problem that IIUC this diagnostic can only catch issues that will also be caught (and caught earlier) by ImplWithoutBase. If that's the case, this diagnostic seems like unnecessary clutter.

Copy link
Contributor Author

@dwblaikie dwblaikie Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But if we want to error on virtual/abstract-before-base (for consistency? for detecting whether the virtual functions shadows something in the base (& should be impl instead), etc?), we can't do it at the virtual function and would need to do this second pass, or some other mechanism?

I'm not sure of the best way to implement that. Possibly there's somewhere ephemeral where we could track whether we've seen any virtual/abstract methods yet?

Yeah, this might be the best place to look further into - to answer some of the broader questions of vtable layout, etc, maybe we'll want some bigger change/more invasive representational features for dynamic classes. I've started a discussion ( #4671 ) to examine that.

Might hold off on this review (I can mark it closed, perhaps?) for now, then.

if (auto function_decl =
context.insts().TryGetAs<SemIR::FunctionDecl>(inst_id)) {
auto& function = context.functions().Get(function_decl->function_id);
if (function.virtual_modifier == SemIR::Function::VirtualModifier::Impl) {
CARBON_DIAGNOSTIC(
BaseDeclAfterImplDecl, Error,
"`base` declaration must appear before impl function declarations");
context.emitter().Emit(node_id, BaseDeclAfterImplDecl);
return true;
}
}
}

auto base_info = CheckBaseType(context, base_type_node_id, base_type_expr_id);

// TODO: Should we diagnose if there are already any fields?
Expand Down
75 changes: 74 additions & 1 deletion toolchain/check/testdata/class/virtual_modifiers.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,10 @@ class C {
package FailModifiers;

class C {
// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:3: error: impl without base class [ImplWithoutBase]
// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:3: error: impl without base class [ImplWithoutBase]
// CHECK:STDERR: impl fn F();
// CHECK:STDERR: ^~~~~~~~~~~~
// CHECK:STDERR:
impl fn F();
}

Expand Down Expand Up @@ -118,6 +119,25 @@ class Derived {
impl fn F();
}

// --- fail_impl_before_base.carbon

package ImplBeforeBase;

base class Base {
}

class Derived {
// CHECK:STDERR: fail_impl_before_base.carbon:[[@LINE+4]]:3: error: impl without base class [ImplWithoutBase]
// CHECK:STDERR: impl fn F();
// CHECK:STDERR: ^~~~~~~~~~~~
// CHECK:STDERR:
impl fn F();
// CHECK:STDERR: fail_impl_before_base.carbon:[[@LINE+3]]:3: error: `base` declaration must appear before impl function declarations [BaseDeclAfterImplDecl]
// CHECK:STDERR: extend base: Base;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~
extend base: Base;
}

// CHECK:STDOUT: --- modifiers.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
Expand Down Expand Up @@ -695,3 +715,56 @@ class Derived {
// CHECK:STDOUT:
// CHECK:STDOUT: impl fn @F();
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_impl_before_base.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
// CHECK:STDOUT: %Base: type = class_type @Base [template]
// CHECK:STDOUT: %empty_struct_type: type = struct_type {} [template]
// CHECK:STDOUT: %complete_type.1: <witness> = complete_type_witness %empty_struct_type [template]
// CHECK:STDOUT: %Derived: type = class_type @Derived [template]
// CHECK:STDOUT: %F.type: type = fn_type @F [template]
// CHECK:STDOUT: %F: %F.type = struct_value () [template]
// CHECK:STDOUT: %ptr: type = ptr_type <vtable> [template]
// CHECK:STDOUT: %struct_type.vptr: type = struct_type {.<vptr>: %ptr} [template]
// CHECK:STDOUT: %complete_type.2: <witness> = complete_type_witness %struct_type.vptr [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: imports {
// CHECK:STDOUT: %Core: <namespace> = namespace file.%Core.import, [template] {
// CHECK:STDOUT: import Core//prelude
// CHECK:STDOUT: import Core//prelude/...
// CHECK:STDOUT: }
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: file {
// CHECK:STDOUT: package: <namespace> = namespace [template] {
// CHECK:STDOUT: .Core = imports.%Core
// CHECK:STDOUT: .Base = %Base.decl
// CHECK:STDOUT: .Derived = %Derived.decl
// CHECK:STDOUT: }
// CHECK:STDOUT: %Core.import = import Core
// CHECK:STDOUT: %Base.decl: type = class_decl @Base [template = constants.%Base] {} {}
// CHECK:STDOUT: %Derived.decl: type = class_decl @Derived [template = constants.%Derived] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @Base {
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template = constants.%complete_type.1]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%Base
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @Derived {
// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} {}
// CHECK:STDOUT: %Base.ref: type = name_ref Base, file.%Base.decl [template = constants.%Base]
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %struct_type.vptr [template = constants.%complete_type.2]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%Derived
// CHECK:STDOUT: .F = %F.decl
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: impl fn @F();
// CHECK:STDOUT:
1 change: 1 addition & 0 deletions toolchain/diagnostics/diagnostic_kind.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ CARBON_DIAGNOSTIC_KIND(BaseDeclRepeated)
CARBON_DIAGNOSTIC_KIND(BaseIsFinal)
CARBON_DIAGNOSTIC_KIND(BaseMissingExtend)
CARBON_DIAGNOSTIC_KIND(BaseDeclAfterFieldDecl)
CARBON_DIAGNOSTIC_KIND(BaseDeclAfterImplDecl)
CARBON_DIAGNOSTIC_KIND(ClassAbstractHere)
CARBON_DIAGNOSTIC_KIND(ClassForwardDeclaredHere)
CARBON_DIAGNOSTIC_KIND(ClassSpecificDeclOutsideClass)
Expand Down
Loading