Skip to content
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.

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

Add a General Purpose 'MetadataValidator' to the Slice Parser #3067

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
299 changes: 299 additions & 0 deletions cpp/src/Slice/MetadataValidator.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,299 @@
// Copyright (c) ZeroC, Inc.

#include "MetadataValidator.h"

#include <cassert>

using namespace std;
using namespace Slice;

namespace
{
optional<string> validateAmd(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p)
{
if (!dynamic_pointer_cast<InterfaceDef>(p) && !dynamic_pointer_cast<Operation>(p))
{
return "the 'amd' metadata can only be applied to interfaces and operations";
}

if (!metadata->arguments().empty())
{
return "the 'amd' metadata does not take any arguments";
}

return nullopt;
}

optional<string> validateDeprecated(const SyntaxTreeBasePtr& p)
{
if (dynamic_pointer_cast<Unit>(p))
{
return "the 'deprecated' metadata cannot be specified as file metadata";
}

if (dynamic_pointer_cast<Builtin>(p))
{
return "the 'deprecated' metadata cannot be applied to builtin types";
}

if (dynamic_pointer_cast<Module>(p) || dynamic_pointer_cast<ParamDecl>(p))
{
const string kind = dynamic_pointer_cast<Contained>(p)->kindOf();
return "the 'deprecated' metadata cannot be applied to " + kind + "s";
}

return nullopt;
}

optional<string> validateFormat(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p)
{
if (dynamic_pointer_cast<Operation>(p))
InsertCreativityHere marked this conversation as resolved.
Show resolved Hide resolved
{
return "the 'format' metadata can only be applied to operations";
}

const string& arguments = metadata->arguments();
if (arguments != "compact" && arguments != "sliced" && arguments != "default")
{
return "invalid argument '" + arguments + "' supplied to 'format' metadata" +
"\nonly the following formats are valid: 'compact', 'sliced', 'default'";
}

return nullopt;
}

optional<string> validateMarshaledResult(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p)
{
if (!dynamic_pointer_cast<InterfaceDef>(p) && !dynamic_pointer_cast<Operation>(p))
{
return "the 'marshaled-result' metadata can only be applied to interfaces and operations";
}

if (!metadata->arguments().empty())
{
return "the 'marshaled-result' metadata does not take any arguments";
}

return nullopt;
}

optional<string> validateProtected(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p)
{
if (!dynamic_pointer_cast<DataMember>(p) && !dynamic_pointer_cast<ClassDef>(p) &&
!dynamic_pointer_cast<Struct>(p) && !dynamic_pointer_cast<Slice::Exception>(p))
{
return "the 'protected' metadata can only be applied to data members, classes, structs, and exceptions";
}

if (!metadata->arguments().empty())
{
return "the 'protected' metadata does not take any arguments";
}

return nullopt;
}

optional<string> validateSuppressWarning(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p)
{
if (!dynamic_pointer_cast<Unit>(p))
{
return "the 'suppress-warning' metadata can only be specified as file metadata. Ex: [[suppress-warning]]";
}

const string& arguments = metadata->arguments();
if (arguments != "" && arguments != "all" && arguments != "deprecated" && arguments != "invalid-metadata")
{
return "invalid category '" + arguments + "' supplied to 'suppress-warning' metadata" +
"\nonly the following categories are valid: 'all', 'deprecated', 'invalid-metadata'";
}

return nullopt;
}

// TODO: we should probably just remove this metadata. It's only checked by slice2java,
// and there's already a 'java:UserException' metadata that we also check... better to only keep that one.
optional<string> validateUserException(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p)
{
if (!dynamic_pointer_cast<Operation>(p))
{
return "the 'UserException' metadata can only be applied to operations";
}

if (!metadata->arguments().empty())
{
return "the 'UserException' metadata does not take any arguments";
}

return nullopt;
}
}

bool
Slice::MetadataValidator::visitUnitStart(const UnitPtr& p)
{
DefinitionContextPtr dc = p->findDefinitionContext(p->topLevelFile());
assert(dc);
dc->setMetadata(validate(dc->getMetadata(), p));
return true;
}

bool
Slice::MetadataValidator::visitModuleStart(const ModulePtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
return true;
}

void
Slice::MetadataValidator::visitClassDecl(const ClassDeclPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
}

bool
Slice::MetadataValidator::visitClassDefStart(const ClassDefPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
return true;
}

void
Slice::MetadataValidator::visitInterfaceDecl(const InterfaceDeclPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
}

bool
Slice::MetadataValidator::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
return true;
}

bool
Slice::MetadataValidator::visitExceptionStart(const ExceptionPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
return true;
}

bool
Slice::MetadataValidator::visitStructStart(const StructPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
return true;
}

void
Slice::MetadataValidator::visitOperation(const OperationPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
}

void
Slice::MetadataValidator::visitParamDecl(const ParamDeclPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
}

void
Slice::MetadataValidator::visitDataMember(const DataMemberPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
}

void
Slice::MetadataValidator::visitSequence(const SequencePtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
p->setTypeMetadata(validate(p->typeMetadata(), p->type()));
}

void
Slice::MetadataValidator::visitDictionary(const DictionaryPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
p->setKeyMetadata(validate(p->keyMetadata(), p->keyType()));
p->setValueMetadata(validate(p->valueMetadata(), p->valueType()));
}

void
Slice::MetadataValidator::visitEnum(const EnumPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
}

