Add Support for Nested Module Syntax #2012

InsertCreativityHere · 2024-04-02T15:00:18Z

This PR adds support for 'nested module syntax' to Ice-Slice.
So now instead of: module A { module B { ... } } you can write: module A::B { ... }.

Note that these syntaxes are literally equivalent. Both introduce two modules, in the same order.
This is purely a syntactical shorthand.

Metadata applied to this thing is now applied to the outer-most module:

[metadata-for-A]
module A::B

Does this seem correct to everyone?

It also updates our Slice files to use this new syntax where it was possible,
and where "having separate modules" didn't feel like it was important to the test.

For the reviewers:
Where are there other Slice files I should look through?

Yes, our Slice style is inconsistent and sometimes bad looking.
I tried not to touch it, lest this PR would take a day instead of an hour.

bernardnormier · 2024-04-02T15:45:39Z

Metadata applied to this thing is applied to the inner-most module:
[metadata-for-b]
module A::B
Does this seem correct to everyone?

No, this is wrong. We discussed this at length for IceRPC. See icerpc/icerpc-csharp#3187.

bernardnormier · 2024-04-02T15:49:05Z

Yes, our Slice style is inconsistent and sometimes bad looking.

I don't know what you're talking about. If some Slice files are poorly formatted, we should fix them.

InsertCreativityHere · 2024-04-02T16:02:11Z

I don't know what you're talking about. If some Slice files are poorly formatted, we should fix them.

Yes, I just don't want to do it in this particular PR.
I'll fix their formatting in a separate PR after this one.
There's little point to doing this when the Slice files are changing so much.

pepone · 2024-04-02T16:32:46Z

cpp/src/Slice/Grammar.y

@@ -385,6 +385,75 @@ module_def
        $$ = nullptr;
    }
 }
