Reject Metadata on Nested-Module-Definitions #3768

InsertCreativityHere · 2025-03-24T18:47:41Z

This PR:

Adds bool usesNestedSyntax to Module, which stores whether a module used nested-module syntax.
Changes the MetadataValidator to reject all metadata that is applied to nested-module-definitions.
Adds a getStatus method to Unit which returns an exit code based on whether any errors have been emitted.

Up until now, libSlice only emitted errors during parsing. So the slice2xxx compilers only checked for errors from parsing. Now that we emit an error during the validation phase, I updated all the slice2xxx compilers to also check for errors at the end of everything: parsing, validation, and code-gen.

InsertCreativityHere · 2025-03-24T18:53:43Z

cpp/src/ice2slice/Main.cpp


+            status |= p->getStatus();


After we've finished parsing, validation, and code-gen, we want to check if any errors were reported.
We use |= because we don't want to overwrite an EXIT_FAILURE with an EXIT_SUCCESS.

We also don't want to return because we're looping over a list of files to compile. One of them failing shouldn't stop compilation of all the others.

InsertCreativityHere · 2025-03-24T18:54:42Z

ruby/src/IceRuby/Slice.cpp

@@ -135,6 +135,11 @@ IceRuby_loadSlice(int argc, VALUE* argv, VALUE /*self*/)
            //
            out << "# encoding: utf-8\n";
            generate(u, all, includePaths, out);


Copied from the lines above:

if (!icecpp->close() || parseStatus == EXIT_FAILURE) { u->destroy(); throw RubyException(rb_eArgError, "Slice parsing failed for `%s'", cmd.c_str()); }

InsertCreativityHere · 2025-03-24T18:55:13Z

python/modules/IcePy/Slice.cpp

@@ -143,6 +143,12 @@ IcePy_loadSlice(PyObject* /*self*/, PyObject* args)
        out.setUseTab(false);

        generate(u, all, includePaths, out);


Copied from the lines above:

if (!icecpp->close() || parseStatus == EXIT_FAILURE) { PyErr_Format(PyExc_RuntimeError, "Slice parsing failed for `%s'", cmd); u->destroy(); return nullptr; }

InsertCreativityHere · 2025-03-24T18:56:18Z

cpp/test/Slice/errorDetection/IllegalMetadata.err

I renamed the old IllegalMetadata test to FileMetadataMustComeFirst, since it was specifically focused on that.

bernardnormier · 2025-03-24T21:55:38Z

I find validating metadata as part of the code generation sub-optimal. The logic was not introduced by this PR but is made worse by this PR since this validation can now generate errors - not just warnings.

It would be nicer for "validate metadata" to be a separate step. And if this step fails (completes with an error), we don't generate any code. With this PR, I can get into the odd situation where my Slice compilation fails with an error and yet overwrites all my generated files.

InsertCreativityHere · 2025-03-25T00:22:29Z

Yes, @externl had a similar thought.

I started fixing this, by moving validation out of the gen functions, and adding a new check for errors before code-gen, which if hit, skips code-gen.

But I think I'll ask you both to ignore this issue for now, and put all those changes in an immediate follow-up PR.
Much of the validateMetadata implementations are inside the Gen classes. So fixing it moves around code which has nothing to do with the this PR's focus.

InsertCreativityHere · 2025-03-25T02:51:04Z

cpp/test/Slice/errorDetection/IllegalMetadata.ice

@@ -1,7 +1,11 @@
 // Copyright (c) ZeroC, Inc.


// Suggestion:

Suggested change

// Copyright (c) ZeroC, Inc.

{ "hello":"there" }

// Ice:

{ "hello":"there" }

// JSON:

{ "hello":"there" }

// Slice:

{ "hello":"there" }

// Suggestion:

Suggested change

// Copyright (c) ZeroC, Inc.

{ "Copyright (c)":"ZeroC, Inc." }

// Ice:

{ "Copyright (c)":"ZeroC, Inc." }

// JSON:

{ "Copyright (c)":"ZeroC, Inc." }

// Slice:

{ "Copyright (c)":"ZeroC, Inc." }

Suggestion:

Suggested change

// Copyright (c) ZeroC, Inc.

