Add SemIR support for virtual functions #4272

dwblaikie · 2024-09-03T19:26:04Z

I guess this technically would also allow code to pass check that hasn't before, and that isn't covered by tests (since it's masked by other failures in the tests that already test this functionality) - should I add another test/add some code to a valid test case?

Also, this'll miscompile in lowering, since there's no support there yet - should I do anything about that to make lowering fail in some way? Or is it acceptable that some things just silently mis-lower? (I could add a currently-miscompiling test case too, to demonstrate this? (not sure if the autogenerated tests leave space for comments that would explain that the currently-tested behavior is incorrect?))

Is the addition to EntityWithParamsBase suitable? of course not all functions can be virtual, so it's a wasted bit at the moment for all those cases (though it's free, since it's bitpacked - but as we want to add more bits in there it might not be a scalable solution)?

toolchain/check/testdata/class/fail_todo_modifiers.carbon

toolchain/sem_ir/entity_with_params_base.h

jonmeow · 2024-09-03T22:20:18Z

I guess this technically would also allow code to pass check that hasn't before, and that isn't covered by tests (since it's masked by other failures in the tests that already test this functionality) - should I add another test/add some code to a valid test case?

Yes, I think there should be tests. Note though, I've commented where this is affecting tests -- those should be cleaned up.

Also, this'll miscompile in lowering, since there's no support there yet - should I do anything about that to make lowering fail in some way? Or is it acceptable that some things just silently mis-lower? (I could add a currently-miscompiling test case too, to demonstrate this? (not sure if the autogenerated tests leave space for comments that would explain that the currently-tested behavior is incorrect?))

This is fine: code which passes check is not currently assumed to compile correctly (and could crash in lowering).

We're not fuzzing lowering right now because of this.

Is the addition to EntityWithParamsBase suitable? of course not all functions can be virtual, so it's a wasted bit at the moment for all those cases (though it's free, since it's bitpacked - but as we want to add more bits in there it might not be a scalable solution)?

I think these belong on Function, not EntityWithParamsBase. You're correct not all functions can be virtual, but not all entities are functions and I think that's more significant. I would be reticent to put entity-specific features on EntityWithParamsBase.

Note I don't think we've done any explicit bitpacking in SemIR (at least, not on these entities). It may be something we want to adjust, but right now we're prioritizing readability over saving bytes.

josh11b

Seems like you have some changes to make based on @jonmeow 's comments, so I'm pausing my review.

josh11b · 2024-09-03T20:06:17Z

toolchain/check/handle_function.cpp

+  if (!is_virtual &&
+      introducer.modifier_set.HasAnyOf(KeywordModifierSet::Method)) {


I think this only works because the method modifiers are mutually exclusive? Otherwise this would accept virtual along with an unsupported method modifier. Maybe worth a comment.

Oh, right - thanks!

Now that I've implemented @jonmeow's suggestion of also handling abstract and impl, there's nothing left to diagnose here so I've removed this block entirely.

dwblaikie

Is the addition to EntityWithParamsBase suitable? of course not all functions can be virtual, so it's a wasted bit at the moment for all those cases (though it's free, since it's bitpacked - but as we want to add more bits in there it might not be a scalable solution)?

I think these belong on Function, not EntityWithParamsBase. You're correct not all functions can be virtual, but not all entities are functions and I think that's more significant. I would be reticent to put entity-specific features on EntityWithParamsBase.

Ah, right, makes sense - done that.

Note I don't think we've done any explicit bitpacking in SemIR (at least, not on these entities). It may be something we want to adjust, but right now we're prioritizing readability over saving bytes.

Ah, sure - I've removed the bit field stuff then.

dwblaikie · 2024-09-05T00:31:56Z

toolchain/check/handle_function.cpp

+  if (!is_virtual &&
+      introducer.modifier_set.HasAnyOf(KeywordModifierSet::Method)) {


Oh, right - thanks!

Now that I've implemented @jonmeow's suggestion of also handling abstract and impl, there's nothing left to diagnose here so I've removed this block entirely.

toolchain/check/testdata/class/fail_todo_modifiers.carbon

dwblaikie · 2024-09-05T17:28:55Z

Oh, I had one other question - is mapping these language features directly to SemIR the right choice? Or should we consider a more semantic description of these properties? (like could all 3 of these properties map to "virtual"? I guess then we wouldn't know the difference between abstract and a virtual function that's defined externally (if that's even possible in Carbon? I assume so) - could have two properties, then, virtual and abstract (& perhaps all abstract things are also marked virtual))?

I guess probably not - but thought it was worth asking/understanding better what semantic representation SemIR is going for.

jonmeow · 2024-09-05T19:06:10Z

toolchain/sem_ir/function.h

+  // Is this function declaration is virtual - which is to say, it includes an
+  // implementation, but can be overridden in a derived class.
+  bool is_virtual;
+
+  // Is this function declaration is abstract - having no implementation, and
+  // must be implemented in a concrete derived class.
+  bool is_abstract;
+
+  // Is this function declaration an implementation - overriding an abstract or
+  // virtual function declared in a base class.
+  bool is_impl;


These are mutually exclusive, had you considered an enum?

Done - open to haggling over naming, of course.

jonmeow · 2024-09-05T19:11:02Z

toolchain/check/testdata/class/dynamic_function_modifiers.carbon

+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/dynamic_function_modifiers.carbon


Maybe "virtual_function_modifiers" (or even just "virtual_modifiers")? Although these are one way to get dynamic dispatch, https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/classes.md#virtual-methods calls these "virtual methods" and "virtual override keywords"

Ah, sure - done.

toolchain/check/testdata/class/dynamic_function_modifiers.carbon

jonmeow · 2024-09-05T19:31:35Z

Oh, I had one other question - is mapping these language features directly to SemIR the right choice? Or should we consider a more semantic description of these properties? (like could all 3 of these properties map to "virtual"? I guess then we wouldn't know the difference between abstract and a virtual function that's defined externally (if that's even possible in Carbon? I assume so) - could have two properties, then, virtual and abstract (& perhaps all abstract things are also marked virtual))?

I guess probably not - but thought it was worth asking/understanding better what semantic representation SemIR is going for.

This is essentially what I'd expect, with the enum comment. Note though, I would generally encourage a little more implementation work to see how changes interact -- this is just a straightforward change to me, and some things (e.g. printing of "virtual" in formatter) seem like they might mirror a need to do similar "virtual"/"impl"/"abstract" stringification in diagnostics, which might yield a slightly different approach.

Regarding mapping all to "virtual", each of the keywords implies different semantics, so they do need to be tracked separately.

could have two properties, then, virtual and abstract (& perhaps all abstract things are also marked virtual))?

For example, it is invalid to declare a final class that inherits from a class without providing impl for all the abstract functions. That is not true of virtual functions either way: virtual provides a root of a virtual call, and shadowed virtual functions are just that (shadowed, not impls).

dwblaikie · 2024-09-09T18:08:32Z

Oh, I had one other question - is mapping these language features directly to SemIR the right choice? Or should we consider a more semantic description of these properties? (like could all 3 of these properties map to "virtual"? I guess then we wouldn't know the difference between abstract and a virtual function that's defined externally (if that's even possible in Carbon? I assume so) - could have two properties, then, virtual and abstract (& perhaps all abstract things are also marked virtual))?
I guess probably not - but thought it was worth asking/understanding better what semantic representation SemIR is going for.

This is essentially what I'd expect, with the enum comment. Note though, I would generally encourage a little more implementation work to see how changes interact -- this is just a straightforward change to me, and some things (e.g. printing of "virtual" in formatter) seem like they might mirror a need to do similar "virtual"/"impl"/"abstract" stringification in diagnostics, which might yield a slightly different approach.

Sorry, I'm not quite following here - you mean: Good to go with what we have now, not try to overdesign it without experience. Or you'd encourage doing more implementation work/prototyping before a patch like this goes in, so we're heading in a more informed direction?

Regarding mapping all to "virtual", each of the keywords implies different semantics, so they do need to be tracked separately.

could have two properties, then, virtual and abstract (& perhaps all abstract things are also marked virtual))?

For example, it is invalid to declare a final class that inherits from a class without providing impl for all the abstract functions. That is not true of virtual functions either way: virtual provides a root of a virtual call, and shadowed virtual functions are just that (shadowed, not impls).

Sorry, I should've used more words: I figured those invariants would be checked in check, and then maybe SemIR would implement the resulting semantics without the need to maintain enough information to diagnose those things - just enough to implement the requested semantics.

But I guess it's probably convenient to have all 3 markers for lowering, if not technically necessary (C++ being a proof of concept that it's not /necessary/ and can be derived from a rougher description). Being able to tell which functions introduce new vtable slots (virtual), those without implementations (so vtable slot can be initialized with null/skip the relocation/symbol reference), and overrides.

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

… and impl properties as well

Turned out the access modifier testing wasn't failing by itstelf, so remove the `fail_` prefix from the remaining test coverage, and rename it to be more specific about what's covered (access modifiers).

…now they're all covered/implemented in Check/SemIR

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

jonmeow

Basically LG here, but one small style request (and one mulling comment, just mentioning the thought here)

Feel free to merge after adjusting the struct order

jonmeow · 2024-09-09T22:10:42Z

toolchain/sem_ir/function.h

@@ -23,6 +23,12 @@ struct FunctionFields {
  // always present if the function has a declared return type.
  InstId return_storage_id;

+  enum class VirtualModifier { None, Virtual, Abstract, Impl };


Sorry, can you move this to the top (for declaration order styles, types/enums come before instance members)

Ah, sure - done in a2d80a6

jonmeow · 2024-09-09T22:13:24Z

toolchain/check/handle_function.cpp

+  if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Virtual)) {
+    virtual_modifier = SemIR::FunctionFields::VirtualModifier::Virtual;
+  }
+  if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Abstract)) {
+    virtual_modifier = SemIR::FunctionFields::VirtualModifier::Abstract;
+  }
+  if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Impl)) {
+    virtual_modifier = SemIR::FunctionFields::VirtualModifier::Impl;
  }


Note, not suggesting a change here, but thinking about this...

Compare what you're doing here with the class code:
https://github.com/carbon-language/carbon-lang/blob/trunk/toolchain/check/handle_class.cpp#L203-L208

Maybe we should have a helper or something on KeywordModifierSet to help switch modifiers to an appropriate enum? It feels like something that we could maybe do with a variadic template...

Oh, yeah - I can look into that.

Sent #4290

Aside: The handle_class.cpp case uses a non-scoped enum (enum, not enum class) and references the entities via the derived type (Class::Enumerator rather than ClassFields::Enumerator) - I guess the latter's clearly an improvement over the code I've written here for the function case, but should we standardize on the enum or enum class here? In this particular case an unscoped enum seems OK?

…4290) (based on #4272 (comment)) Could haggle over the name "ToEnum" probably avoids the debate over "enumeration" (the type being returned) v "enumerator" (the value being returned) --------- Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

github-actions bot requested a review from josh11b September 3, 2024 19:26

github-actions bot added the toolchain label Sep 3, 2024

jonmeow reviewed Sep 3, 2024

View reviewed changes

toolchain/check/testdata/class/fail_todo_modifiers.carbon Outdated Show resolved Hide resolved

toolchain/check/testdata/class/fail_todo_modifiers.carbon Outdated Show resolved Hide resolved

toolchain/sem_ir/entity_with_params_base.h Outdated Show resolved Hide resolved

josh11b reviewed Sep 3, 2024

View reviewed changes

dwblaikie commented Sep 5, 2024

View reviewed changes

jonmeow reviewed Sep 5, 2024

View reviewed changes

dwblaikie and others added 10 commits September 9, 2024 21:49

Add SemIR support for virtual functions

02c9ac9

Fix typo

c445d43

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

Move property from EntityWithParamsBase to Function, and add abstract…

5a2d360

… and impl properties as well

Add abstract and impl to the sem_ir textual output

acc4f75

Move now-supported modifier test coverage into a non-fail test

4ac3af5

Move dynamic function modifier check testing to a separate file

1a483eb

Turned out the access modifier testing wasn't failing by itstelf, so remove the `fail_` prefix from the remaining test coverage, and rename it to be more specific about what's covered (access modifiers).

Remove TODO since the Method set is just {virtual,abstract,impl} and …

ae8ab35

…now they're all covered/implemented in Check/SemIR

Remove some excess empty lines

9541363

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

Use enumeration instead of individual booleans

7922570

Rename dynamic_function_modifiers.carbon to virtual_modifiers.carbon

c25da7d

dwblaikie force-pushed the check_semir_virtual_functions branch from ffc0e43 to c25da7d Compare September 9, 2024 21:56

Update test

998b6f4

jonmeow approved these changes Sep 9, 2024

View reviewed changes

Move enum class to the top of the class to match the style guide.

a2d80a6

dwblaikie enabled auto-merge September 10, 2024 18:01

Merge branch 'trunk' into check_semir_virtual_functions

a4cfd48

dwblaikie added this pull request to the merge queue Sep 10, 2024

Merged via the queue into carbon-language:trunk with commit 5806d83 Sep 10, 2024
8 checks passed

dwblaikie deleted the check_semir_virtual_functions branch September 10, 2024 18:36

dwblaikie mentioned this pull request Sep 10, 2024

Add KeywordModifierSet helper for conversion to (likely SemIR) enums #4290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SemIR support for virtual functions #4272

Add SemIR support for virtual functions #4272

dwblaikie commented Sep 3, 2024

jonmeow commented Sep 3, 2024

josh11b left a comment

josh11b Sep 3, 2024

dwblaikie Sep 5, 2024

dwblaikie left a comment

dwblaikie Sep 5, 2024

dwblaikie commented Sep 5, 2024

jonmeow Sep 5, 2024

dwblaikie Sep 9, 2024

jonmeow Sep 5, 2024 •

edited

Loading

dwblaikie Sep 9, 2024

jonmeow commented Sep 5, 2024 •

edited

Loading

dwblaikie commented Sep 9, 2024

jonmeow left a comment

jonmeow Sep 9, 2024

dwblaikie Sep 10, 2024

jonmeow Sep 9, 2024

dwblaikie Sep 10, 2024

dwblaikie Sep 10, 2024

		if (!is_virtual &&
		introducer.modifier_set.HasAnyOf(KeywordModifierSet::Method)) {

Add SemIR support for virtual functions #4272

Add SemIR support for virtual functions #4272

Conversation

dwblaikie commented Sep 3, 2024

jonmeow commented Sep 3, 2024

josh11b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwblaikie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwblaikie commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow commented Sep 5, 2024 • edited Loading

dwblaikie commented Sep 9, 2024

jonmeow left a 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

Choose a reason for hiding this comment

jonmeow Sep 5, 2024 •

edited

Loading

jonmeow commented Sep 5, 2024 •

edited

Loading