Refactor AST::addScopedType.
Makes NamedType receive full name in constructor.
Adds test that defined type names are unique within one scope.
Test: mma
Test: hidl_error_test
Change-Id: If218e1febc2af9f44c5908408f67e772efdda18e
diff --git a/AST.cpp b/AST.cpp
index cef3c4b..ed58a37 100644
--- a/AST.cpp
+++ b/AST.cpp
@@ -38,7 +38,7 @@
AST::AST(const Coordinator* coordinator, const std::string& path)
: mCoordinator(coordinator),
mPath(path),
- mRootScope("(root scope)", Location::startOf(path), nullptr /* parent */) {}
+ mRootScope("(root scope)", FQName(), Location::startOf(path), nullptr /* parent */) {}
Scope* AST::getRootScope() {
return &mRootScope;
@@ -84,6 +84,11 @@
status_t AST::postParse() {
status_t err;
+ // validateDefinedTypesUniqueNames is the first call,
+ // as other errors could appear because user meant
+ // different type than we assumed.
+ err = validateDefinedTypesUniqueNames();
+ if (err != OK) return err;
// checkAcyclicTypes is before resolveInheritance, as we
// need to have no cycle while getting parent class.
err = checkAcyclicTypes();
@@ -133,6 +138,19 @@
&visitedTypes);
}
+status_t AST::validateDefinedTypesUniqueNames() const {
+ std::unordered_set<const Type*> visited;
+ return mRootScope.recursivePass(
+ [&](const Type* type) -> status_t {
+ // We only want to validate type definition names in this place.
+ if (type->isScope()) {
+ return static_cast<const Scope*>(type)->validateUniqueNames();
+ }
+ return OK;
+ },
+ &visited);
+}
+
status_t AST::resolveInheritance() {
std::unordered_set<const Type*> visited;
return mRootScope.recursivePass(&Type::resolveInheritance, &visited);
@@ -278,13 +296,8 @@
mImportedASTs.insert(ast);
}
-bool AST::addScopedType(NamedType* type, std::string* errorMsg, Scope* scope) {
- bool success = scope->addType(type, errorMsg);
- if (!success) {
- return false;
- }
-
- std::vector<std::string> pathComponents{{type->localName()}};
+FQName AST::makeFullName(const char* localName, Scope* scope) const {
+ std::vector<std::string> pathComponents{{localName}};
for (; scope != &mRootScope; scope = scope->parent()) {
pathComponents.push_back(scope->localName());
}
@@ -292,12 +305,12 @@
std::reverse(pathComponents.begin(), pathComponents.end());
std::string path = StringHelper::JoinStrings(pathComponents, ".");
- FQName fqName(mPackage.package(), mPackage.version(), path);
- type->setFullName(fqName);
+ return FQName(mPackage.package(), mPackage.version(), path);
+}
- mDefinedTypesByFullName[fqName] = type;
-
- return true;
+void AST::addScopedType(NamedType* type, Scope* scope) {
+ scope->addType(type);
+ mDefinedTypesByFullName[type->fqName()] = type;
}
EnumValue* AST::lookupEnumValue(const FQName& fqName, std::string* errorMsg, Scope* scope) {
diff --git a/AST.h b/AST.h
index f77c0b7..0f27c85 100644
--- a/AST.h
+++ b/AST.h
@@ -54,8 +54,10 @@
bool isInterface() const;
bool containsInterfaces() const;
- // Returns true iff successful.
- bool addScopedType(NamedType* type, std::string* errorMsg, Scope* scope);
+ // Adds package, version and scope stack to local name
+ FQName makeFullName(const char* localName, Scope* scope) const;
+
+ void addScopedType(NamedType* type, Scope* scope);
const std::string &getFilename() const;
@@ -77,6 +79,10 @@
status_t constantExpressionRecursivePass(
const std::function<status_t(ConstantExpression*)>& func);
+ // Recursive tree pass that validates that all defined types
+ // have unique names in their scopes.
+ status_t validateDefinedTypesUniqueNames() const;
+
// Recursive tree pass that completes type declarations
// that depend on super types
status_t resolveInheritance();
diff --git a/CompoundType.cpp b/CompoundType.cpp
index 4211958..0254a6c 100644
--- a/CompoundType.cpp
+++ b/CompoundType.cpp
@@ -26,9 +26,9 @@
namespace android {
-CompoundType::CompoundType(Style style, const char* localName, const Location& location,
- Scope* parent)
- : Scope(localName, location, parent), mStyle(style), mFields(NULL) {}
+CompoundType::CompoundType(Style style, const char* localName, const FQName& fullName,
+ const Location& location, Scope* parent)
+ : Scope(localName, fullName, location, parent), mStyle(style), mFields(NULL) {}
CompoundType::Style CompoundType::style() const {
return mStyle;
diff --git a/CompoundType.h b/CompoundType.h
index a5dd7fc..0e61c28 100644
--- a/CompoundType.h
+++ b/CompoundType.h
@@ -31,7 +31,8 @@
STYLE_UNION,
};
- CompoundType(Style style, const char* localName, const Location& location, Scope* parent);
+ CompoundType(Style style, const char* localName, const FQName& fullName,
+ const Location& location, Scope* parent);
Style style() const;
diff --git a/EnumType.cpp b/EnumType.cpp
index edfbf47..1ed3849 100644
--- a/EnumType.cpp
+++ b/EnumType.cpp
@@ -27,9 +27,9 @@
namespace android {
-EnumType::EnumType(const char* localName, const Location& location,
+EnumType::EnumType(const char* localName, const FQName& fullName, const Location& location,
const Reference<Type>& storageType, Scope* parent)
- : Scope(localName, location, parent),
+ : Scope(localName, fullName, location, parent),
mValues(),
mStorageType(storageType),
mBitfieldType(new BitFieldType(parent)) {
diff --git a/EnumType.h b/EnumType.h
index c5cd662..51dd454 100644
--- a/EnumType.h
+++ b/EnumType.h
@@ -30,8 +30,8 @@
struct BitFieldType;
struct EnumType : public Scope {
- EnumType(const char* localName, const Location& location, const Reference<Type>& storageType,
- Scope* parent);
+ EnumType(const char* localName, const FQName& fullName, const Location& location,
+ const Reference<Type>& storageType, Scope* parent);
const Type *storageType() const;
const std::vector<EnumValue *> &values() const;
diff --git a/Interface.cpp b/Interface.cpp
index 1ae6638..0fcd081 100644
--- a/Interface.cpp
+++ b/Interface.cpp
@@ -69,9 +69,9 @@
LAST_HIDL_TRANSACTION = 0x0fffffff,
};
-Interface::Interface(const char* localName, const Location& location, Scope* parent,
- const Reference<Type>& superType)
- : Scope(localName, location, parent),
+Interface::Interface(const char* localName, const FQName& fullName, const Location& location,
+ Scope* parent, const Reference<Type>& superType)
+ : Scope(localName, fullName, location, parent),
mSuperType(superType),
mIsJavaCompatibleInProgress(false) {}
diff --git a/Interface.h b/Interface.h
index 02b0f4d..0a15bf9 100644
--- a/Interface.h
+++ b/Interface.h
@@ -29,8 +29,8 @@
struct InterfaceAndMethod;
struct Interface : public Scope {
- Interface(const char* localName, const Location& location, Scope* parent,
- const Reference<Type>& superType);
+ Interface(const char* localName, const FQName& fullName, const Location& location,
+ Scope* parent, const Reference<Type>& superType);
bool addMethod(Method *method);
bool addAllReservedMethods();
diff --git a/NamedType.cpp b/NamedType.cpp
index c0c8450..a57c8cd 100644
--- a/NamedType.cpp
+++ b/NamedType.cpp
@@ -18,17 +18,14 @@
namespace android {
-NamedType::NamedType(const char* localName, const Location& loc, Scope* parent)
- : Type(parent), mLocalName(localName), mLocation(loc) {}
+NamedType::NamedType(const char* localName, const FQName& fullName, const Location& loc,
+ Scope* parent)
+ : Type(parent), mLocalName(localName), mFullName(fullName), mLocation(loc) {}
bool NamedType::isNamedType() const {
return true;
}
-void NamedType::setFullName(const FQName &fullName) {
- mFullName = fullName;
-}
-
const FQName &NamedType::fqName() const {
return mFullName;
}
diff --git a/NamedType.h b/NamedType.h
index f666ba1..a0ef302 100644
--- a/NamedType.h
+++ b/NamedType.h
@@ -28,12 +28,10 @@
namespace android {
struct NamedType : public Type {
- NamedType(const char* localName, const Location& loc, Scope* parent);
+ NamedType(const char* localName, const FQName& fullName, const Location& loc, Scope* parent);
bool isNamedType() const override;
- void setFullName(const FQName &fullName);
-
const FQName &fqName() const;
std::string localName() const;
@@ -54,7 +52,7 @@
private:
const std::string mLocalName;
- FQName mFullName;
+ const FQName mFullName;
const Location mLocation;
DISALLOW_COPY_AND_ASSIGN(NamedType);
diff --git a/Scope.cpp b/Scope.cpp
index 64ccd2e..b9f90b9 100644
--- a/Scope.cpp
+++ b/Scope.cpp
@@ -22,32 +22,30 @@
#include <android-base/logging.h>
#include <hidl-util/Formatter.h>
+#include <iostream>
#include <vector>
namespace android {
-Scope::Scope(const char* localName, const Location& location, Scope* parent)
- : NamedType(localName, location, parent) {}
+Scope::Scope(const char* localName, const FQName& fullName, const Location& location, Scope* parent)
+ : NamedType(localName, fullName, location, parent) {}
Scope::~Scope(){}
-bool Scope::addType(NamedType *type, std::string *errorMsg) {
- const std::string &localName = type->localName();
-
- auto it = mTypeIndexByName.find(localName);
-
- if (it != mTypeIndexByName.end()) {
- *errorMsg = "A type named '";
- (*errorMsg) += localName;
- (*errorMsg) += "' is already declared in the current scope.";
-
- return false;
- }
-
+void Scope::addType(NamedType* type) {
size_t index = mTypes.size();
mTypes.push_back(type);
- mTypeIndexByName[localName] = index;
+ mTypeIndexByName[type->localName()] = index;
+}
- return true;
+status_t Scope::validateUniqueNames() const {
+ for (const auto* type : mTypes) {
+ if (mTypes[mTypeIndexByName.at(type->localName())] != type) {
+ std::cerr << "ERROR: A type named '" << type->localName()
+ << "' is already declared in the scope at " << type->location() << "\n";
+ return UNKNOWN_ERROR;
+ }
+ }
+ return OK;
}
NamedType *Scope::lookupType(const FQName &fqName) const {
@@ -218,8 +216,9 @@
////////////////////////////////////////
-RootScope::RootScope(const char* localName, const Location& location, Scope* parent)
- : Scope(localName, location, parent) {}
+RootScope::RootScope(const char* localName, const FQName& fullName, const Location& location,
+ Scope* parent)
+ : Scope(localName, fullName, location, parent) {}
RootScope::~RootScope() {}
std::string RootScope::typeName() const {
diff --git a/Scope.h b/Scope.h
index 848352c..25ffb7e 100644
--- a/Scope.h
+++ b/Scope.h
@@ -32,10 +32,12 @@
struct LocalIdentifier;
struct Scope : public NamedType {
- Scope(const char* localName, const Location& location, Scope* parent);
+ Scope(const char* localName, const FQName& fullName, const Location& location, Scope* parent);
virtual ~Scope();
- bool addType(NamedType *type, std::string *errorMsg);
+ void addType(NamedType* type);
+
+ status_t validateUniqueNames() const;
// lookup a type given an FQName.
// Assume fqName.package(), fqName.version(), fqName.valueName() is empty.
@@ -88,7 +90,8 @@
};
struct RootScope : public Scope {
- RootScope(const char* localName, const Location& location, Scope* parent);
+ RootScope(const char* localName, const FQName& fullName, const Location& location,
+ Scope* parent);
virtual ~RootScope();
virtual status_t validate() const override;
diff --git a/TypeDef.cpp b/TypeDef.cpp
index 6295746..1592778 100644
--- a/TypeDef.cpp
+++ b/TypeDef.cpp
@@ -21,9 +21,9 @@
namespace android {
-TypeDef::TypeDef(const char* localName, const Location& location, Scope* parent,
- const Reference<Type>& type)
- : NamedType(localName, location, parent), mReferencedType(type) {}
+TypeDef::TypeDef(const char* localName, const FQName& fullName, const Location& location,
+ Scope* parent, const Reference<Type>& type)
+ : NamedType(localName, fullName, location, parent), mReferencedType(type) {}
const ScalarType *TypeDef::resolveToScalarType() const {
CHECK(!"Should not be here");
diff --git a/TypeDef.h b/TypeDef.h
index 19af261..0e380fa 100644
--- a/TypeDef.h
+++ b/TypeDef.h
@@ -23,7 +23,7 @@
namespace android {
struct TypeDef : public NamedType {
- TypeDef(const char* localName, const Location& location, Scope* parent,
+ TypeDef(const char* localName, const FQName& fullName, const Location& location, Scope* parent,
const Reference<Type>& type);
const ScalarType *resolveToScalarType() const override;
diff --git a/hidl-gen_y.yy b/hidl-gen_y.yy
index a3ceb53..f1a19aa 100644
--- a/hidl-gen_y.yy
+++ b/hidl-gen_y.yy
@@ -675,15 +675,11 @@
}
Interface* iface = new Interface(
- $2, convertYYLoc(@2), *scope, *superType);
+ $2, ast->makeFullName($2, *scope), convertYYLoc(@2), *scope, *superType);
// Register interface immediately so it can be referenced inside
// definition.
- std::string errorMsg;
- if (!ast->addScopedType(iface, &errorMsg, *scope)) {
- std::cerr << "ERROR: " << errorMsg << " at " << @2 << "\n";
- YYERROR;
- }
+ ast->addScopedType(iface, *scope);
enterScope(ast, scope, iface);
}
@@ -706,14 +702,9 @@
// The reason we wrap the given type in a TypeDef is simply to suppress
// emitting any type definitions later on, since this is just an alias
// to a type defined elsewhere.
- TypeDef* typeDef = new TypeDef($3, convertYYLoc(@2), *scope, *$2);
-
- std::string errorMsg;
- if (!ast->addScopedType(typeDef, &errorMsg, *scope)) {
- std::cerr << "ERROR: " << errorMsg << " at " << @3 << "\n";
- YYERROR;
- }
-
+ TypeDef* typeDef = new TypeDef(
+ $3, ast->makeFullName($3, *scope), convertYYLoc(@2), *scope, *$2);
+ ast->addScopedType(typeDef, *scope);
$$ = typeDef;
}
;
@@ -854,7 +845,8 @@
named_struct_or_union_declaration
: struct_or_union_keyword valid_type_name
{
- CompoundType *container = new CompoundType($1, $2, convertYYLoc(@2), *scope);
+ CompoundType *container = new CompoundType(
+ $1, $2, ast->makeFullName($2, *scope), convertYYLoc(@2), *scope);
enterScope(ast, scope, container);
}
struct_or_union_body
@@ -864,13 +856,7 @@
container->setFields($4);
leaveScope(ast, scope);
-
- std::string errorMsg;
- if (!ast->addScopedType(container, &errorMsg, *scope)) {
- std::cerr << "ERROR: " << errorMsg << " at " << @2 << "\n";
- YYERROR;
- }
-
+ ast->addScopedType(container, *scope);
$$ = container;
}
;
@@ -950,21 +936,17 @@
named_enum_declaration
: ENUM valid_type_name enum_storage_type
{
- enterScope(ast, scope, new EnumType($2, convertYYLoc(@2), *$3, *scope));
+ EnumType* enumType = new EnumType(
+ $2, ast->makeFullName($2, *scope), convertYYLoc(@2), *$3, *scope);
+ enterScope(ast, scope, enumType);
}
enum_declaration_body
{
CHECK((*scope)->isEnum());
+ EnumType* enumType = static_cast<EnumType*>(*scope);
- EnumType *enumType = static_cast<EnumType *>(*scope);
leaveScope(ast, scope);
-
- std::string errorMsg;
- if (!ast->addScopedType(enumType, &errorMsg, *scope)) {
- std::cerr << "ERROR: " << errorMsg << " at " << @2 << "\n";
- YYERROR;
- }
-
+ ast->addScopedType(enumType, *scope);
$$ = enumType;
}
;
diff --git a/test/error_test/Android.bp b/test/error_test/Android.bp
index b7481ce..260f34c 100644
--- a/test/error_test/Android.bp
+++ b/test/error_test/Android.bp
@@ -45,6 +45,9 @@
"!($(location hidl-gen) -L check -r test:system/tools/hidl/test/error_test" +
" test.interface_unique_method_names_inheritance@1.0 >/dev/null 2>&1)" +
"&&" +
+ "!($(location hidl-gen) -L check -r test:system/tools/hidl/test/error_test" +
+ " test.scope_unique_type_names@1.0 >/dev/null 2>&1)" +
+ "&&" +
"echo 'int main(){return 0;}' > $(genDir)/TODO_b_37575883.cpp",
out: ["TODO_b_37575883.cpp"],
srcs: [
@@ -60,6 +63,7 @@
"no_two_interfaces/1.0/IFoo.hal",
"same_name_interface/1.0/IFoo.hal",
"same_package_name/1.0/IFoo.hal",
+ "scope_unique_type_names/1.0/IFoo.hal",
],
}
diff --git a/test/error_test/scope_unique_type_names/1.0/IFoo.hal b/test/error_test/scope_unique_type_names/1.0/IFoo.hal
new file mode 100644
index 0000000..815c8a2
--- /dev/null
+++ b/test/error_test/scope_unique_type_names/1.0/IFoo.hal
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package test.scope_unique_type_names@1.0;
+
+interface IFoo {
+ struct S {};
+ enum S : int32_t {}; // duplicates first struct name
+};