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.

Sign up for GitHub

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 cs:identifier Metadata #3445

Merged
merged 17 commits into from
Jan 30, 2025
Merged

Add Support for cs:identifier Metadata #3445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5590716
Step 1.
InsertCreativityHere Jan 28, 2025
01dc1cb
Let's just always qualift built-in our types with 'global::'
InsertCreativityHere Jan 28, 2025
710f44b
Remapping calls and removing dead code part 1.
InsertCreativityHere Jan 28, 2025
3bdafa4
Finished fixing all the calls and removing some dead code.
InsertCreativityHere Jan 28, 2025
55c9add
Fixed the header file.
InsertCreativityHere Jan 28, 2025
efa5031
Added support for 'cs:identifier' metadata.
InsertCreativityHere Jan 28, 2025
1d40420
Fixed the C#/Ice/servantLocator test.
InsertCreativityHere Jan 28, 2025
768ff58
Finished changes to the 'slice2cs'.
InsertCreativityHere Jan 28, 2025
fda72a1
Final self-review.
InsertCreativityHere Jan 28, 2025
e031cd5
Eat any leading '@' characters when prefixing things.
InsertCreativityHere Jan 29, 2025
13ab236
Bug fixes.
InsertCreativityHere Jan 29, 2025
72d093b
Fixed the escape tests.
InsertCreativityHere Jan 29, 2025
d684620
Formatting and added some doc-comment testing.
InsertCreativityHere Jan 29, 2025
fd8d009
You can specify which separator you want 'mappedScoped' to use now.
InsertCreativityHere Jan 29, 2025
c16f28f
Review fixes.
InsertCreativityHere Jan 29, 2025
87313ca
Forgot to strip leading scope separator in some places.
InsertCreativityHere Jan 29, 2025
fb54fcf
More formatting...
InsertCreativityHere Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 5 additions & 39 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,20 +931,20 @@ Slice::Contained::mappedName() const
}

string
Slice::Contained::mappedScoped() const
Slice::Contained::mappedScoped(const string& separator) const
{
return mappedScope() + mappedName();
return mappedScope(separator) + mappedName();
}

string
Slice::Contained::mappedScope() const
Slice::Contained::mappedScope(const string& separator) const
{
string scoped;
if (auto container = dynamic_pointer_cast<Contained>(_container))
{
scoped = container->mappedScoped();
scoped = container->mappedScoped(separator);
}
return scoped + "::";
return scoped + separator;
}

string
Expand Down Expand Up @@ -2628,22 +2628,6 @@ Slice::ClassDef::classDataMembers() const
return result;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allClassDataMembers functions were only called by unused variables.
So all dead code.

// Return the class data members of this class and its parent classes, in base-to-derived order.
DataMemberList
Slice::ClassDef::allClassDataMembers() const
{
DataMemberList result;
if (_base)
{
result = _base->allClassDataMembers();
}

// Append this class's class members.
DataMemberList myMembers = classDataMembers();
result.splice(result.end(), myMembers);
return result;
}

bool
Slice::ClassDef::canBeCyclic() const
{
Expand Down Expand Up @@ -3709,24 +3693,6 @@ Slice::Exception::classDataMembers() const
return result;
}

// Return the class data members of this exception and its parent exceptions, in base-to-derived order.
DataMemberList
Slice::Exception::allClassDataMembers() const
{
DataMemberList result;

// Check if we have a base exception. If so, recursively get the class data members of the base exception(s).
if (base())
{
result = base()->allClassDataMembers();
}

// Append this exceptions's class data members.
DataMemberList myMembers = classDataMembers();
result.splice(result.end(), myMembers);
return result;
}

