Use the new MetadataValidator in C#, Java, JavaScript, Ruby, PHP, and MATLAB #3256

InsertCreativityHere · 2024-12-11T16:19:13Z

C++ and Parser Metadata

all that changed was some style for consistency and readability.

MATLAB, PHP, and Ruby

We don't support any language-specific metadata in these languages. But now we will explicitly reject metadata for these
languages, whereas before we were just silently ignoring it (ie. if a user put php:type now we reject it).

JS

We had no validation of javascipt metadata before this PR. Now we do.
Previously, the code-gen allowed js:defined-in on any user-defined type, now we restrict it to only classes and interfaces.
The docs say that this metadata is for forward-declarations, so we should only support it on forward-declarable types.

C#

We are now validating metadata on interfaces (we weren't before)
We are now validating inner metadata on sequences, dictionaries, and constants (we weren't before)
slice2cs was validating file metadata in #included files? (we no longer do this)
Removed validation for the 'cs:type' metadata which may or may not of existed?

Java

We are now validating metadata on interfaces (we weren't before)
We are now validating inner metadata on dictionaries, and constants (we weren't before)
slice2java was validating file metadata in #included files? (we no longer do this)
Removed 'validation' for these dead directives: ["java:tie", "java:optional"]
Restricted the 'java:serialVersionUID' metadata to only be allowed on 'class', 'exception' and 'struct' (the only things where we check for it).
Disallow using both 'java:type' and 'java:buffer' on the same sequence; both compete for the mapped type.

InsertCreativityHere · 2024-12-11T16:20:10Z

cpp/src/Slice/MetadataValidation.cpp

@@ -73,68 +73,62 @@ Slice::validateMetadata(const UnitPtr& p, string_view prefix, map<string, Metada
    // So, we add all the language-agnostic metadata validation into the provided list.

    // "amd"


All that I changed here was the style. Compared to brace initialization, I found that setting each field is:

more readable, because you can see the field names to know what is being set.

reduces clutter, by only settings the fields I care about, instead of having to repeat default values just so I can set a field which is further down the struct.

I prefer to use structure brace initialization. You an specify the field names when initializing a struct.

InsertCreativityHere · 2024-12-11T16:20:59Z

cpp/src/Slice/MetadataValidation.cpp

@@ -350,7 +344,7 @@ MetadataVisitor::isMetadataValid(const MetadataPtr& metadata, const SyntaxTreeBa

    // Fourth, we check what the metadata was applied to - does that Slice definition support this metadata?
    SyntaxTreeBasePtr appliedTo;
-    if (info.acceptedContexts == MetadataApplicationContext::Definitions)


I also changed the name acceptedContexts to be singular.

Making it plural gives the impressions this enum is a bit-flag, where you can 'set multiple contexts'.
That's not what it is; you can only set a single value for it.

InsertCreativityHere · 2024-12-11T16:21:53Z

cpp/src/Slice/Parser.cpp

@@ -105,7 +105,7 @@ Slice::Metadata::Metadata(string rawMetadata, string file, int line) : GrammarBa
    {
        // Check if the metadata starts with a language prefix.
        // NOTE: It is important that this list is kept in alphabetical order!
-        constexpr string_view languages[] = {"cpp", "cs", "java", "js", "matlab", "php", "python", "ruby", "swift"};
+        constexpr string_view languages[] = {"cpp", "cs", "java", "js", "matlab", "php", "python", "rb", "swift"};


These should match the names of their respective compilers.
(Except python, where we used an inconsistent name for it's metadata...)

InsertCreativityHere · 2024-12-11T16:22:08Z

cpp/src/slice2cpp/Gen.cpp

@@ -859,33 +859,28 @@ Slice::Gen::validateMetadata(const UnitPtr& u)
    map<string, MetadataInfo> knownMetadata;

    // "cpp:array"


Same here. all that changed was the style.

Ditto for my earlier commit. Another option is to add a constructor.

Fixed (by using brace initialization).

InsertCreativityHere · 2024-12-11T16:24:51Z

cpp/src/slice2java/JavaUtil.h

@@ -230,7 +227,7 @@ namespace Slice
        // metadata of the type's original definition, as well as any optional
        // metadata that typically represents a data member or parameter.
        //
-        static bool hasTypeMetadata(const TypePtr&, const MetadataList& = MetadataList());
+        static bool hasTypeMetadata(const SequencePtr&, const MetadataList& = MetadataList());


This function is only called on sequences and is all about checking for sequence metadata.

InsertCreativityHere · 2024-12-11T17:00:34Z

cpp/src/slice2java/JavaUtil.cpp

+            // Check if either 1) No type metadata was applied to this sequence at all or 2) 'java:type' was applied to
+            // where the sequence is used (which overrides any metadata on the definition) or 3) 'java:type' was applied
+            // to the sequence definition, and there is no metadata overriding it where the sequence is used.


An unrelated bug I stumbled across. We weren't always checking for these metadata in the same order, which could cause us to generate uncompilable code(because the types wouldn't match). Pretty niche corner case, but still.

You'll see me messing around with java:type, java:buffer, and java:serializable a few times in this file.
All I'm doing is making sure we check them in a consistent (and correct) order.

InsertCreativityHere · 2024-12-11T17:02:52Z

cpp/src/slice2java/JavaUtil.cpp

-    BuiltinPtr builtin = dynamic_pointer_cast<Builtin>(seq->type());
-    if (builtin && builtin->kind() == Builtin::KindByte)


The validator already ensures that java:serializable only goes on byte sequences.
No need to repeat the validation here.

InsertCreativityHere · 2024-12-11T17:04:38Z

cpp/src/slice2java/JavaUtil.cpp

-    if (builtin && (builtin->kind() == Builtin::KindByte || builtin->kind() == Builtin::KindShort ||
-                    builtin->kind() == Builtin::KindInt || builtin->kind() == Builtin::KindLong ||
-                    builtin->kind() == Builtin::KindFloat || builtin->kind() == Builtin::KindDouble))


Same. No need to repeat the validation here.

InsertCreativityHere · 2024-12-11T17:05:56Z

cpp/src/slice2java/JavaUtil.cpp

@@ -2192,49 +1809,16 @@ Slice::JavaGenerator::getTypeMetadata(const MetadataList& metadata, string& inst
 }

 bool
-Slice::JavaGenerator::hasTypeMetadata(const TypePtr& type, const MetadataList& localMetadata)


Two simplifications:

This is only called for sequences, so we can eliminate alot of casting/checking of types.

This function included validation of the metadata, which was already being performed by the validator.

InsertCreativityHere · 2024-12-11T17:06:10Z

cpp/src/slice2java/JavaUtil.cpp


-        if ((builtin->kind() == Builtin::KindByte || builtin->kind() == Builtin::KindShort ||


Same, no need to repeat validation again.

Copilot reviewed 2 out of 17 changed files in this pull request and generated no suggestions.

Files not reviewed (15)

cpp/src/Slice/MetadataValidation.cpp: Language not supported
cpp/src/Slice/MetadataValidation.h: Language not supported
cpp/src/Slice/Parser.cpp: Language not supported
cpp/src/slice2cpp/Gen.cpp: Language not supported
cpp/src/slice2cs/CsUtil.cpp: Language not supported
cpp/src/slice2cs/CsUtil.h: Language not supported
cpp/src/slice2java/Gen.cpp: Language not supported
cpp/src/slice2java/JavaUtil.h: Language not supported
cpp/src/slice2js/Gen.cpp: Language not supported
cpp/src/slice2js/Gen.h: Language not supported
cpp/src/slice2matlab/Main.cpp: Language not supported
cpp/src/slice2php/Main.cpp: Language not supported
cpp/src/slice2php/PHPUtil.h: Language not supported
cpp/src/slice2rb/RubyUtil.cpp: Language not supported
cpp/src/slice2rb/RubyUtil.h: Language not supported

externl · 2024-12-11T19:11:59Z

cpp/src/Slice/MetadataValidation.cpp

@@ -73,68 +73,62 @@ Slice::validateMetadata(const UnitPtr& p, string_view prefix, map<string, Metada
    // So, we add all the language-agnostic metadata validation into the provided list.

    // "amd"


I prefer to use structure brace initialization. You an specify the field names when initializing a struct.

externl · 2024-12-11T19:12:49Z

cpp/src/slice2cpp/Gen.cpp

@@ -859,33 +859,28 @@ Slice::Gen::validateMetadata(const UnitPtr& u)
    map<string, MetadataInfo> knownMetadata;

    // "cpp:array"


Ditto for my earlier commit. Another option is to add a constructor.

externl · 2024-12-11T20:03:57Z

cpp/src/slice2java/JavaUtil.cpp

+            os << "failed to parse custom serialVersionUID value for " << p->kindOf() << " `" << p->scoped()
+               << "'; generating default value";


Not related to your PR, I think this should be an error, not a warning.

externl · 2024-12-11T20:11:23Z

cpp/src/slice2java/JavaUtil.cpp

+    MetadataInfo serialVersionUIDInfo;
+    serialVersionUIDInfo.validOn = {typeid(ClassDecl), typeid(Slice::Exception), typeid(Struct)};
+    serialVersionUIDInfo.acceptedArgumentKind = MetadataArgumentKind::SingleArgument;
+    knownMetadata.emplace("java:serialVersionUID", std::move(serialVersionUIDInfo));


should we do the validation serialVersionUIDInfo here in an extraValidation function?

Good idea!
I moved the extra stoll check into the metadata validation, instead of doing it on the spot.

pepone · 2024-12-12T09:08:03Z

cpp/src/slice2cs/CsUtil.cpp

+    // "cs:attribute"
+    MetadataInfo attributeInfo;
+    attributeInfo.validOn = {
+        typeid(Unit),


Does Unit correspond with file scoped metadata?

Yes, that is exactly correct.

pepone · 2024-12-12T09:09:01Z

cpp/src/slice2cs/CsUtil.cpp

+        typeid(Operation),
+        typeid(Slice::Exception),
+        typeid(Struct),
+        typeid(Enum),


Are we missing Enumerators here?

Yes, there's an open issue for adding it (#3246), but I didn't want to change too much in this PR.
So my validator stays as close to the original validation, including any mistakes that were in it.

After I've updated the compilers to use my MetadataValidator, then I'll go back and fix all the bugs that it's uncovered.

pepone · 2024-12-12T09:17:02Z

cpp/src/slice2cs/CsUtil.cpp

+        if (auto mod = dynamic_pointer_cast<Module>(p))
+        {
+            // Top-level modules are contained by the 'Unit'. Non-top-level modules are contained in 'Module's.
+            if (!dynamic_pointer_cast<Unit>(mod->container()))


I would merge the two nested if clauses using && condition.

Changed as suggested.

pepone · 2024-12-12T09:24:13Z

cpp/src/slice2java/Gen.cpp

@@ -3703,17 +3703,13 @@ void
 Slice::Gen::HelperVisitor::visitSequence(const SequencePtr& p)
 {
    BuiltinPtr builtin = dynamic_pointer_cast<Builtin>(p->type());
-    if (!hasTypeMetadata(p) && builtin && builtin->kind() <= Builtin::KindString)
+    bool mappedToCustomType = p->hasMetadata("java:type");
+    if (builtin && builtin->kind() < Builtin::KindObject && !mappedToCustomType)


builtin && builtin->kind() < Builtin::KindObject Don't we have a more idiomatic way to do this check? It is not clear what it means < Builtin::KindObject

We did not, but I added a method named mapsToJavaBuiltinType that we call now instead.
It's the same logic, but hopefully the name makes it more digestable.

pepone · 2024-12-12T09:28:42Z

cpp/src/slice2java/Gen.cpp

-    if (!hasTypeMetadata(p) && builtin && builtin->kind() <= Builtin::KindString)
+    bool mappedToCustomType = p->hasMetadata("java:type");
+    if (builtin && builtin->kind() < Builtin::KindObject && !mappedToCustomType)
    {
-        return; // No helpers for sequence of primitive types
-    }
-    else if (hasTypeMetadata(p) && !p->hasMetadata("java:type"))
-    {
-        return; // No helpers for custom metadata other than java:type


Let me know if I get this right:

The previous check for the second if applies to non built-in types before, With the new check there is no need for this because java:bufer and java:serializable only apply to sequences of built-in types?

pepone · 2024-12-12T09:33:08Z

cpp/src/slice2java/JavaUtil.cpp

-                os << "ignoring invalid serialVersionUID for " << p->kindOf() << " `" << p->scoped()
-                   << "'; generating default value";
+            ostringstream os;
+            os << "failed to parse custom serialVersionUID value for " << p->kindOf() << " `" << p->scoped()


Suggested change

os << "failed to parse custom serialVersionUID value for " << p->kindOf() << " `" << p->scoped()

os << "failed to parse custom serialVersionUID value for " << p->kindOf() << " '" << p->scoped()

I went with @externl's suggestion and moved this check into the metadata validation section,
and I used single quotes (as I always do when writing new code).

pepone · 2024-12-12T09:45:21Z

cpp/src/slice2java/JavaUtil.cpp

-                    builtin->kind() == Builtin::KindInt || builtin->kind() == Builtin::KindLong ||
-                    builtin->kind() == Builtin::KindFloat || builtin->kind() == Builtin::KindDouble))
+    static const string bytebuffer = "java:buffer";
+    if ((seq->hasMetadata(bytebuffer) || hasMetadata(bytebuffer, metadata)) && !hasMetadata("java:type", metadata))


&& !hasMetadata("java:type", metadata) is this to ensure java:type metadata in the parameter overrides java:buffer in the definition?

Do we allow both java:buffer and java:type in the same definition? Or is just one in the definition and override it in the param with another?

is this to ensure java:type metadata in the parameter overrides java:buffer in the definition?

Precisely. But more generally we want to make sure that parameter metadata always overrides definition metadata.

Do we allow both java:buffer and java:type in the same definition?

No, we explicitly check that you can only have one of java:type, java:buffer or java:serializable on your definition. The only way to have "two of them" is by putting one on the definition and one on the parameter.
And in this case, parameter metadata is always given priority.

So we know that if seq->hasMetadata(bytebuffer) is true, then the only place we need to check is the parameter's metadata (and for completeness, java:serializable can only go on the definition, so the only thing left to check for is whether java:type was put on the parameter).

pepone · 2024-12-12T09:52:44Z

cpp/src/slice2java/JavaUtil.cpp

+
+            // This check for 'hasMetadata("java:type")' looks redundant, but is necessary to ensure
+            // that 'java:type' was placed on the _definition_ and not on where the sequence was used.
+            if (seq->hasMetadata("java:type") && seq->hasMetadata("java:buffer"))


Why is the check seq->hasMetadata("java:buffer") isn't this validator only called for java:buffer? Or is the comment above supposed to refer to java:buffer instead of java:type?

Good catch, the comment was supposed to refer to java:buffer, I got myself mixed up.
Fixed.

pepone · 2024-12-12T09:55:40Z

cpp/src/slice2java/JavaUtil.cpp

+        if (auto mod = dynamic_pointer_cast<Module>(p))
+        {
+            // Top-level modules are contained by the 'Unit'. Non-top-level modules are contained in 'Module's.
+            if (!dynamic_pointer_cast<Unit>(mod->container()))


I would merge the nested if conditions here too.

Also changed!

bernardnormier · 2024-12-12T15:28:52Z

cpp/src/slice2cs/CsUtil.cpp

+    MetadataInfo attributeInfo;
+    attributeInfo.validOn = {
+        typeid(Unit),
+        typeid(Module),


.NET does not support attributes on C# namespaces:
https://learn.microsoft.com/en-us/dotnet/api/system.attributetargets?view=net-9.0

So this looks bogus.

Agreed, this was the old behavior which I just tried to preserve.
I added this to the C# metadata issue I already opened: #3246 (comment)

bernardnormier · 2024-12-12T15:32:05Z

cpp/src/slice2cs/CsUtil.cpp

-                    continue;
-                }
+                return "'cs:generic:" + argument +
+                       "' is not supported on sequences of objects; only 'List' is supported for object sequences";


We should not use 'objects', as it's confusing. sequence of classes? sequence of class instances?

I went with sequences of classes types, I can see how "objects" would be confusing. Now it reads:

"'cs:generic:" + argument + "' is not supported on sequences of classes; only 'List' is supported for such sequences"

bernardnormier · 2024-12-12T15:34:05Z

cpp/src/slice2cs/CsUtil.cpp

+
+    // "cs:property"
+    MetadataInfo propertyInfo;
+    propertyInfo.validOn = {typeid(ClassDecl), typeid(Slice::Exception), typeid(Struct)};


For validOn, we must used ClassDecl/InterfaceDecl and not ClassDef/InterfaceDef?

Yes, after my PR for simplifying the Decl/Def situation, you must always pass in the Decls.
There's a comment saying so in MetadataValidator.h:

ice/cpp/src/Slice/MetadataValidation.h

Lines 68 to 70 in 6e237b5

/// Note that `ClassDef` and `InterfaceDef` should never appear in this list, since class & interface metadata

/// is always stored on the corresponding `ClassDecl` and `InterfaceDecl` types, which should be used instead.

std::list<std::reference_wrapper<const std::type_info>> validOn;

bernardnormier · 2024-12-12T15:43:18Z

cpp/src/slice2java/JavaUtil.cpp

+    getsetInfo.acceptedArgumentKind = MetadataArgumentKind::NoArguments;
+    knownMetadata.emplace("java:getset", std::move(getsetInfo));
+
+    // "java:implements"


How does this work?

Without partial (like in C#), how is the user going to implement any of the interface methods?

From the docs:

When used with structs, this metadata can only refer to interfaces without operations. With classes or interfaces, the generated Java interface will be marked as abstract. The code is responsible for registering a value factory if the Slice class is transferred over-the-wire and uses this metadata to implement native Java interfaces.

To be clear: even though it's documented, it's not usable and should be removed.

…oc-ice/ice#, Java, JavaScript, Ruby, PHP, and MATLAB (zeroc-ice/ice#3256)

InsertCreativityHere added 7 commits December 9, 2024 16:53

Added metadata validation for the simpler stuff.

762b334

Did slice2js

ff6ca06

Switch the declaration style we were using for cpp.

99ed644

Also refactor the style used in the parser as well.

3eb18f3

forgot an equals sign.

7cb146e

Unrelated cleanup.

2ad9856

Did cs, java, and js.

f34b90f

InsertCreativityHere commented Dec 11, 2024

View reviewed changes

Self review fixes.

85e74ca

InsertCreativityHere marked this pull request as ready for review December 11, 2024 17:11

InsertCreativityHere requested review from bernardnormier, pepone and externl December 11, 2024 17:12

externl requested a review from Copilot December 11, 2024 19:07

Copilot AI reviewed Dec 11, 2024

View reviewed changes

externl approved these changes Dec 11, 2024

View reviewed changes

pepone reviewed Dec 12, 2024

View reviewed changes

bernardnormier approved these changes Dec 12, 2024

View reviewed changes

InsertCreativityHere added 4 commits December 16, 2024 13:54

Review fixes.

3ed45ad

Formatting...

bbe9493

Ubuntu handles the field initialization syntax differently.

4a37234

Another thing that Ubuntu disagrees with Windows on...

1b6d671

InsertCreativityHere requested a review from pepone December 16, 2024 20:13

pepone approved these changes Dec 16, 2024

View reviewed changes

InsertCreativityHere merged commit d9a2cd9 into zeroc-ice:main Dec 16, 2024
19 checks passed

InsertCreativityHere added a commit to InsertCreativityHere/compiler-comparison that referenced this pull request Jan 1, 2025

HEAD:(zeroc-ice/ice@d9a2cd9f0f) Use the new MetadataValidator in Czer…

8ad5418

…oc-ice/ice#, Java, JavaScript, Ruby, PHP, and MATLAB (zeroc-ice/ice#3256)

		@@ -73,68 +73,62 @@ Slice::validateMetadata(const UnitPtr& p, string_view prefix, map<string, Metada
		// So, we add all the language-agnostic metadata validation into the provided list.

		// "amd"

		@@ -859,33 +859,28 @@ Slice::Gen::validateMetadata(const UnitPtr& u)
		map<string, MetadataInfo> knownMetadata;

		// "cpp:array"

		BuiltinPtr builtin = dynamic_pointer_cast<Builtin>(seq->type());
		if (builtin && builtin->kind() == Builtin::KindByte)


		if ((builtin->kind() == Builtin::KindByte \|\| builtin->kind() == Builtin::KindShort \|\|

		os << "failed to parse custom serialVersionUID value for " << p->kindOf() << " `" << p->scoped()
		<< "'; generating default value";

	/// Note that `ClassDef` and `InterfaceDef` should never appear in this list, since class & interface metadata
	/// is always stored on the corresponding `ClassDecl` and `InterfaceDecl` types, which should be used instead.
	std::list<std::reference_wrapper<const std::type_info>> validOn;

Use the new MetadataValidator in C#, Java, JavaScript, Ruby, PHP, and MATLAB #3256

Use the new MetadataValidator in C#, Java, JavaScript, Ruby, PHP, and MATLAB #3256

Conversation

InsertCreativityHere commented Dec 11, 2024 • edited Loading

C++ and Parser Metadata

MATLAB, PHP, and Ruby

JS

C#

Java

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

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 Dec 11, 2024 •

edited

Loading