Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support for cpp:identifier Metadata, and the Groundwork for xxx:identifier Metadata in General #3292

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

InsertCreativityHere
Copy link
Member

@InsertCreativityHere InsertCreativityHere commented Dec 18, 2024

This PR implements #3264 and is a proof of concept for #2864 by adding support for this metadata to C++.

It does this by changing 3 fundamental functions in the libSlice Parser:

  • name: returns the identifier of a Slice definition
  • scope: returns the enclosing scope of a Slice definition (with a trailing :: for the record)
  • scoped returns the fully-scoped identifier of a Slice definition (scope + name)

Currently on main, all 3 of these return the Slice identifier, and it's up the language mappings to massage them however.
Now, these functions take an optional string_view langPrefix parameter, allowing callers to signal that they want the identifier for a given mapping. When this parameter is present, these functions will check for <langPrefix>:identifier metadata. If it is present, we return that metadata's argument. If not, we return the Slice identifier like normal.


The general rule is:

  • do not pass in an argument if you want the Slice identifier
  • do pass in an argument if you want the mapped identifier

One downside of this is that previously, is that it's more involved to compute an element's scope, because it's legal to put xxx:identifier on modules. So to resolve a scope for some Slice element, we have to check each of its parents for this metadata.

@bernardnormier
Copy link
Member

because it's legal to put xxx:identifier on modules.

We should not do that, just like we don't allow cs::identifier on modules in "new Slice":
https://docs.icerpc.dev/slice2/language-guide/attributes#c#-attributes

@bernardnormier
Copy link
Member

Now, these functions take an optional string_view langPrefix parameter, allowing callers to signal that they want the identifier for a given mapping.

I find this sub-optimal. All our compilers generate code for only one language, so it would be nicer for the compiler to tell the Parser from the get-go "my language is xxx, please lookup xxx:identifier when resolving mapped identifiers".

@InsertCreativityHere
Copy link
Member Author

We should not do that, just like we don't allow cs::identifier on modules in "new Slice":

It's worth noting that modules are pretty different in IceRPC vs Ice Slice.
In IceRPC they're more of a declaration that applies to a file (like Java's package keyword), whereas in Ice, they're just another Slice element like anything else.

Theoretics aside, how do you propose a C++ user fixes this then?

module auto { ... }

Or even worse, this:

module project { module default { ... }}

@InsertCreativityHere
Copy link
Member Author

I find this sub-optimal. All our compilers generate code for only one language, so it would be nicer for the compiler to tell the Parser from the get-go "my language is xxx, please lookup xxx:identifier when resolving mapped identifiers".

Alright, I'll see what I can do to improve it. But I'll wait until we figure out the module situation.
Because disallowing this metadata on modules will already let me simplify things a good bit.

@bernardnormier
Copy link
Member

Theoretics aside, how do you propose a C++ user fixes this then?

module auto { ... }

Or even worse, this:

module project { module default { ... }}

It would be logical to add a new cpp:namespace directive that renames the mapped C++ namespace. However, this would be inconsistent with the existing cs:namespace, which confusingly specifies an enclosing namespace. And it's undesirable to change the semantics of 3.7 metadata in 3.8.

Now, back to new Slice: why do we have cs::namespace for modules and cs::identifier that doesn't apply to modules?

It's because the semantics are different and more specialized:
https://docs.icerpc.dev/slice2/language-guide/module#c#-mapping

We should do the same for Ice Slice. The question is which metadata do we use? Say we use cpp:ns (just an example), the semantics would be:

["cpp:ns:Reservation::Bundles"]
module Res::Vacation
{
  ... 
}

maps to C++ namespace Reservation::Bundles.

["cpp:ns:Foo::Reservation"]
module Res
{
    ["cpp:ns:Bundles"]
    module Vacation { ... }
}

maps to:

namespace Foo::Reservation
{
    namespace Bundles
    {
    }
}

Keep in mind the situation is simpler with new Slice since we don't support the module { ... } syntax:
https://docs.icerpc.dev/slice2/language-guide/module

@InsertCreativityHere
Copy link
Member Author

We should do the same for Ice Slice. The question is which metadata do we use? Say we use cpp:ns (just an example), the > semantics would be:

["cpp:ns:Reservation::Bundles"]
module Res::Vacation
{
 ... 
}

maps to C++ namespace Reservation::Bundles

Yes, but you're just describing the exact semantics of cpp:identifer. With my PR you can absolutely write:

["cpp:identifier:Foo::Reservation"]
module Res
{
    ["cpp:identifier:Bundles"]
    module Vacation { ... }
}

And it will be mapped to:

namespace Foo::Reservation {
    namespace Bundles { ... }
}

Since we're targeting C++17, which supports nested namespace definitions.


However... There might be a bug here with putting cpp:identifer on nested modules.
I'm not sure if it will affect the last module, or the entire nested module... I need to check.