void
Slice::MetadataValidator::visitConst(const ConstPtr& p)
{
p->setMetadata(validate(p->getMetadata(), p));
p->setTypeMetadata(validate(p->typeMetadata(), p->type()));
}

MetadataList
Slice::MetadataValidator::validate(MetadataList metadata, const SyntaxTreeBasePtr& p)
{
// Iterate through the provided metadata and check each one for validity.
for (MetadataList::const_iterator i = metadata.begin(); i != metadata.end(); ++i)
{
const MetadataPtr& meta = *i++;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you're incrementing the iterator twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Fixed.
My fingers are too used to typing the ++i in the for loop...

if (auto message = validateMetadata(meta, p))
{
// If a warning message was returned from the validation function, it means the metadata was invalid.
// We remove it from the list and emit a warning to the user about it.
metadata.remove(meta);
Copy link
Member

Choose a reason for hiding this comment

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

This works because MetadaList is a std::list and not a std::vector.

Since you are using an iterator, you should remove this element using this iterator.

p->unit()->warning(meta->file(), meta->line(), InvalidMetadata, *message);
}
}

return metadata;
}

optional<string>
Slice::MetadataValidator::validateMetadata(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p)
{
// We only want to check metadata that that doesn't have a language prefix (ie. parser metadata).
const string& directive = metadata->directive();
if (directive.find(':') != string::npos)
{
return nullopt;
}

// Check each of the language-agnostic metadata directives that the parser is aware of.
if (directive == "amd")
Copy link
Member

Choose a reason for hiding this comment

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

This looks a lot like the logic we have for dispatching operations: we receive an operation name ("op") and we need to select the correct handler that unmarshals the parameters and more. It would make sense to use the same "dispatch" logic. See for example:

Ice::Object::dispatch(IncomingRequest& request, std::function<void(OutgoingResponse)> sendResponse)

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 operation dispatch logic is definitely more performant than my code,
but I also think it's much more complicated to follow, and there's more places for things to go wrong (we need to keep indices, the list, and it's length in sync)...

So, personally I'd prefer to keep the simpler logic I have here, since the Slice compiler's performance is a secondary concern.

{
return validateAmd(metadata, p);
}
else if (directive == "deprecate")
{
return validateDeprecated(p);
}
else if (directive == "deprecated")
{
return validateDeprecated(p);
}
else if (directive == "format")
{
return validateFormat(metadata, p);
}
else if (directive == "marshaled-result")
{
return validateMarshaledResult(metadata, p);
}
else if (directive == "protected")
{
return validateProtected(metadata, p);
}
else if (directive == "suppress-warning")
{
return validateSuppressWarning(metadata, p);
}
else if (directive == "UserException")
{
return validateUserException(metadata, p);
}
else
{
return "ignoring unknown metadata directive: '" + directive + "'";
}
}
45 changes: 45 additions & 0 deletions cpp/src/Slice/MetadataValidator.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) ZeroC, Inc.

#ifndef SLICE_UTIL_H
InsertCreativityHere marked this conversation as resolved.
Show resolved Hide resolved
#define SLICE_UTIL_H

#include "Parser.h"

#include <functional>

namespace Slice
{
class MetadataValidator final : public ParserVisitor
{
public:
/// This function performs the actual validation of metadata.
/// If the metadata pass validation, implementations should return `nullopt`.
/// If there is a problem with the metadata, implementations should return a message explaining the problem.
///
/// The implementation in this class validates parser metadata (metadata without a language prefix).
/// So subclasses which override this should:
/// 1) Perform their own language specific validation
/// 2) Call this implementation with `MetadataValidator::validateMetadata`
virtual std::optional<std::string> validateMetadata(const MetadataPtr& metadata, const SyntaxTreeBasePtr& p);

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;
void visitParamDecl(const ParamDeclPtr&) final;
void visitDataMember(const DataMemberPtr&) final;
void visitSequence(const SequencePtr&) final;
void visitDictionary(const DictionaryPtr&) final;
void visitEnum(const EnumPtr&) final;
void visitConst(const ConstPtr&) final;

private:
MetadataList validate(MetadataList metadata, const SyntaxTreeBasePtr& p);
};
}
#endif
24 changes: 24 additions & 0 deletions cpp/src/Slice/Parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4025,6 +4025,12 @@ Slice::Sequence::typeMetadata() const
return _typeMetadata;
}

void
Slice::Sequence::setTypeMetadata(MetadataList metadata)
{
_typeMetadata = metadata;
}

bool
Slice::Sequence::usesClasses() const
{
Expand Down Expand Up @@ -4103,6 +4109,18 @@ Slice::Dictionary::valueMetadata() const
return _valueMetadata;
}

void
Slice::Dictionary::setKeyMetadata(MetadataList metadata)
{
_keyMetadata = metadata;
}

void
Slice::Dictionary::setValueMetadata(MetadataList metadata)
{
_valueMetadata = metadata;
}

bool
Slice::Dictionary::usesClasses() const
{
Expand Down Expand Up @@ -4410,6 +4428,12 @@ Slice::Const::typeMetadata() const
return _typeMetadata;
}

void
Slice::Const::setTypeMetadata(MetadataList metadata)
{
_typeMetadata = metadata;
}

SyntaxTreeBasePtr
Slice::Const::valueType() const
{
Expand Down
Loading
Loading