Improve Bad-Identifier Handling for Modules, Structs, and Operations #4286

InsertCreativityHere · 2025-08-05T16:10:09Z

This PR follows #4223 and #4276, and continues improving the way the Slice parser handles keyword or missing identifiers.

It improves identifier handling for modules, structs, and operations. All that's left after this are classes, interfaces, and exceptions.

It also slightly tweaks how the parser checks for 'changed meaning' errors. Often the logic was double-counting 'redefinition' errors as 'changed meaning' errors, which isn't helpful.

InsertCreativityHere · 2025-08-05T16:10:40Z

cpp/src/Slice/Grammar.y

@@ -309,7 +309,7 @@ definition
 // ----------------------------------------------------------------------
 : module_def opt_semicolon
 {
-    assert($1 == nullptr || dynamic_pointer_cast<Module>($1));


We always create the element now, even if it has a 'bad' identifier.
So the returned element should never be nullptr.

InsertCreativityHere · 2025-08-05T16:11:07Z

cpp/src/Slice/Grammar.y

@@ -368,35 +368,22 @@ definition
 // ----------------------------------------------------------------------
 module_def
 // ----------------------------------------------------------------------
-: ICE_MODULE ICE_IDENTIFIER
+: ICE_MODULE definition_name
 {
    currentUnit->setSeenDefinition();

    auto ident = dynamic_pointer_cast<StringTok>($2);
    ContainerPtr cont = currentUnit->currentContainer();
    ModulePtr module = cont->createModule(ident->v, false);


Again, we always create the element, so there's no risk of module being nullptr.
This helps simplify the rest of the logic here.

InsertCreativityHere · 2025-08-05T16:12:55Z

cpp/src/Slice/Grammar.y

@@ -618,25 +579,10 @@ optional_type_id
 }
 ;

-// ----------------------------------------------------------------------


The new definition_name rule handles keywords and identifiers.
It replaces these little side-rules we used to have before.

InsertCreativityHere · 2025-08-05T16:13:18Z

cpp/src/Slice/Grammar.y

@@ -700,7 +634,7 @@ class_name
 // ----------------------------------------------------------------------
 class_id
 // ----------------------------------------------------------------------


ICE_IDENT_OPEN is just <identifier>(, same for definition_name_open, it just means 'something' followed by an opening parenthesis.

InsertCreativityHere · 2025-08-05T16:14:16Z

cpp/src/Slice/Grammar.y

-            currentUnit->pushContainer(op);
-        }
+        interface->checkHasChangedMeaning(name, op);
+        currentUnit->pushContainer(op);
        $$ = op;
    }
    else
    {
        $$ = nullptr;
    }
 }


No longer needed. definition_name handles the keyword case.

InsertCreativityHere · 2025-08-05T16:15:53Z

cpp/src/Slice/Parser.cpp

@@ -1642,7 +1653,6 @@ SequencePtr
 Slice::Container::createSequence(const string& name, const TypePtr& type, MetadataList metadata)
 {
    SequencePtr p = make_shared<Sequence>(shared_from_this(), name, type, std::move(metadata));
-    _contents.push_back(p);


I moved these to the bottoms of their respective createXXX methods for consistency of placement.
It doesn't meaningfully change anything.

InsertCreativityHere · 2025-08-05T16:16:54Z

cpp/src/Slice/Parser.cpp

+        // If the two identifiers resolve to different elements, but those elements are in the same container,
+        // then the 'real' error is that the user redefined an element, not that an element changed meaning.
+        // This error is reported elsewhere in the code, so nothing to do here.
+        if (it->second->container() == namedThing->container())
+        {
+            return;
+        }


This is the new addition to this logic.
You'll see that adding it reduced the number of unnecessary changed meaning errors in the errorDetection tests.

InsertCreativityHere · 2025-08-05T16:18:08Z

cpp/test/Slice/errorDetection/CaseInsensitive.err

 CaseInsensitive.ice:151: enumeration 'eN1' differs only in capitalization from enumeration 'en1'
-CaseInsensitive.ice:151: 'eN1' has changed meaning


The real error was that eN1 and en1 conflict with one another.
Not that eN1 changed meaning...

InsertCreativityHere · 2025-08-05T16:18:40Z

cpp/test/Slice/errorDetection/ChangedMeaning.err

 ChangedMeaning.ice:123: 'E' is not an exception
-ChangedMeaning.ice:123: 'E' has changed meaning


The real error is that E was used as an exception when it is not, not that it changed meaning.

InsertCreativityHere · 2025-08-05T16:21:44Z

cpp/test/Slice/errorDetection/IdentAsKeyword.err

 IdentAsKeyword.ice:60: interfaces can only be defined within a module
+IdentAsKeyword.ice:60: keyword 'module' cannot be used as a name


Here it's actually good to have 2 errors, because they're both true and unrelated.
We didn't see the 2nd error before, because the createInterface logic exited early with nullptr.
Now that it doesn't exit early, we report all the errors we see.

InsertCreativityHere · 2025-08-06T13:45:25Z

cpp/test/Slice/errorDetection/DerivedRedefinition.err

+DerivedRedefinition.ice:15: redefinition of operation 'op'
 DerivedRedefinition.ice:15: operation 'op' is already defined as an operation in a base interface


Maybe we could improve it, but getting 2 errors for line 15 is correct, because op is doubly-redefined.
One in the same interface and once in a parent interface.

InsertCreativityHere · 2025-08-06T13:45:56Z

cpp/src/Slice/Grammar.cpp

Generated code, probably not worth reading.

bernardnormier · 2025-08-06T15:27:27Z

cpp/src/Slice/Parser.cpp

-        bool differsOnlyInCase = matches.front()->name() != name;
-        ModulePtr module = dynamic_pointer_cast<Module>(p);
-        if (module)
+        bool hasValidIdentifier = true;


Should be something like hasConflictingIdentifier

Yes, that's a more clear name.
I updated them all as suggested.

bernardnormier · 2025-08-06T15:28:21Z

cpp/src/Slice/Parser.cpp

+                    unit()->error(os.str());
+                    hasValidIdentifier = false;


I would add a break after the (first) error.

bernardnormier · 2025-08-06T15:29:00Z

cpp/src/Slice/Parser.cpp

-    if (!checkIdentifier(name))
-    {
-        return nullptr;
+        checkIdentifier(name);


Add a comment - why do we check the identifier so late?

I updated the name of this function to reportIllegalSuffixOrUnderscore.

This is much more clear than checkIdentifier

It makes it clear that (going forward) we don't care about the return value.
After I finish updating classes, exceptions, and interfaces like I did in this PR, I'll remove the return value all together.

bernardnormier · 2025-08-06T15:30:56Z

cpp/src/Slice/Parser.cpp

-        unit()->error(os.str());
-        return nullptr;
-    }
+        bool hasValidIdentifier = !doesNameConflict(name, "operation", _contents);


Need to rename variable: it's not about valid identifier (as checked by checkIdentifier, that this function doesn't call as far as I can tell).
It's only about a name that is not empty and non-conflicting.

