Another attempt

zeroc-ice · Dec 4, 2024 · 23ba5ed · 23ba5ed
1 parent b463378
commit 23ba5ed
Show file tree

Hide file tree

Showing 10 changed files with 158 additions and 133 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -21,67 +21,21 @@ jobs:
       matrix:
         include:
           # Release builds
-          - os: macos-15
-            config: "release"
           - os: ubuntu-24.04
             config: "release"
-          - os: windows-2022
-            config: "release"
-            build_flags: "/p:Platform=x64"
-            test_flags: "--platform=x64"
-          - os: windows-2022
-            config: "cpp-win32-release"
-            working_directory: "cpp"
-            build_flags: "/p:Platform=Win32"
-            msbuild_project: "msbuild/ice.proj"
-            test_flags: "--platform=Win32"
 
           # Debug builds
-          - os: macos-15
-            config: "debug"
-            build_flags: "OPTIMIZE=no"
-            test_flags: "--swift-config=debug"
           - os: ubuntu-24.04
             config: "debug"
             build_flags: "OPTIMIZE=no"
-          # TODO - figure out how to properly install debug Python
-          - os: windows-2022
-            config: "debug"
-            working_directory: "cpp"
-            build_flags: "/p:Platform=x64 /p:Configuration=Debug"
-            test_flags: "--platform=x64 --config=Debug"
-            msbuild_project: "msbuild/ice.proj"
-
-          # # Xcode SDK builds
-          # # TODO - Should we also test the debug config here as well?
-          # - macos-15
-          #   config: "xcodesdk"
-          #   working_directory: "cpp"
-          #   build_flags: "CONFIGS=xcodesdk PLATFORMS=iphonesimulator"
-          #   test_flags: "--config=xcodesdk --platform=iphonesimulator --controller-app"
-          #   build_cpp_and_python: true
-
-          # MATLAB
-          - os: ubuntu-24.04
-            config: "matlab"
-            working_directory: "matlab"
-            build_cpp_and_python: true
-          - os: windows-2022
-            config: "matlab"
-            working_directory: "matlab"
-            build_flags: "/p:Platform=x64"
-            msbuild_project: "msbuild/ice.proj"
-            test_flags: "--platform=x64"
-            build_cpp_and_python: true
 
           # Cross tests
           - os: ubuntu-24.04
             config: "cross"
-            test_flags: "--all-cross"
+            test_flags: "--all-cross --filter ruby"
           - os: macos-15
             config: "cross"
-            # We want to test C++ and Swift only (in each direction)
-            test_flags: "--all-cross --filter cpp --filter swift"
+            test_flags: "--all-cross --filter ruby"
 
     runs-on: ${{ matrix.os }}
     steps:

diff --git a/cpp/src/Slice/Grammar.cpp b/cpp/src/Slice/Grammar.cpp
@@ -1791,7 +1791,7 @@ YYLTYPE yylloc = yyloc_default;
     auto contained = dynamic_pointer_cast<Contained>(yyvsp[0]);
     if (contained && !metadata->v.empty())
     {
-        contained->setMetadata(std::move(metadata->v));
+        contained->appendMetadata(std::move(metadata->v));
     }
 }
 #line 1798 "src/Slice/Grammar.cpp"
@@ -2670,7 +2670,7 @@ YYLTYPE yylloc = yyloc_default;
     auto contained = dynamic_pointer_cast<Contained>(yyvsp[-2]);
     if (contained && !metadata->v.empty())
     {
-        contained->setMetadata(std::move(metadata->v));
+        contained->appendMetadata(std::move(metadata->v));
     }
 }
 #line 2677 "src/Slice/Grammar.cpp"
@@ -3221,7 +3221,7 @@ YYLTYPE yylloc = yyloc_default;
     auto contained = dynamic_pointer_cast<Contained>(yyvsp[-2]);
     if (contained && !metadata->v.empty())
     {
-        contained->setMetadata(std::move(metadata->v));
+        contained->appendMetadata(std::move(metadata->v));
     }
 }
 #line 3228 "src/Slice/Grammar.cpp"