ExceptionPtr
Slice::Exception::base() const
{
Expand Down
8 changes: 3 additions & 5 deletions cpp/src/Slice/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,10 @@ namespace Slice
/// Returns the mapped identifier that this element will use in the target language.
[[nodiscard]] std::string mappedName() const;
/// Returns the mapped scope that this element will be generated in in the target language.
[[nodiscard]] std::string mappedScoped() const;
/// (equivalent to `mappedScope(separator) + mappedName()`).
[[nodiscard]] std::string mappedScoped(const std::string& separator = "::") const;
/// Returns the mapped fully-scoped identifier that this element will use in the target language.
/// (equivalent to `mappedScoped() + mappedName()`).
[[nodiscard]] std::string mappedScope() const;
[[nodiscard]] std::string mappedScope(const std::string& separator = "::") const;

[[nodiscard]] std::string file() const;
[[nodiscard]] int line() const;
Expand Down Expand Up @@ -605,7 +605,6 @@ namespace Slice
[[nodiscard]] DataMemberList orderedOptionalDataMembers() const;
[[nodiscard]] DataMemberList allDataMembers() const;
[[nodiscard]] DataMemberList classDataMembers() const;
[[nodiscard]] DataMemberList allClassDataMembers() const;
[[nodiscard]] bool canBeCyclic() const;
void visit(ParserVisitor* visitor) final;
[[nodiscard]] int compactId() const;
Expand Down Expand Up @@ -787,7 +786,6 @@ namespace Slice
[[nodiscard]] DataMemberList orderedOptionalDataMembers() const;
[[nodiscard]] DataMemberList allDataMembers() const;
[[nodiscard]] DataMemberList classDataMembers() const;
[[nodiscard]] DataMemberList allClassDataMembers() const;
[[nodiscard]] ExceptionPtr base() const;
[[nodiscard]] ExceptionList allBases() const;
[[nodiscard]] bool isBaseOf(const ExceptionPtr& otherException) const;
Expand Down
1 change: 1 addition & 0 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ namespace
return stName;
}

// TODO replace this with mappedScoped("_");
/// Returns the fully scoped name of the provided Slice definition, but scope separators will be replaced with
/// '_' characters (converting a scoped identifier into a single identifier).
string flattenedScopedName(const ContainedPtr& p)
Expand Down
160 changes: 40 additions & 120 deletions cpp/src/slice2cs/CsUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#include "CsUtil.h"
#include "../Slice/MetadataValidation.h"
#include "../Slice/Util.h"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DotNetNames files were only about escaping conflicts with 'built-in' C# methods and properties like ToString, HResult, etc. It was deleted.

#include "DotNetNames.h"
#include "Ice/StringUtil.h"

#include <algorithm>
Expand All @@ -22,38 +21,6 @@ using namespace std;
using namespace Slice;
using namespace IceInternal;

namespace
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's up to users to escape their identifiers if they're using a reserved keyword now.
Just like we did in slice2cpp.

{
string lookupKwd(const string& name, unsigned int baseTypes, bool mangleCasts = false)
{
//
// Keyword list. *Must* be kept in alphabetical order.
//
static const string keywordList[] = {
"abstract", "as", "async", "await", "base", "bool", "break", "byte",
"case", "catch", "char", "checked", "class", "const", "continue", "decimal",
"default", "delegate", "do", "double", "else", "enum", "event", "explicit",
"extern", "false", "finally", "fixed", "float", "for", "foreach", "goto",
"if", "implicit", "in", "int", "interface", "internal", "is", "lock",
"long", "namespace", "new", "null", "object", "operator", "out", "override",
"params", "private", "protected", "public", "readonly", "record", "ref", "required",
"return", "sbyte", "scoped", "sealed", "short", "sizeof", "stackalloc", "static",
"string", "struct", "switch", "this", "throw", "true", "try", "typeof",
"uint", "ulong", "unchecked", "unsafe", "ushort", "using", "var", "virtual",
"void", "volatile", "while"};
bool found = binary_search(&keywordList[0], &keywordList[sizeof(keywordList) / sizeof(*keywordList)], name);
if (found)
{
return "@" + name;
}
if (mangleCasts && (name == "checkedCast" || name == "uncheckedCast"))
{
return string(DotNet::manglePrefix) + name;
}
return Slice::DotNet::mangleName(name, baseTypes);
}
}

string
Slice::CsGenerator::getNamespacePrefix(const ContainedPtr& cont)
{
Expand Down Expand Up @@ -85,52 +52,18 @@ Slice::CsGenerator::getNamespacePrefix(const ContainedPtr& cont)
string
Slice::CsGenerator::getNamespace(const ContainedPtr& cont)
{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixId used to convert :: into . in addition to escaping keywords and such.
Now that it's been delete, the conversion of :: to . has to be done here instead.

string scope = fixId(cont->scope());
if (scope.rfind('.') == scope.size() - 1)
{
scope = scope.substr(0, scope.size() - 1);
}
string prefix = getNamespacePrefix(cont);
if (!prefix.empty())
{
if (!scope.empty())
{
return prefix + "." + scope;
}
else
{
return prefix;
}
}

return scope;
}
assert(!dynamic_pointer_cast<Module>(cont));

string
Slice::CsGenerator::getUnqualified(const string& type, const string& scope, bool builtin)
{
if (type.find('.') != string::npos && type.find(scope) == 0 && type.find('.', scope.size() + 1) == string::npos)
{
return type.substr(scope.size() + 1);
}
else if (builtin)
{
return type.find('.') == string::npos ? type : "global::" + type;
}
else
{
return "global::" + type;
}
string scope = cont->mappedScope(".").substr(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So mapped scope returns ".Foo.Bar.", and that is why we need substr(1) here?

I wonder if we can have a scope prefix, and C# can pass either "" or "global::" and use it for the initial occurrence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct, and also not a bad idea.

But I want to check how often we actually use this. Because the other option is just to remove this global scope piece all together and let callers add it by hand. Because I'm pretty sure we almost always just remove it except for niche things like type-ids.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make the intent clear, otherwise each time you see this code have to wonder why is substr(1) necessary.

Copy link
Member

@bernardnormier bernardnormier Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should either:

  • don't start with the scope separator in the string returned by mappedScope (and update mappedScope doc-comment)
    or
  • add a prefix parameter

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree with @bernardnormier's comment.

All I'm saying is that I lean towards not starting with the scope separator, but I want to wait and see how we're actually using this. I suspect the leading separator is more of a nuisance than a help, but I won't know until I look.

scope.pop_back(); // Remove the trailing '.' on the scope.
string prefix = getNamespacePrefix(cont);
return (prefix.empty() ? scope : prefix + "." + scope);
}

string
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had 2 versions of getUnqualified which I combined into 1 simpler version.

The cost of doing this is that we no remove the Ice. prefix from builtin types like Ice::Object when we're building Ice itself. I can add it back if this is a problem. But removing this simplifies the code and is only useful in the very niche case when you're building Ice itself.

Slice::CsGenerator::getUnqualified(
const ContainedPtr& p,
const string& package,
const string& prefix,
const string& suffix)
Slice::CsGenerator::getUnqualified(const ContainedPtr& p, const string& package)
{
string name = fixId(prefix + p->name() + suffix);
string name = p->mappedName();
string contPkg = getNamespace(p);
if (contPkg == package || contPkg.empty())
{
Expand All @@ -142,42 +75,10 @@ Slice::CsGenerator::getUnqualified(
}
}

//
// If the passed name is a scoped name, return the identical scoped name,
// but with all components that are C# keywords replaced by
// their "@"-prefixed version; otherwise, if the passed name is
// not scoped, but a C# keyword, return the "@"-prefixed name;
// otherwise, check if the name is one of the method names of baseTypes;
// if so, prefix it with ice_; otherwise, return the name unchanged.
//
string
Slice::CsGenerator::fixId(const string& name, unsigned int baseTypes, bool mangleCasts)
Slice::CsGenerator::removeEscapePrefix(const string& identifier)
{
if (name.empty())
{
return name;
}
if (name[0] != ':')
{
return lookupKwd(name, baseTypes, mangleCasts);
}
vector<string> ids = splitScopedName(name);
vector<string> newIds;
newIds.reserve(ids.size());
for (const auto& id : ids)
{
newIds.push_back(lookupKwd(id, baseTypes));
}
stringstream result;
for (auto j = newIds.begin(); j != newIds.end(); ++j)
{
if (j != newIds.begin())
{
result << '.';
}
result << *j;
}
return result.str();
return identifier.find('@') == 0 ? identifier.substr(1) : identifier;
}

string
Expand All @@ -200,7 +101,7 @@ Slice::CsGenerator::getStaticId(const TypePtr& type)
}
else
{
return getUnqualified(cl) + ".ice_staticId()";
return getUnqualified(cl, "") + ".ice_staticId()";
}
}

Expand Down Expand Up @@ -237,11 +138,11 @@ Slice::CsGenerator::typeToString(const TypePtr& type, const string& package, boo
{
if (builtin->kind() == Builtin::KindObject)
{
return getUnqualified(builtinTable[Builtin::KindValue], package, true);
return builtinTable[Builtin::KindValue];
}
else
{
return getUnqualified(builtinTable[builtin->kind()], package, true);
return builtinTable[builtin->kind()];
}
}

Expand All @@ -254,7 +155,7 @@ Slice::CsGenerator::typeToString(const TypePtr& type, const string& package, boo
InterfaceDeclPtr proxy = dynamic_pointer_cast<InterfaceDecl>(type);
if (proxy)
{
return getUnqualified(proxy, package, "", "Prx?");
return getUnqualified(proxy, package) + "Prx?";
}

SequencePtr seq = dynamic_pointer_cast<Sequence>(type);
Expand Down Expand Up @@ -298,7 +199,8 @@ string
Slice::CsGenerator::resultStructName(const string& className, const string& opName, bool marshaledResult)
{
ostringstream s;
s << className << "_" << IceInternal::toUpper(opName.substr(0, 1)) << opName.substr(1)
string fixedOpName = removeEscapePrefix(opName);
s << className << "_" << IceInternal::toUpper(fixedOpName.substr(0, 1)) << fixedOpName.substr(1)
<< (marshaledResult ? "MarshaledResult" : "Result");
return s.str();
}
Expand All @@ -309,7 +211,7 @@ Slice::CsGenerator::resultType(const OperationPtr& op, const string& package, bo
InterfaceDefPtr interface = op->interface();
if (dispatch && op->hasMarshaledResult())
{
return getUnqualified(interface, package, "", resultStructName("", op->name(), true));
return getUnqualified(interface, package) + resultStructName("", op->mappedName(), true);
}

string t;
Expand All @@ -322,7 +224,7 @@ Slice::CsGenerator::resultType(const OperationPtr& op, const string& package, bo
}
else if (op->returnType() || outParams.size() > 1)
{
t = getUnqualified(interface, package, "", resultStructName("", op->name()));
t = getUnqualified(interface, package) + resultStructName("", op->mappedName());
}
else
{
Expand Down Expand Up @@ -654,7 +556,7 @@ Slice::CsGenerator::writeMarshalUnmarshalCode(
DictionaryPtr d = dynamic_pointer_cast<Dictionary>(type);
if (d)
{
helperName = getUnqualified(d, package, "", "Helper");
helperName = getUnqualified(d, package) + "Helper";
}
else
{
Expand Down Expand Up @@ -1025,7 +927,7 @@ Slice::CsGenerator::writeSequenceMarshalUnmarshalCode(
assert(cont);
if (useHelper)
{
string helperName = getUnqualified(seq, scope, "", "Helper");
string helperName = getUnqualified(seq, scope) + "Helper";
if (marshal)
{
out << nl << helperName << ".write(" << stream << ", " << param << ");";
Expand Down Expand Up @@ -1328,7 +1230,6 @@ Slice::CsGenerator::writeSequenceMarshalUnmarshalCode(
}
out << nl << "for (int ix = 0; ix < szx; ++ix)";
out << sb;
string scoped = dynamic_pointer_cast<Contained>(type)->scoped();
// Remove trailing '?'
string nonNullableTypeS = typeS.substr(0, typeS.size() - 1);
out << nl << stream << ".readValue(" << patcherName << '<' << nonNullableTypeS << ">(" << param
Expand Down Expand Up @@ -1551,11 +1452,11 @@ Slice::CsGenerator::writeSequenceMarshalUnmarshalCode(
string helperName;
if (dynamic_pointer_cast<InterfaceDecl>(type))
{
helperName = getUnqualified(dynamic_pointer_cast<InterfaceDecl>(type), scope, "", "PrxHelper");
helperName = getUnqualified(dynamic_pointer_cast<InterfaceDecl>(type), scope) + "PrxHelper";
}
else
{
helperName = getUnqualified(dynamic_pointer_cast<Contained>(type), scope, "", "Helper");
helperName = getUnqualified(dynamic_pointer_cast<Contained>(type), scope) + "Helper";
}

string func;
Expand Down Expand Up @@ -1921,6 +1822,25 @@ Slice::CsGenerator::validateMetadata(const UnitPtr& u)
};
knownMetadata.emplace("cs:generic", genericInfo);

// "cs:identifier"
MetadataInfo identifierInfo = {
.validOn =
{typeid(InterfaceDecl),
typeid(Operation),
typeid(ClassDecl),
typeid(Slice::Exception),
typeid(Struct),
typeid(Sequence),
typeid(Dictionary),
typeid(Enum),
typeid(Enumerator),
typeid(Const),
typeid(Parameter),
typeid(DataMember)},
.acceptedArgumentKind = MetadataArgumentKind::SingleArgument,
};
knownMetadata.emplace("cs:identifier", std::move(identifierInfo));

// "cs:namespace"
MetadataInfo namespaceInfo = {
.validOn = {typeid(Module)},
Expand Down
Loading
Loading