Reject/error on base declarations that appear after field declarations #4553
Conversation
dwblaikie
force-pushed
the
disallow_impl_without_base
branch
from
79d53b5
to
6e58442
Compare
|
Open to feedback on the diagnostic phrasing, where the handling is being done (too early? too late?) etc. |
| @@ -512,6 +512,15 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool { | |||
| return true; | |||
| } | |||
|
|
|||
| if (!context.struct_type_fields_stack().PeekArray().empty()) { | |||
There was a problem hiding this comment.
| if (!context.struct_type_fields_stack().PeekArray().empty()) { | |
| if (auto fields = context.struct_type_fields_stack().PeekArray(); !fields.empty()) { |
(see note suggestion below)
toolchain/check/handle_class.cpp
Outdated
| context.emitter().Emit(node_id, BaseDeclAfterFieldDecl, | ||
| Lex::TokenKind::Base); |
There was a problem hiding this comment.
Does it work to add a note for the first field location? i.e., something like:
| context.emitter().Emit(node_id, BaseDeclAfterFieldDecl, | |
| Lex::TokenKind::Base); | |
| CARBON_DIAGNOSTIC(FirstFieldDecl, Note, | |
| "First field declared here"); | |
| context.emitter().Build(node_id, BaseDeclAfterFieldDecl, | |
| Lex::TokenKind::Base).Note(fields.front(), FirstFieldDecl).Emit(); |
(I think FirstFieldDecl is a bad name, but I'm struggling with better naming)
There was a problem hiding this comment.
For now the StructTypeFields don't have source locations for this extra note, but once we track the FieldDecls separately in #4514 I can revisit this to add the note. I'll add a TODO for this.
toolchain/check/handle_class.cpp
Outdated
| "`{0}` declaration appears after field declaration(s)", | ||
| Lex::TokenKind); | ||
| context.emitter().Emit(node_id, BaseDeclAfterFieldDecl, | ||
| Lex::TokenKind::Base); |
There was a problem hiding this comment.
Why do you pass in Base here, versus putting base directly in the message?
There was a problem hiding this comment.
Inspired by some nearby code (DiagnoseClassSpecificDeclRepeated - which admittedly is actually parameterized, with one caller using Lex::TokenKind::Base and another using Lex::TokenKind::Adapt, whereas this case only has the one value). Smallest argument in favor of type safety/write-it-once - there's one bit of code for mapping this keyword to text, rather than including freeform text that is consistent today, but isn't checked in any way...
But I certainly don't feel strongly. I see there are at least several cases where it's written literally/in the string, so I'll change to that.
Co-authored-by: Jon Ross-Perkins <[email protected]>
(sounded better to me and we have one prior diagnostic example of `appear before` and none for `come before`) (admittedly we also have one example of "come after" and none for "appear after"... )
dwblaikie
force-pushed
the
disallow_impl_without_base
branch
from
466d1ce
to
a0e2222
Compare
No description provided.