// Copyright (c) ZeroC, Inc. hello

JSON:

// Copyright (c) ZeroC, Inc. hello

Ice:

// Copyright (c) ZeroC, Inc. hello

Slice:

// Copyright (c) ZeroC, Inc. hello

bernardnormier · 2025-03-24T21:31:00Z

cpp/src/Slice/Parser.cpp

+int
+Slice::Unit::getStatus() const
+{
+    return (_errors ? EXIT_FAILURE : EXIT_SUCCESS);


This should be _errors > 0 ? ....

Is there some obscure difference here?
Or are you saying for clarity's sake?

Clarity sake.

bernardnormier · 2025-03-24T21:42:47Z

cpp/src/Slice/MetadataValidation.cpp

+    {
+        // Metadata cannot be applied to modules that used nested-module-syntax, since it's ambiguous in meaning.
+        // We issue an error, and clear the metadata, so that we're in a valid state for further validation.
+        p->unit()->error(p->file(), p->line(), "metadata cannot be applied to nested module definitions");


This is a confusing message. Here is a nested module definition:

module Outer { module Nested {} // a nested module definition }

I can certainly put metadata on Nested.

It would be clearer to emit:
"metadata cannot be applied to a module defined using the nested module syntax".

I was using the wording C++uses

/tmp/SAClaD0ZbY/main.cpp:13:1: error: a nested namespace definition cannot have attributes 13 | namespace [[deprecated]] Foo::Bar {} | ^~~~~~~~~

I'll switch it back to what I had originally,
but I don't like talking about the "syntax" here, and I think calling this thing a module defined using nested module syntax is verbose. We should come up with some better term for this thing.

Switched to
metadata cannot be applied to modules defined with nested module syntax
Same idea, but a little shorter at least.

should be "with the nested module syntax".

InsertCreativityHere added 4 commits March 24, 2025 11:45

Store whether a module used nested-module-syntax or not.

622661a

Added a test, and moved an existing test.

0a57850

The compilers need to handle validation errors now.

2442b54

Fix typos.

5623d86

InsertCreativityHere requested review from externl and bernardnormier March 24, 2025 18:47

Clang-format of course.

48d12d8

InsertCreativityHere commented Mar 24, 2025

View reviewed changes

InsertCreativityHere commented Mar 25, 2025

View reviewed changes

bernardnormier reviewed Mar 25, 2025

View reviewed changes

externl approved these changes Mar 25, 2025

View reviewed changes

Switched to a longer error message.

57cda0e

InsertCreativityHere requested a review from bernardnormier March 25, 2025 15:28

New error messages.

4b365f1

bernardnormier approved these changes Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject Metadata on Nested-Module-Definitions #3768

Reject Metadata on Nested-Module-Definitions #3768

InsertCreativityHere commented Mar 24, 2025 •

edited

Loading

InsertCreativityHere Mar 24, 2025

InsertCreativityHere Mar 24, 2025

InsertCreativityHere Mar 24, 2025

InsertCreativityHere Mar 24, 2025

bernardnormier commented Mar 24, 2025

InsertCreativityHere commented Mar 25, 2025

InsertCreativityHere Mar 25, 2025

bernardnormier Mar 24, 2025

InsertCreativityHere Mar 25, 2025

externl Mar 26, 2025

bernardnormier Mar 24, 2025

InsertCreativityHere Mar 25, 2025

InsertCreativityHere Mar 25, 2025

bernardnormier Mar 25, 2025

		@@ -143,6 +143,12 @@ IcePy_loadSlice(PyObject* /self/, PyObject* args)
		out.setUseTab(false);

		generate(u, all, includePaths, out);

	// Copyright (c) ZeroC, Inc.
	{ "Copyright (c)":"ZeroC, Inc." }

	// Copyright (c) ZeroC, Inc.
	// Copyright (c) ZeroC, Inc. hello

Reject Metadata on Nested-Module-Definitions #3768

Are you sure you want to change the base?

Reject Metadata on Nested-Module-Definitions #3768

Conversation

InsertCreativityHere commented Mar 24, 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

bernardnormier commented Mar 24, 2025

InsertCreativityHere commented Mar 25, 2025

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 Mar 24, 2025 •

edited

Loading