Error on impl functions prior to extends base #4648
Conversation
Since impl will need to be checked against the base class's virtual functions, and on the basis of the information accumulation principle (I think?). This does produce an awkward sequence of diagnostics "can't have impl without a base class" then "can't have base class after an impl". I guess we'd prefer to only have the latter? Could defer the "impl without base" until the end of the class to implement that correctly? Oh, and currently this will only produce one of "base must come before fields" or "base must come before impls" - I guess it'd be preferable to produce both?
| @@ -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()) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
shouldbe 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.
There was a problem hiding this comment.
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.
Since impl will need to be checked against the base class's virtual
functions, and on the basis of the information accumulation principle (I
think?).
This does produce an awkward sequence of diagnostics "can't have impl
without a base class" then "can't have base class after an impl". I
guess we'd prefer to only have the latter? Could defer the "impl without
base" until the end of the class to implement that correctly?
Oh, and currently this will only produce one of "base must come before
fields" or "base must come before impls" - I guess it'd be preferable to
produce both?