@@ -3435,7 +3435,7 @@ YYLTYPE yylloc = yyloc_default;
     auto enumerator = dynamic_pointer_cast<Enumerator>(yyvsp[-2]);
     if (enumerator && !metadata->v.empty())
     {
-        enumerator->setMetadata(std::move(metadata->v));
+        enumerator->appendMetadata(std::move(metadata->v));
     }
     auto enumeratorList = dynamic_pointer_cast<EnumeratorListTok>(yyvsp[0]);
     enumeratorList->v.push_front(enumerator);
@@ -3451,7 +3451,7 @@ YYLTYPE yylloc = yyloc_default;
     auto enumerator = dynamic_pointer_cast<Enumerator>(yyvsp[0]);
     if (enumerator && !metadata->v.empty())
     {
-        enumerator->setMetadata(std::move(metadata->v));
+        enumerator->appendMetadata(std::move(metadata->v));
     }
     auto enumeratorList = make_shared<EnumeratorListTok>();
     enumeratorList->v.push_front(enumerator);
@@ -3599,7 +3599,7 @@ YYLTYPE yylloc = yyloc_default;
         auto metadata = dynamic_pointer_cast<MetadataListTok>(yyvsp[-1]);
         if (!metadata->v.empty())
         {
-            pd->setMetadata(std::move(metadata->v));
+            pd->appendMetadata(std::move(metadata->v));
         }
     }
 }
@@ -3619,7 +3619,7 @@ YYLTYPE yylloc = yyloc_default;
         auto metadata = dynamic_pointer_cast<MetadataListTok>(yyvsp[-1]);
         if (!metadata->v.empty())
         {
-            pd->setMetadata(std::move(metadata->v));
+            pd->appendMetadata(std::move(metadata->v));
         }
     }
 }

diff --git a/cpp/src/Slice/Grammar.y b/cpp/src/Slice/Grammar.y
@@ -234,7 +234,7 @@ definitions
     auto contained = dynamic_pointer_cast<Contained>($3);
     if (contained && !metadata->v.empty())
     {
-        contained->setMetadata(std::move(metadata->v));
+        contained->appendMetadata(std::move(metadata->v));
     }
 }
 | %empty
@@ -989,7 +989,7 @@ data_members
     auto contained = dynamic_pointer_cast<Contained>($2);
     if (contained && !metadata->v.empty())
     {
-        contained->setMetadata(std::move(metadata->v));
+        contained->appendMetadata(std::move(metadata->v));
     }
 }
 | error ';' data_members
@@ -1470,7 +1470,7 @@ operations
     auto contained = dynamic_pointer_cast<Contained>($2);
     if (contained && !metadata->v.empty())
     {
-        contained->setMetadata(std::move(metadata->v));
+        contained->appendMetadata(std::move(metadata->v));
     }
 }
 | error ';' operations
@@ -1648,7 +1648,7 @@ enumerator_list
     auto enumerator = dynamic_pointer_cast<Enumerator>($2);
     if (enumerator && !metadata->v.empty())
     {
-        enumerator->setMetadata(std::move(metadata->v));
+        enumerator->appendMetadata(std::move(metadata->v));
     }
     auto enumeratorList = dynamic_pointer_cast<EnumeratorListTok>($4);
     enumeratorList->v.push_front(enumerator);
@@ -1660,7 +1660,7 @@ enumerator_list
     auto enumerator = dynamic_pointer_cast<Enumerator>($2);
     if (enumerator && !metadata->v.empty())
     {
-        enumerator->setMetadata(std::move(metadata->v));
+        enumerator->appendMetadata(std::move(metadata->v));
     }
     auto enumeratorList = make_shared<EnumeratorListTok>();
     enumeratorList->v.push_front(enumerator);
