Disallow base virtual in adapter

[dwblaikie]

According to https://docs.carbon-lang.dev/docs/design/generics/details.html#adapting-types:

You can add any declaration that you could add to a class except for declarations that would change the representation of the type. This means you can add methods, functions, interface implementations, and aliases, but not fields, base classes, or virtual functions. The specific implementations of virtual functions are part of the type representation, and so no virtual functions may be overridden in an adapter either.
So, let's check/reject that.

Checking at the end of the class ensures that no matter the order of methods and adapt statements, the issue will still be correctly diagnosed.

[zygoloid]

Do we also reject if the virtual fn declaration precedes the adapt declaration? (Checking this at the end of the class may allow us to avoid putting the check in two places.)

[dwblaikie]

Oh, right you are indeed. Thanks that helps explain why the checks are at the end of the class, and why this new check should go there too.

So, now that brings me to the question of - how do I look at all the member functions of a SemIR::Class? Do I walk it as a NameScope and filter out the member functions from other members (like the adapt declaration, fields, etc? Assuming those are all in one collection/inst block/thingy?)?

[dwblaikie]

Thanks for the offline help understanding things a bit more.

Implemented a class-block scanning type solution. Though the class's inst block is empty at this point, with some debugging I figured out it was because it hadn't been completed - which happens somewhere where the inst_block_stack is popped and put into the class's inst block.

So I implemented this by iterating over the inst block at the top of the inst block stack. Is that right? Or are there ways the top of the stack could be something other than the class's scope? Or some other reason to do it differently?

(also, currently this means we'll only get one of the various adapter errors - should we change it to get all the errors (fields, virtual functions, base class, etc)?)

[zygoloid]

This looks good, thanks.

Given that the primary error points at the adapt declaration, we could consider adding notes for each thing in the class that is incompatible with that (or maybe just each kind of thing -- the first field, first (only) base, and first virtual function). My inclination would be to do that in a different PR if we want to pursue that direction. If so, I think we'd also want to generalize the main error diagnostic text so it works with different kinds of note:

error: adapter with state
note: first field is here
note: first virtual function is here

or similar might work? (I'm not sure "state" is the right word though.)

[zygoloid]

Thanks!

[zygoloid]

Looks like the initial comment on this PR is referring to the previous implementation strategy still; can you update that before we merge, please? (That ends up as the commit message.)

[dwblaikie]

Looks like the initial comment on this PR is referring to the previous implementation strategy still; can you update that before we merge, please? (That ends up as the commit message.)

Updated. I minor issue I couldn't find at a glance in the spec - is this "adapt Foo;" a statement? a declaration? some other entity/terminology? (& I take it the canonical terminology for what we'd call member functions in C++ is "method" in carbon? - is there a general lexicon/terminology list for carbon in the docs somewhere?)

[zygoloid]

Looks like you have a test failure after merging into trunk; you probably need to merge trunk back into this and autoupdate.

[zygoloid]

I minor issue I couldn't find at a glance in the spec - is this "adapt Foo;" a statement? a declaration? some other entity/terminology?

Officially, I don't think we have our own definitions for these terms, and we've just sort of inherited the C++ definitions -- so unofficially, a statement is a complete sequentially executable program fragment, and a declaration is a complete program fragment that can appear in non-sequentially-executable scopes. We refer to adapt Foo; and extend base Foo; as AdaptDecl and BaseDecl in the toolchain, and I think I'd call them declarations.

(& I take it the canonical terminology for what we'd call member functions in C++ is "method" in carbon?

Yes, "method" in Carbon approximately corresponds to "non-static member function" in C++.

Methods are somewhat more general in Carbon, because they can have a wider range of self types and can be declared in other contexts than classes (such as in impls) -- and in fact we don't currently have any restrictions on where methods can be declared (though we've talked about adding a restriction) so nothing currently prevents:

fn Negate[self: i32]() -> i32 { return -self; }
var n: i32 = 1;
var m: i32 = n.(Negate)();

(n.Negate() wouldn't work because it'd perform name lookup into i32.)

is there a general lexicon/terminology list for carbon in the docs somewhere?)

There's https://github.com/carbon-language/carbon-lang/blob/trunk/docs/guides/glossary.md but it's extremely incomplete.