I renamed the variable to hasConflictingIdentifier per your other suggestion.

externl · 2025-08-06T16:15:14Z

cpp/src/Slice/Parser.cpp

-            return nullptr;
+            // If this module has a valid identifier and doesn't conflict with another definition,
+            // add it to the unit's contentMap so it can be looked up later.
+            unit()->addContent(q);


Should we be doing this after checkIdentifier like before?

The order really doesn't matter.
I updated the name of this function to reportIllegalSuffixOrUnderscore, which is much clearer than checkIdentifier.

Copilot

Pull Request Overview

This PR improves how the Slice parser handles keyword or missing identifiers for modules, structs, and operations, as part of a series of improvements to bad-identifier handling. The changes make error messages more consistent and fix issues with "changed meaning" error detection.

Standardizes error messages for keyword misuse to use "cannot be used as a name"
Refactors identifier validation logic to better handle edge cases and reduce duplicate error reporting
Improves "changed meaning" error detection to avoid double-counting redefinition errors

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
IllegalUseOfKeyword.err	Updates expected error messages to use standardized "cannot be used as a name" format
IdentAsKeyword.err	Updates expected error messages and removes duplicate "changed meaning" errors
DerivedRedefinition.err	Adds expected redefinition error that was previously missing
ChangedMeaning.ice	Updates comment to reflect corrected error detection logic
ChangedMeaning.err	Removes duplicate "changed meaning" error that was incorrectly reported
CaseInsensitive.err	Removes duplicate "changed meaning" errors for redefinition cases
Util.h	Renames function from `checkIdentifier` to `reportIllegalSuffixOrUnderscore`
SliceUtil.cpp	Updates function name to match header
Parser.h	Removes unused `nodeType` parameter from `createStruct`
Parser.cpp	Major refactoring of module, struct, and operation creation with improved error handling
Grammar.y	Simplifies grammar rules and consolidates keyword error handling

Improve bad-identifier handling for module, struct, operation.

f430ee4

InsertCreativityHere requested a review from Copilot August 5, 2025 16:10

This comment was marked as outdated.

Sign in to view

InsertCreativityHere commented Aug 5, 2025

View reviewed changes

InsertCreativityHere added 2 commits August 5, 2025 17:30

Fixed a bug where we didn't properly recover from 'global' modules.

7e5b369

Formatting...

f8a96c4

InsertCreativityHere commented Aug 6, 2025

View reviewed changes

cpp/src/Slice/Grammar.cpp Outdated

Copy link

Member Author

InsertCreativityHere Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated code, probably not worth reading.

externl reacted with eyes emoji

InsertCreativityHere requested review from externl and bernardnormier August 6, 2025 13:46

bernardnormier reviewed Aug 6, 2025

View reviewed changes

externl approved these changes Aug 6, 2025

View reviewed changes

Review fixes.

3f7d27e

InsertCreativityHere requested review from bernardnormier and Copilot August 6, 2025 19:32

Copilot AI reviewed Aug 6, 2025

View reviewed changes

bernardnormier approved these changes Aug 6, 2025

View reviewed changes

InsertCreativityHere merged commit 5484ea4 into zeroc-ice:main Aug 6, 2025
26 of 27 checks passed

InsertCreativityHere mentioned this pull request Aug 20, 2025

Improve Bad-Identifier Handling for DataMembers, Parameters, and Exceptions. #4332

Merged

		CaseInsensitive.ice:151: enumeration 'eN1' differs only in capitalization from enumeration 'en1'
		CaseInsensitive.ice:151: 'eN1' has changed meaning

		ChangedMeaning.ice:123: 'E' is not an exception
		ChangedMeaning.ice:123: 'E' has changed meaning

		IdentAsKeyword.ice:60: interfaces can only be defined within a module
		IdentAsKeyword.ice:60: keyword 'module' cannot be used as a name

		DerivedRedefinition.ice:15: redefinition of operation 'op'
		DerivedRedefinition.ice:15: operation 'op' is already defined as an operation in a base interface

Improve Bad-Identifier Handling for Modules, Structs, and Operations #4286

Improve Bad-Identifier Handling for Modules, Structs, and Operations #4286

Uh oh!

Conversation

InsertCreativityHere commented Aug 5, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!