@@ -1788,7 +1788,7 @@ parameters
         auto metadata = dynamic_pointer_cast<MetadataListTok>($2);
         if (!metadata->v.empty())
         {
-            pd->setMetadata(std::move(metadata->v));
+            pd->appendMetadata(std::move(metadata->v));
         }
     }
 }
@@ -1804,7 +1804,7 @@ parameters
         auto metadata = dynamic_pointer_cast<MetadataListTok>($4);
         if (!metadata->v.empty())
         {
-            pd->setMetadata(std::move(metadata->v));
+            pd->appendMetadata(std::move(metadata->v));
         }
     }
 }

diff --git a/cpp/src/Slice/MetadataValidation.cpp b/cpp/src/Slice/MetadataValidation.cpp
@@ -19,12 +19,13 @@ namespace
     public:
         MetadataVisitor(string_view language, map<string, MetadataInfo> knownMetadata);
 
+        // We don't visit `ClassDef` and `InterfaceDef` because their metadata is always stored on their corresponding
+        // `ClassDecl` and `InterfaceDecl` types. So just visiting the `Decl` types is sufficient to check everything.
+
         bool visitUnitStart(const UnitPtr&) final;
         bool visitModuleStart(const ModulePtr&) final;
         void visitClassDecl(const ClassDeclPtr&) final;
-        bool visitClassDefStart(const ClassDefPtr&) final;
         void visitInterfaceDecl(const InterfaceDeclPtr&) final;
-        bool visitInterfaceDefStart(const InterfaceDefPtr&) final;
         bool visitExceptionStart(const ExceptionPtr&) final;
         bool visitStructStart(const StructPtr&) final;
         void visitOperation(const OperationPtr&) final;
@@ -40,7 +41,7 @@ namespace
 
         string_view _language;
         map<string, MetadataInfo> _knownMetadata;
-        set<string> _seenDirectives;
+        map<string, MetadataPtr> _seenDirectives;
     };
 }
 