+| ICE_MODULE ICE_SCOPED_IDENTIFIER
+{
+    auto ident = dynamic_pointer_cast<StringTok>($2);


Can ICE_SCOPED_IDENTIFIER start with ::?

Yes it can, but if you type: ::Hello::There, we'll pick up 3 modules: ["", "Hello", "There"].
And "" is not a valid module identifier, so it fails.

pepone · 2024-04-02T16:36:52Z

cpp/src/Slice/Grammar.y

+        // Where `N` is the number of scope separators ("::").
+        size_t scopePos = 0;
+        auto ident = dynamic_pointer_cast<StringTok>($2);
+        while ((scopePos = ident->v.find("::", scopePos + 2)) != string::npos)


Why the +2, does ident always start with ::?

The intent was to skip over "::" in between module identifiers...
But you're right, this won't work for something like A::B...

InsertCreativityHere · 2024-04-02T19:35:00Z

After talking it over during lunch, I switch the metadata, so it now applies to the outermost module.
This may be counter-intuitive to users, but it's the most pragmatic option.

All module metadata falls into 2 categories:

It renames the module (Or prefixes it). These metadata only make sense for top-level modules (ie. the outer module).
metadata that is inherited by the module's contents. Since these are inherited, it makes no difference whether they're on an outer or inner module.

ReeceHumphreys

Looks good

ReeceHumphreys · 2024-04-02T20:17:13Z

cpp/src/Slice/SliceUtil.cpp

+    // Check that the identifier isn't empty.
+    if (name.empty())
+    {
+        currentUnit->error("missing identifier: illegal empty identifier");


Suggested change

currentUnit->error("missing identifier: illegal empty identifier");

currentUnit->error("missing identifier: invalid empty identifier");

This message was removed entirely.
It should now be impossible for this function to ever have an empty identifier.

ReeceHumphreys · 2024-04-02T20:18:27Z

cpp/test/Slice/errorDetection/NestedModule.err

+NestedModule.ice:9: illegal identifier `Holder': `Holder' suffix is reserved
+NestedModule.ice:11: illegal identifier `Holder': `Holder' suffix is reserved
+NestedModule.ice:15: illegal leading underscore in identifier `__Iceberg'
+NestedModule.ice:18: illegal identifier `Holder': `Holder' suffix is reserved
+NestedModule.ice:20: illegal identifier `Holder': `Holder' suffix is reserved
+NestedModule.ice:24: illegal identifier `APtr': `Ptr' suffix is reserved
+NestedModule.ice:24: illegal identifier `BPrx': `Prx' suffix is reserved
+NestedModule.ice:24: illegal identifier `CHelper': `Helper' suffix is reserved
+NestedModule.ice:27: missing identifier: illegal empty identifier
+NestedModule.ice:27: illegal identifier `Helper': `Helper' suffix is reserved


This will have to be updated accordingly if you implement my above suggestion.

I don't find "illegal empty identifier" as a good way to say "a module name must start with a letter".

The message became illegal identifier: module identifiers cannot start with '::' prefix

bernardnormier · 2024-04-02T20:51:57Z

cpp/src/Slice/SliceUtil.cpp

@@ -483,7 +483,15 @@ Slice::checkIdentifier(const string& id)
        name = id;
    }

+    // Check that the identifier isn't empty.
+    if (name.empty())


Is it possible to reach this code?

If yes, please add a test.
If not, an assert would be more appropriate.

After some research, it should be impossible to hit.
So, I switched it to an assert.

I hit this by accident when I was using it to check modules, and saw that passing in "" gets a warning of "illegal trailing underscore"...... which is obviously not true (It's a fluke of how we check for trailing underscores).

externl

LGTM

cpp/test/Slice/errorDetection/NestedModule.ice

Co-authored-by: Joe George <[email protected]>

…eroc-ice/ice#2012)

InsertCreativityHere and others added 5 commits April 2, 2024 10:25

Add support for nested module syntax.

c4a6969

Regenerated Slice compiler.

9684e9f

Added warning ignore.

0265404

Added a test.

d76a022

Updated Slice files to use nested module syntax.

1cc31a0

InsertCreativityHere requested review from pepone, externl, bernardnormier and ReeceHumphreys April 2, 2024 15:00

Fixed error line numbers.

ce16efd

pepone reviewed Apr 2, 2024

View reviewed changes

InsertCreativityHere added 2 commits April 2, 2024 15:00

Put metadata on the outer-most module.

a6ebebc

Review suggestions.

a4c8da2

InsertCreativityHere and others added 2 commits April 2, 2024 15:41

Regenerated the grammar.

ff060b4

Updated the tests.

15a9738

ReeceHumphreys approved these changes Apr 2, 2024

View reviewed changes

bernardnormier approved these changes Apr 2, 2024

View reviewed changes

InsertCreativityHere and others added 3 commits April 2, 2024 17:24

Issue specific error for '::' leading module identifiers.

4cac1b9

Regenerated compiler again.

3dd4bbb

Updated test error message.

52743a9

InsertCreativityHere requested a review from pepone April 2, 2024 21:47

pepone approved these changes Apr 3, 2024

View reviewed changes

externl approved these changes Apr 3, 2024

View reviewed changes

cpp/test/Slice/errorDetection/NestedModule.ice Outdated Show resolved Hide resolved

Update cpp/test/Slice/errorDetection/NestedModule.ice

ed6fb65

Co-authored-by: Joe George <[email protected]>

InsertCreativityHere merged commit 84d76e8 into zeroc-ice:main Apr 3, 2024
23 checks passed

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

HEAD:(zeroc-ice/ice@84d76e80b3) Add Support for Nested Module Syntax (z…

9dd3418

…eroc-ice/ice#2012)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Nested Module Syntax #2012

Add Support for Nested Module Syntax #2012

InsertCreativityHere commented Apr 2, 2024 •

edited

Loading

bernardnormier commented Apr 2, 2024

bernardnormier commented Apr 2, 2024

InsertCreativityHere commented Apr 2, 2024 •

edited

Loading

pepone Apr 2, 2024

InsertCreativityHere Apr 2, 2024

pepone Apr 2, 2024

InsertCreativityHere Apr 2, 2024

InsertCreativityHere Apr 2, 2024

InsertCreativityHere commented Apr 2, 2024

ReeceHumphreys left a comment

ReeceHumphreys Apr 2, 2024

InsertCreativityHere Apr 2, 2024

ReeceHumphreys Apr 2, 2024

bernardnormier Apr 2, 2024

InsertCreativityHere Apr 2, 2024

bernardnormier Apr 2, 2024

InsertCreativityHere Apr 2, 2024

externl left a comment

	currentUnit->error("missing identifier: illegal empty identifier");
	currentUnit->error("missing identifier: invalid empty identifier");

Add Support for Nested Module Syntax #2012

Add Support for Nested Module Syntax #2012

Conversation

InsertCreativityHere commented Apr 2, 2024 • edited Loading

bernardnormier commented Apr 2, 2024

bernardnormier commented Apr 2, 2024

InsertCreativityHere commented Apr 2, 2024 • 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

InsertCreativityHere commented Apr 2, 2024

ReeceHumphreys left a 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

externl left a comment

Choose a reason for hiding this comment

InsertCreativityHere commented Apr 2, 2024 •

edited

Loading

InsertCreativityHere commented Apr 2, 2024 •

edited

Loading