Using a name like cpp:identifier, it's clear that this metadata just replaces the Slice identifier with whatever you specify. This avoids the appearance of being inconsistent with cs:namespace, since they have totally different names.

And it's easier to explain cpp:identifier without a special carve-out for modules: You can put it on anything with a Slice identifier, and the compiler will just copy/paste whatever you provided in the metadata in place of that Slice identifier.
No need to say: ... except for modules, in which case you should use this other metadata instead.


Back to new Slice though, modules are quite different between Ice and IceRPC.
So I don't think it's crazy to say "the way we handle metadata on them should be different".

And while the design of modules in IceRPC is simpler, I don't think that the way we handle and treat modules in Ice is wrong, just different. IceRPC uses the Java approach (package Hello;), and Ice uses the C++ approach (namespace Hello {...}).

@InsertCreativityHere
Copy link
Member Author

Also, as an aside, if we allow cs:identifier on modules in Slice, we could actually phase out/deprecate cs:namespace.
Anything you can do with cs:namespace you can also do with cs:identifier, but less confusingly, and with even more control.

If you have:

// module M is mapped to namespace MyWidget.M
["cs:namespace:MyWidget"]
module M {}

It is completely backwards compatible to update this to:

// module M is mapped to namespace MyWidget.M
["cs:identifier:MyWidget.M"]
module M {}

@bernardnormier
Copy link
Member

The argument for cs::namespace in new Slice is not an identifier but a scoped name.

You think it's ok to use lang:identifier in Ice Slice on modules where the argument of this "identifier" metadata is not an identifier?

With your example:

// module M is mapped to namespace MyWidget.M
["cs:identifier:MyWidget.M"] // MyWidget.M - the argument of cs:identifier - is not an identifier.
module M {}

@bernardnormier
Copy link
Member

bernardnormier commented Dec 19, 2024

Interestingly, cppreference names ns-name "identifier":
https://en.cppreference.com/w/cpp/language/namespace#Namespaces

probably because it was an identifier before C++17.

@InsertCreativityHere
Copy link
Member Author

Your point isn't lost on me.
To be entirely honest, the fact that this works is just an accident!
I didn't think about this case, let alone intentionally design for this.

But... yes, I do think it's okay. Although I agree it's not great.


But think mechanistically. With your idea, we'd have two metadata: cpp:identifier and let's say cpp:ns (hypothetically).
cpp:identifier can go on everything except modules.
cpp:ns can only go on modules.

And they would have the following effect:

["cpp:ns:There"]
module Hello
{
    ["cpp:identifier:MyThing"]
    struct Thing { int i; }
}

when compiled with slice2cpp maps to

namespace There
{
    struct MyThing { ::std::int16_t i; }
}

These effects are indistinguishable. In both cases, instead of using the Slice-provided identifier, a different name was generated based on some metadata.
And if we implemented both of these in slice2cpp, those implementations would be identical.

So, these sure feel like the same metadata to me. Just that it was artificially split into 2.
And now we have two metadata to document, and test, and support. Instead of just 1.
And users who know about cpp:identifier will be annoyed when it doesn't work on modules (for no reason they can see).

@InsertCreativityHere
Copy link
Member Author

InsertCreativityHere commented Dec 20, 2024

My example is a little contrived though, because we're only ever dealing with identifiers.
Obviously the 'interesting' case is when we have nested module syntax. But, before that, let's admit that most users do not use nested module syntax. So, for most users, who are using 'normal' module syntax, there is truly no problem here.

But okay. Nested module syntax looks like this:

module Hello::There {...}

An actual question: What would you call the Hello::There here?

Internally, in the Slice compiler, we call it a scoped identifier:

| ICE_MODULE ICE_SCOPED_IDENTIFIER
This is definitely not the same as identifier, but it is still a kind of identifier.

And as you point out, the cppreference calls it an ns-name. But, name is really just a synonym for identifier. We use 'identifier' and 'name' interchangably in the Slice compiler (although I wish we were more consistent).

A similar argument applies to how you called it a "scoped name".
To me, this is same as just saying "scoped identifier".


Yes, Hello::There isn't exactly an identifier, but it's in the same ballpark as one.
And it's definitely close enough that any of our users will understand what we mean.
If they can understand that amd really means asynchronous method dispatch.
They can figure out that cpp:identifier can accept both identifiers and scoped identifiers.
But it's on them to use them correctly based on the context.

Also, interestingly, cppreference has been using ns_name since at least 2012.
But back then they were using name in the explanation instead of identifier.
https://web.archive.org/web/20121024204606/en.cppreference.com/w/cpp/language/namespace
(Note that the page will be a little slow to load).

@bernardnormier
Copy link
Member

bernardnormier commented Dec 20, 2024

Regardless of which metadata we eventually select for modules, I recommend you rework this PR to focus exclusively on cpp:identifier for constructs other than modules.

Metadata for modules / namespaces / scopes deserves its own PR. For now, just rename the Slice "keyword" modules in the C++ tests to something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants