Add Support for `cs:identifier` Metadata #3445

InsertCreativityHere · 2025-01-29T16:21:14Z

This PR adds support for cs:identifier in the same was as cpp:identifier.
And removes all the logic for escaping keywords/name collisions.

InsertCreativityHere · 2025-01-29T16:22:25Z

cpp/src/Slice/Parser.cpp

@@ -2612,22 +2612,6 @@ Slice::ClassDef::classDataMembers() const
    return result;
 }



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

InsertCreativityHere · 2025-01-29T16:23:34Z

cpp/src/slice2cs/CsUtil.cpp

@@ -3,7 +3,6 @@
 #include "CsUtil.h"
 #include "../Slice/MetadataValidation.h"
 #include "../Slice/Util.h"


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

InsertCreativityHere · 2025-01-29T16:24:06Z

cpp/src/slice2cs/CsUtil.cpp

@@ -22,38 +21,6 @@ using namespace std;
 using namespace Slice;
 using namespace IceInternal;

-namespace


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

InsertCreativityHere · 2025-01-29T16:25:11Z

cpp/src/slice2cs/CsUtil.cpp

@@ -85,52 +52,42 @@ Slice::CsGenerator::getNamespacePrefix(const ContainedPtr& cont)
 string
 Slice::CsGenerator::getNamespace(const ContainedPtr& cont)
 {


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.

InsertCreativityHere · 2025-01-29T16:28:44Z

cpp/src/slice2cs/CsUtil.cpp

        }
        else
        {
            return prefix;
        }
    }

-    return scope;
+    return fixedScope;
 }

 string


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.

InsertCreativityHere · 2025-01-29T16:37:20Z

cpp/src/slice2cs/Gen.h

@@ -44,12 +44,10 @@ namespace Slice

        static std::string getParamAttributes(const ParameterPtr&);

-        std::string writeValue(const TypePtr&, const std::string&);


Dead function.

InsertCreativityHere · 2025-01-29T16:37:47Z

cpp/src/slice2cs/Gen.h

        void writeConstantValue(const TypePtr&, const SyntaxTreeBasePtr&, const std::string&);

        // Generates "= null!" for non-nullable fields (Slice class and exception only).
-        void writeDataMemberInitializers(const DataMemberList&, unsigned int);
+        void writeDataMemberInitializers(const DataMemberList&);


This unsigned int was a bit-flag we used to determine which category of names to escape.

InsertCreativityHere · 2025-01-29T16:38:23Z

cpp/src/slice2java/Gen.cpp

@@ -1618,9 +1618,6 @@ Slice::JavaVisitor::writeMarshaling(Output& out, const ClassDefPtr& p)
    }
    out << eb;



All these variables were unused.
I found them when checking allClassDataMembers.

InsertCreativityHere · 2025-01-29T16:39:09Z

csharp/test/Slice/escape/Client.cs

@@ -2,7 +2,7 @@

 public class Client : Test.TestHelper
 {
-    public sealed class caseI : @abstract.caseDisp_
+    public sealed class caseI : cs_abstract.caseDisp_


I chose cs_abstract for now until we decide on how to escape module names.

InsertCreativityHere · 2025-01-29T16:41:47Z

cpp/src/slice2cs/Gen.cpp

@@ -45,19 +44,20 @@ namespace
        string result = "<see cref=\"";
        if (!identifier.empty())


This is the comment-link-callback.
It should be calling mappedName but cannot.
My other PR fixes this, so don't worry about it.

InsertCreativityHere · 2025-01-29T17:05:18Z

cpp/src/slice2cs/Gen.cpp

@@ -605,69 +601,13 @@ Slice::CsVisitor::emitNonBrowsableAttribute()
         << "[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]";
 }

-string


Uncalled function.

InsertCreativityHere · 2025-01-29T17:06:25Z

cpp/src/slice2cs/Gen.cpp

+            string fixedParamName = addPrefixToIdentifier("", param->mappedName()); // Strip off any leading '@'s.
+            _out << nl << "/// <param name=\"" << fixedParamName << "\">";


I checked and doc-comment links to parameters cannot start with @.
The link should just be to the actual identifier, without any escaping prefixes.

pepone · 2025-01-29T17:58:00Z

cpp/src/slice2cs/CsUtil.cpp

    string prefix = getNamespacePrefix(cont);
    if (!prefix.empty())
    {
-        if (!scope.empty())
+        if (!fixedScope.empty())


Can fixedScope be empty?

Theoritically it could if you passed a top-level module into this function.
But after reviewing it, we never pass a module in here.

I added an assert(!dynamic_pointer_cast<Module>(p)) to the top of the function and simplified the code.

pepone · 2025-01-29T17:59:59Z

cpp/src/slice2cs/CsUtil.cpp

 string
-Slice::CsGenerator::fixId(const string& name, unsigned int baseTypes, bool mangleCasts)
+Slice::CsGenerator::getMappedScoped(const ContainedPtr& p)


seems confusing to have getMappedScoped and mappedScoped in the parser.

I agree.

The mappedScope and mappedScoped in the Parser don't make much sense since they use ::.

The scope is not :: in all languages and we don't have a mechanism to pass the language scope separator from the Slice compiler to the Parser.

Yes, good point.

I fixed mappedScoped (the parser function) to accept a separator argument.
Now C# can just pass "." and let the parser handle it. Much cleaner.

pepone · 2025-01-29T18:01:04Z

cpp/src/slice2cs/CsUtil.cpp

-        "Ice.Object", // not used anymore
-        "Ice.ObjectPrx?",
-        "Ice.Value?"};
+        "global::Ice.Object", // not used anymore


I thought we decided to not use global:: for Ice.

Oh okay, I removed the global:: prefixes.

pepone · 2025-01-29T18:02:11Z

cpp/src/slice2cs/CsUtil.cpp

@@ -298,7 +243,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 = addPrefixToIdentifier("", opName); // Strip any leading '@' from the name.


I don't like using empty prefix as a mean of removing @

Use my helper function.

I used your helper function.
But named it removeEscapePrefix instead of removeAtPrefix because the latter sounds like it's removing something at a position, which it isn't.

pepone · 2025-01-29T18:07:20Z

cpp/src/slice2cs/Gen.cpp

@@ -1874,7 +1762,7 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
            true);
        emitObsoleteAttribute(p, _out);
        _out << nl << taskResultType(p, ns);
-        _out << " " << p->name() << "Async" << spar << inParams
+        _out << " " << name << "Async" << spar << inParams


Don't we nee to remove @ if present in name?

We can but it's optional. @ creates a syntax error only when it's in the middle of an identifier. It's ignored as prefix.

The approach that I took was to only remove the @ when absolutely necessary and for internal code that users won't be seeing.

I think it's desirable for cs:identifier to be respected as much as possible. And if you put cs:identifier:@throw we will use @throw everywhere, even if the escape character isn't strictly necessary there.

The less magical cs:identifier is the better IMO.

cpp/src/slice2cs/Gen.cpp

bernardnormier · 2025-01-29T18:37:04Z

cpp/src/slice2cs/CsUtil.cpp

 string
-Slice::CsGenerator::fixId(const string& name, unsigned int baseTypes, bool mangleCasts)
+Slice::CsGenerator::getMappedScoped(const ContainedPtr& p)


I agree.

The mappedScope and mappedScoped in the Parser don't make much sense since they use ::.

The scope is not :: in all languages and we don't have a mechanism to pass the language scope separator from the Slice compiler to the Parser.

bernardnormier · 2025-01-29T18:38:42Z

cpp/src/slice2cs/CsUtil.cpp

    stringstream result;
-    for (auto j = newIds.begin(); j != newIds.end(); ++j)
+
+    string name = p->mappedScoped();


I find this code rather confusing: it operates on a ::-separated name with the mapped C# name at the end.

I removed this function.
Now you can tell the parser which scope-separator you want when you call mappedScoped.

bernardnormier · 2025-01-29T18:41:09Z

cpp/src/slice2cs/CsUtil.cpp

+string
+Slice::CsGenerator::addPrefixToIdentifier(const string& prefix, const string& ident)
+{
+    return prefix + (ident.find('@') == 0 ? ident.substr(1) : ident);
+}
+


I would add a helper function to remove the @, like:

string removeAtPrefix(const string&)

bernardnormier · 2025-01-29T18:42:25Z

cpp/src/slice2cs/CsUtil.cpp

@@ -298,7 +243,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 = addPrefixToIdentifier("", opName); // Strip any leading '@' from the name.


Use my helper function.

InsertCreativityHere · 2025-01-29T19:28:54Z

I changed the signature of mappedScope to accept a const string& separator argument.

This way the languages don't need to replace all the :: with their separator of choice.
Instead they tell the parser what they want, and let it handle that.

bernardnormier · 2025-01-29T20:16:09Z

I changed the signature of mappedScope to accept a const string& separator argument.

This way the languages don't need to replace all the :: with their separator of choice. Instead they tell the parser what they want, and let it handle that.

Should this be instead a parameter of createUnit?

The implementation queries xxx:identifier based on a fixed xxx provided to createUnit, but then requires slice2xxx to provide its scope separator every time it wants a mapped scope? This doesn't sound very consistent.

InsertCreativityHere · 2025-01-29T20:21:38Z

It's not something we call often, in slice2cs we only call it from 4 places, so it's not a huge burden.
It's rare, but we don't always use the same separator. There's two places in slice2cpp where we actually want _ as our separator (instead of '::') because we want a 'flattened' name.

So, I'm inclined to leave it this way until we're passing it many times and it becomes a hassle to deal with.
Which might happen as I do the other language mappings, but so far, it hasn't IMO.

pepone · 2025-01-30T08:42:51Z

cpp/src/slice2cs/CsUtil.cpp

-    {
-        return "global::" + type;
-    }
+    string scope = cont->mappedScope(".").substr(1);


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.

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.

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

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

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.

InsertCreativityHere commented Jan 29, 2025

View reviewed changes

InsertCreativityHere marked this pull request as ready for review January 29, 2025 17:03

InsertCreativityHere commented Jan 29, 2025

View reviewed changes

InsertCreativityHere requested review from bernardnormier, pepone and externl January 29, 2025 17:08

pepone reviewed Jan 29, 2025

View reviewed changes

bernardnormier approved these changes Jan 29, 2025

View reviewed changes

InsertCreativityHere requested a review from pepone January 29, 2025 20:03

pepone approved these changes Jan 30, 2025

View reviewed changes

externl approved these changes Jan 30, 2025

View reviewed changes

InsertCreativityHere added 15 commits January 30, 2025 12:27

Step 1.

5590716

Let's just always qualift built-in our types with 'global::'

01dc1cb

Remapping calls and removing dead code part 1.

710f44b

Finished fixing all the calls and removing some dead code.

3bdafa4

Fixed the header file.

55c9add

Added support for 'cs:identifier' metadata.

efa5031

Fixed the C#/Ice/servantLocator test.

1d40420

Finished changes to the 'slice2cs'.

768ff58

Final self-review.

fda72a1

Eat any leading '@' characters when prefixing things.

e031cd5

Bug fixes.

13ab236

Fixed the escape tests.

72d093b

Formatting and added some doc-comment testing.

d684620

You can specify which separator you want 'mappedScoped' to use now.

fd8d009

Review fixes.

c16f28f

InsertCreativityHere added 2 commits January 30, 2025 12:28

Forgot to strip leading scope separator in some places.

87313ca

More formatting...

fb54fcf

InsertCreativityHere force-pushed the cs-identifier branch from d019364 to fb54fcf Compare January 30, 2025 17:34

InsertCreativityHere mentioned this pull request Jan 30, 2025

Should We Fix scope and scoped in the Slice Parser to no Longer Emit a Leading Scope? #3456

Open

InsertCreativityHere merged commit 26bea31 into zeroc-ice:main Jan 30, 2025
22 of 23 checks passed

		@@ -2612,22 +2612,6 @@ Slice::ClassDef::classDataMembers() const
		return result;
		}

		@@ -44,12 +44,10 @@ namespace Slice

		static std::string getParamAttributes(const ParameterPtr&);

		std::string writeValue(const TypePtr&, const std::string&);

		@@ -1618,9 +1618,6 @@ Slice::JavaVisitor::writeMarshaling(Output& out, const ClassDefPtr& p)
		}
		out << eb;

		@@ -45,19 +44,20 @@ namespace
		string result = "<see cref=\"";
		if (!identifier.empty())

		string fixedParamName = addPrefixToIdentifier("", param->mappedName()); // Strip off any leading '@'s.
		_out << nl << "/// <param name=\"" << fixedParamName << "\">";

Add Support for cs:identifier Metadata #3445

Add Support for cs:identifier Metadata #3445

Conversation

InsertCreativityHere commented Jan 29, 2025 • 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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InsertCreativityHere commented Jan 29, 2025

bernardnormier commented Jan 29, 2025

InsertCreativityHere commented Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add Support for `cs:identifier` Metadata #3445

Add Support for `cs:identifier` Metadata #3445

InsertCreativityHere commented Jan 29, 2025 •

edited

Loading

InsertCreativityHere commented Jan 29, 2025 •

edited

Loading

bernardnormier Jan 30, 2025 •

edited

Loading