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

dwblaikie · 2024-10-07T18:41:32Z

Per: p3762: "Other class, impl, and interface modifiers (abstract, base, final) exist only on the definition, not on the forward declaration."

dwblaikie · 2024-10-07T23:20:53Z

Hmm, in the process of testing this further, I came across existing test coverage that verified a different rule (that the base/abstract specifiers had to match between redeclarations) and linked to a still-open issue: #3384

Looks like #3762 was merged in March 2024, #3384 was filed in Nov 2023, but @josh11b mentioned in #3384 (comment) that there was still ongoing discussion about how this should be addressed?

So... I provide this patch as a foray into discussing how this is meant to work/what bits of the spec/approved proposals are authoritative with respect to answering this question.

josh11b · 2024-10-08T22:19:32Z

Hmm, in the process of testing this further, I came across existing test coverage that verified a different rule (that the base/abstract specifiers had to match between redeclarations) and linked to a still-open issue: #3384

Looks like #3762 was merged in March 2024, #3384 was filed in Nov 2023, but @josh11b mentioned in #3384 (comment) that there was still ongoing discussion about how this should be addressed?

So... I provide this patch as a foray into discussing how this is meant to work/what bits of the spec/approved proposals are authoritative with respect to answering this question.

It looks like #3762 and that discussion ultimately agree that for abstract and base, it should only be on the class definition, not other declarations. This also agrees with the last thing written in #3384 , but we should poke the leads to accept that resolution or say otherwise. But in general I would say a proposal accepted after an issue was posed takes precedence.

josh11b · 2024-10-08T22:23:41Z

toolchain/check/testdata/class/fail_abstract.carbon

  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


It would be nice (but not required) to have the error say this modifier should only be on the definition.

Yeah, it crossed my mind when developing this, but the generic functionality for disallowed modifiers was rather convenient & adding this special case felt like it'd be pretty ugly.

I've implemented it/updated this patch with that change just for the exploration - maybe there's nicer ways to do it (with a callback?) or perhaps it feels worth the complexity/inelegance. Open to ideas/refactorings/revertings/renamings/etc.

josh11b · 2024-10-08T22:28:01Z

toolchain/check/testdata/class/fail_modifiers.carbon

@@ -8,6 +8,10 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/class/fail_modifiers.carbon

+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:9: error: `abstract` not allowed on `class` declaration
+// CHECK:STDERR: private abstract private class DuplicatePrivate;


Feels better to have separate tests for "abstract not allowed on declaration" from "modifier repeated" and so on. Perhaps make these tests use a class definition ({} instead of ;), now that those are different? (similarly other tests)

Agreed & have updated the test in that direction - though wasn't sure how much you were thinking we should go (like - revisiting existing untouched test cases into more distinct/isolated tests? Happy to do that, but haven't done it yet).

josh11b · 2024-10-08T22:29:20Z

Hmm, in the process of testing this further, I came across existing test coverage that verified a different rule (that the base/abstract specifiers had to match between redeclarations) and linked to a still-open issue: #3384
Looks like #3762 was merged in March 2024, #3384 was filed in Nov 2023, but @josh11b mentioned in #3384 (comment) that there was still ongoing discussion about how this should be addressed?
So... I provide this patch as a foray into discussing how this is meant to work/what bits of the spec/approved proposals are authoritative with respect to answering this question.

It looks like #3762 and that discussion ultimately agree that for abstract and base, it should only be on the class definition, not other declarations. This also agrees with the last thing written in #3384 , but we should poke the leads to accept that resolution or say otherwise. But in general I would say a proposal accepted after an issue was posed takes precedence.

Update: #3384 has now been resolved.

…initions) Per: https://docs.carbon-lang.dev/proposals/p3762.html#modifier-keywords:~:text=Other%20class%2C%20impl%2C%20and%20interface%20modifiers%20%28abstract%2C%20base%2C%20final%29%20exist%20only%20on%20the%20definition%2C%20not%20on%20the%20forward%20declaration "Other class, impl, and interface modifiers (`abstract`, `base`, `final`) exist only on the definition, not on the forward declaration."

Also add definitions to cases where the decl/def distinction isn't needed, and the decl case is invalid for other reasons that are tested separately.

…tions, but not declarations

josh11b · 2024-10-09T23:08:21Z

toolchain/check/handle_class.cpp

+  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);


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.

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

josh11b · 2024-10-09T23:17:11Z

toolchain/check/testdata/class/fail_abstract.carbon

+// 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;


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.

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

josh11b · 2024-10-09T23:20:04Z

toolchain/check/testdata/class/fail_base_bad_type.carbon

+// CHECK:STDERR: fail_base_bad_type.carbon:[[@LINE+4]]:1: error: `base` not allowed on `class` declaration, only definition
+// CHECK:STDERR: base class Incomplete;
+// CHECK:STDERR: ^~~~
+// CHECK:STDERR:


Again a case where the tests should be in separate splits. Many of these files predate us supporting splits. This is also a case where "base only allowed on definitions" should be separated from other tests about incomplete types.

Made some splits here too.

josh11b

Thanks, particularly for cleaning up the tests you were touching!

github-actions bot requested a review from josh11b October 7, 2024 18:41

github-actions bot added the toolchain label Oct 7, 2024

josh11b reviewed Oct 8, 2024

View reviewed changes

dwblaikie added 4 commits October 9, 2024 19:49

Baseline

1c3c92b

Remove dead test coverage

af59051

Separate out some test cases

964be4f

Also add definitions to cases where the decl/def distinction isn't needed, and the decl case is invalid for other reasons that are tested separately.

dwblaikie force-pushed the disallow_abstract_class_declaration branch from 762111f to 964be4f Compare October 9, 2024 19:49

Rephrase the warning for modifiers that would be acceptable on defini…

068c7ec

…tions, but not declarations

josh11b reviewed Oct 9, 2024

View reviewed changes

dwblaikie added 4 commits October 10, 2024 00:00

Simplify the implementation of the decl-only-on-def modifier diagnostic

854694f

Add file splitting to some tests

49a27a7

Pull out a common subexpression

bf9c0cf

Bit more fixing for tests now that I understand split tests better

cfc5940

josh11b approved these changes Oct 10, 2024

View reviewed changes

josh11b added this pull request to the merge queue Oct 10, 2024

Merged via the queue into carbon-language:trunk with commit b1014bf Oct 10, 2024
8 checks passed

dwblaikie deleted the disallow_abstract_class_declaration branch October 10, 2024 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dwblaikie commented Oct 7, 2024

dwblaikie commented Oct 7, 2024

josh11b commented Oct 8, 2024 •

edited

Loading

josh11b Oct 8, 2024

dwblaikie Oct 9, 2024

josh11b Oct 8, 2024

dwblaikie Oct 9, 2024

josh11b commented Oct 8, 2024

josh11b Oct 9, 2024

dwblaikie Oct 10, 2024

josh11b Oct 9, 2024

dwblaikie Oct 10, 2024

josh11b Oct 9, 2024

dwblaikie Oct 10, 2024

josh11b left a comment

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

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

Conversation

dwblaikie commented Oct 7, 2024

dwblaikie commented Oct 7, 2024

josh11b commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh11b commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh11b left a comment

Choose a reason for hiding this comment

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

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

josh11b commented Oct 8, 2024 •

edited

Loading