Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 6, 2025
+1,027 −1,149
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f430ee4
Improve bad-identifier handling for module, struct, operation.
InsertCreativityHere Aug 5, 2025
7e5b369
Fixed a bug where we didn't properly recover from 'global' modules.
InsertCreativityHere Aug 5, 2025
f8a96c4
Formatting...
InsertCreativityHere Aug 5, 2025
3f7d27e
Review fixes.
InsertCreativityHere Aug 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,692 changes: 807 additions & 885 deletions cpp/src/Slice/Grammar.cpp
View file
Open in desktop

Large diffs are not rendered by default.

195 changes: 62 additions & 133 deletions cpp/src/Slice/Grammar.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ definition
// ----------------------------------------------------------------------
: module_def opt_semicolon
{
assert($1 == nullptr || dynamic_pointer_cast<Module>($1));
Copy link
Member Author

Choose a reason for hiding this comment

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

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

assert(dynamic_pointer_cast<Module>($1));
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The assertion now assumes $1 is never nullptr, but the createModule function can still return a valid Module object even when there are identifier errors. However, if createModule were to return nullptr for other reasons, this assertion would fail. Consider adding a null check or documenting why $1 is guaranteed to be non-null.

Suggested change
assert(dynamic_pointer_cast<Module>($1));
assert($1 == nullptr || dynamic_pointer_cast<Module>($1));

Copilot uses AI. Check for mistakes.

}
| class_decl ';'
{
Expand Down Expand Up @@ -341,7 +341,7 @@ definition
}
| struct_def opt_semicolon
{
assert($1 == nullptr || dynamic_pointer_cast<Struct>($1));
assert(dynamic_pointer_cast<Struct>($1));
}
| sequence_def ';'
{
Expand All @@ -368,43 +368,30 @@ 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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

if (module)
{
cont->checkHasChangedMeaning(ident->v, module);
currentUnit->pushContainer(module);
$$ = module;
}
else
{
$$ = nullptr;
}

cont->checkHasChangedMeaning(ident->v, module);
currentUnit->pushContainer(module);
$$ = module;
}
'{' definitions '}'
{
if ($3)
{
currentUnit->popContainer();
$$ = $3;
}
else
{
$$ = nullptr;
}
currentUnit->popContainer();
$$ = $3;
}
| ICE_MODULE ICE_SCOPED_IDENTIFIER
{
currentUnit->setSeenDefinition();

auto ident = dynamic_pointer_cast<StringTok>($2);

// Reject scoped identifiers starting with "::". This is generally indicates global scope, but is invalid here.
// Reject scoped identifiers starting with "::". This generally indicates global scope, but is invalid here.
size_t startPos = 0;
if (ident->v.find("::") == 0)
{
Expand All @@ -428,54 +415,35 @@ module_def
{
const auto currentModuleName = modules[i];
ModulePtr module = cont->createModule(currentModuleName, true);
if (module)
{
cont->checkHasChangedMeaning(currentModuleName, module);
currentUnit->pushContainer(module);
$$ = cont = module;
}
else
{
// If an error occurs while creating one of the modules, we have to stop. But, to eagerly report as many
// errors as possible, we still 'create' any remaining modules, which will run _some_ validation on them.
for (size_t j = (i + 1); j < modules.size(); j++)
{
cont->createModule(modules[j], true); // Dummy
}

// Then we roll back the chain, i.e. pop the successfully-created-modules off the container stack.
for (; i > 0; i--)
{
currentUnit->popContainer();
}
$$ = nullptr;
break;
}
cont->checkHasChangedMeaning(currentModuleName, module);
currentUnit->pushContainer(module);
$$ = cont = module;
}
}
'{' definitions '}'
{
if ($3)
{
// We need to pop '(N+1)' modules off the container stack, to navigate out of the nested module.
// Where `N` is the number of scope separators ("::").
size_t startPos = 0;
auto ident = dynamic_pointer_cast<StringTok>($2);
while ((startPos = ident->v.find("::", startPos)) != string::npos)
{
currentUnit->popContainer();
startPos += 2; // Skip the "::" separator.
}
// We need to pop '(N+1)' modules off the container stack, to navigate out of the nested module.
// Where `N` is the number of scope separators ("::").
size_t startPos = 0;
auto ident = dynamic_pointer_cast<StringTok>($2);

// Set the 'return value' to the outer-most module, before we pop it off the stack.
// Whichever module we return, is the one that metadata will be applied to.
$$ = currentUnit->currentContainer();
currentUnit->popContainer();
// Skip over any leading "::". This is invalid syntax of course, but the parser still needs to properly handle it.
if (ident->v.find("::") == 0)
{
startPos += 2; // Skip the leading "::".
}
else

while ((startPos = ident->v.find("::", startPos)) != string::npos)
{
$$ = nullptr;
currentUnit->popContainer();
startPos += 2; // Skip the "::" separator.
}

// Set the 'return value' to the outer-most module, before we pop it off the stack.
// Whichever module we return, is the one that metadata will be applied to.
$$ = currentUnit->currentContainer();
currentUnit->popContainer();
}
;

Expand Down Expand Up @@ -618,25 +586,10 @@ optional_type_id
}
;

// ----------------------------------------------------------------------
Copy link
Member Author

Choose a reason for hiding this comment

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

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

struct_id
// ----------------------------------------------------------------------
: ICE_STRUCT ICE_IDENTIFIER
{
$$ = $2;
}
| ICE_STRUCT keyword
{
auto ident = dynamic_pointer_cast<StringTok>($2);
currentUnit->error("keyword '" + ident->v + "' cannot be used as struct name");
$$ = $2; // Dummy
}
;

// ----------------------------------------------------------------------
struct_decl
// ----------------------------------------------------------------------
: struct_id
: ICE_STRUCT definition_name
{
currentUnit->error("structs cannot be forward declared");
$$ = nullptr; // Dummy
Expand All @@ -646,39 +599,27 @@ struct_decl
// ----------------------------------------------------------------------
struct_def
// ----------------------------------------------------------------------
: struct_id
: ICE_STRUCT definition_name
{
auto ident = dynamic_pointer_cast<StringTok>($1);
auto ident = dynamic_pointer_cast<StringTok>($2);
ContainerPtr cont = currentUnit->currentContainer();
StructPtr st = cont->createStruct(ident->v);
if (st)
{
cont->checkHasChangedMeaning(ident->v, st);
currentUnit->pushContainer(st);
}
else
{
st = cont->createStruct(Ice::generateUUID()); // Dummy
assert(st);
currentUnit->pushContainer(st);
}

cont->checkHasChangedMeaning(ident->v, st);
currentUnit->pushContainer(st);
$$ = st;
}
'{' data_members '}'
{
if ($2)
{
currentUnit->popContainer();
}
$$ = $2;
currentUnit->popContainer();

// Empty structures are not allowed
auto st = dynamic_pointer_cast<Struct>($$);
assert(st);
auto st = dynamic_pointer_cast<Struct>($3);
if (st->dataMembers().empty())
{
currentUnit->error("struct '" + st->name() + "' must have at least one member"); // $$ is a dummy
currentUnit->error("struct '" + st->name() + "' must have at least one member");
}
$$ = st;
}
;

Expand All @@ -700,7 +641,7 @@ class_name
// ----------------------------------------------------------------------
class_id
// ----------------------------------------------------------------------
Copy link
Member Author

Choose a reason for hiding this comment

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

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

: ICE_CLASS ICE_IDENT_OPEN integer_constant ')'
: ICE_CLASS definition_name_open integer_constant ')'
{
auto integer = dynamic_pointer_cast<IntegerTok>($3);
int32_t id = -1;
Expand Down Expand Up @@ -1003,7 +944,7 @@ idempotent_modifier
// ----------------------------------------------------------------------
operation_preamble
// ----------------------------------------------------------------------
: idempotent_modifier return_type ICE_IDENT_OPEN
: idempotent_modifier return_type definition_name_open
{
bool isIdempotent = dynamic_pointer_cast<BoolTok>($1)->v;
auto returnType = dynamic_pointer_cast<OptionalDefTok>($2);
Expand All @@ -1018,45 +959,15 @@ operation_preamble
returnType->tag,
isIdempotent ? Operation::Idempotent : Operation::Normal);

if (op)
{
interface->checkHasChangedMeaning(name, op);
currentUnit->pushContainer(op);
}
interface->checkHasChangedMeaning(name, op);
currentUnit->pushContainer(op);
$$ = op;
}
else
{
$$ = nullptr;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed. definition_name handles the keyword case.

| idempotent_modifier return_type ICE_KEYWORD_OPEN
{
bool isIdempotent = dynamic_pointer_cast<BoolTok>($1)->v;
auto returnType = dynamic_pointer_cast<OptionalDefTok>($2);
string name = dynamic_pointer_cast<StringTok>($3)->v;
auto interface = dynamic_pointer_cast<InterfaceDef>(currentUnit->currentContainer());
if (interface)
{
OperationPtr op = interface->createOperation(
name,
returnType->type,
returnType->isOptional,
returnType->tag,
isIdempotent ? Operation::Idempotent : Operation::Normal);

if (op)
{
currentUnit->pushContainer(op);
currentUnit->error("keyword '" + name + "' cannot be used as operation name");
}
$$ = op; // Dummy
}
else
{
$$ = nullptr;
}
}
;

// ----------------------------------------------------------------------
Expand Down Expand Up @@ -1803,6 +1714,24 @@ definition_name
}
;

// ----------------------------------------------------------------------
definition_name_open
// ----------------------------------------------------------------------
: ICE_IDENT_OPEN
{
// All good, this is a valid identifier.
$$ = $1;
}
| ICE_KEYWORD_OPEN
{
// If an un-escaped keyword was used as an identifier, we emit an error,
// but continue along, pretending like the user escaped the keyword.
auto ident = dynamic_pointer_cast<StringTok>($1);
currentUnit->error("keyword '" + ident->v + "' cannot be used as a name");
$$ = ident;
}
;

// ----------------------------------------------------------------------
keyword
// ----------------------------------------------------------------------
Expand Down
Loading
Loading