@@ -73,17 +74,15 @@ Slice::validateMetadata(const UnitPtr& p, string_view prefix, map<string, Metada
 
     // "amd"
     MetadataInfo amdInfo = {
-        {typeid(InterfaceDef), typeid(Operation)},
+        {typeid(InterfaceDecl), typeid(Operation)},
         MetadataArgumentKind::NoArguments,
     };
     knownMetadata.emplace("amd", std::move(amdInfo));
 
     // "deprecated"
     MetadataInfo deprecatedInfo = {
         {typeid(InterfaceDecl),
-         typeid(InterfaceDef),
          typeid(ClassDecl),
-         typeid(ClassDef),
          typeid(Operation),
          typeid(Exception),
          typeid(Struct),
@@ -95,26 +94,27 @@ Slice::validateMetadata(const UnitPtr& p, string_view prefix, map<string, Metada
          typeid(DataMember)},
         MetadataArgumentKind::OptionalTextArgument,
     };
+    knownMetadata.emplace("deprecate", std::move(deprecatedInfo)); // Kept as an alias for 'deprecated'.
     knownMetadata.emplace("deprecated", std::move(deprecatedInfo));
 
     // "format"
     MetadataInfo formatInfo = {
-        {typeid(InterfaceDef), typeid(Operation)},
+        {typeid(InterfaceDecl), typeid(Operation)},
         MetadataArgumentKind::SingleArgument,
         {{"compact", "sliced", "default"}},
     };
     knownMetadata.emplace("format", std::move(formatInfo));
 
     // "marshaled-result"
     MetadataInfo marshaledResultInfo = {
-        {typeid(InterfaceDef), typeid(Operation)},
+        {typeid(InterfaceDecl), typeid(Operation)},
         MetadataArgumentKind::NoArguments,
     };
     knownMetadata.emplace("marshaled-result", std::move(marshaledResultInfo));
 
     // "protected"
     MetadataInfo protectedInfo = {
-        {typeid(ClassDef), typeid(Slice::Exception), typeid(Struct), typeid(DataMember)},
+        {typeid(ClassDecl), typeid(Slice::Exception), typeid(Struct), typeid(DataMember)},
         MetadataArgumentKind::NoArguments,
     };
     knownMetadata.emplace("protected", std::move(protectedInfo));
@@ -171,26 +171,12 @@ MetadataVisitor::visitClassDecl(const ClassDeclPtr& p)
     p->setMetadata(validate(p->getMetadata(), p));
 }
 
-bool
-MetadataVisitor::visitClassDefStart(const ClassDefPtr& p)
-{
-    p->setMetadata(validate(p->getMetadata(), p));
-    return true;
-}
-
 void
 MetadataVisitor::visitInterfaceDecl(const InterfaceDeclPtr& p)
 {
     p->setMetadata(validate(p->getMetadata(), p));
 }
 
-bool
-MetadataVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
-{
-    p->setMetadata(validate(p->getMetadata(), p));
-    return true;
-}
-
 bool
 MetadataVisitor::visitExceptionStart(const ExceptionPtr& p)
 {
@@ -441,13 +427,34 @@ MetadataVisitor::isMetadataValid(const MetadataPtr& metadata, const SyntaxTreeBa
     // Fifth we check if this metadata is a duplicate, i.e. have we already seen this metadata in this context?
     if (info.mustBeUnique)
     {
-        // 'insert' only returns `true` if the value wasn't already present in the set.
-        bool wasInserted = _seenDirectives.insert(directive).second;
+        // 'emplace' only returns `true` if the value wasn't already present in the set.
+        bool wasInserted = _seenDirectives.emplace(directive, metadata).second;
         if (!wasInserted)
         {
-            auto msg = "ignoring duplicate metadata: '" + directive + "' has already been applied in this context";
-            p->unit()->warning(metadata->file(), metadata->line(), InvalidMetadata, msg);
-            isValid = false;
+            // We make a special exception to uniqueness for metadata on forward declarations, since there can be
+            // multiple forward declarations for the same entity, but they all share a single backing `MetadataList`.
+            // So for these types, even 'unique' metadata to appear multiple times, but we require them to be identical.
+            if (dynamic_pointer_cast<ClassDecl>(p) || dynamic_pointer_cast<InterfaceDecl>(p))
+            {
+                const MetadataPtr& previousMetadata = _seenDirectives[directive];
+                if (arguments != previousMetadata->arguments())
+                {
+                    ostringstream msg;
+                    msg << "ignoring duplicate metadata: '" << *metadata
+                        << "' does not match previously applied metadata of '" << *previousMetadata << "'";
+                    p->unit()->warning(metadata->file(), metadata->line(), InvalidMetadata, msg.str());
+                }
+
+                // Even in this special case, we want to remove the metadata. At worst it was invalid, and at best,
+                // it's an exact duplicate of some other metadata. Either way, there's no reason to keep it around.
+                isValid = false;
+            }
+            else
+            {
+                auto msg = "ignoring duplicate metadata: '" + directive + "' has already been applied in this context";
+                p->unit()->warning(metadata->file(), metadata->line(), InvalidMetadata, msg);
+                isValid = false;
+            }
         }
     }
 

diff --git a/cpp/src/Slice/MetadataValidation.h b/cpp/src/Slice/MetadataValidation.h
@@ -64,6 +64,9 @@ namespace Slice
         /// If this list is empty we don't perform any automatic checking of whether this metadata is validly applied.
         /// Usually, this is because determining validity isn't as straightforward as matching against a list,
         /// and requires a more complex approach, which is achieved through providing an `extraValidation` function.
+        ///
+        /// 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;
 
         /// Specifies how many, and what kinds of arguments, this metadata accepts.