Import fails if parsing fails.
Previously, hidl-gen doesn't distinguish between
failed parse due to parsing errors or due to the
file being missing. This means that any of a number
of times when a file was imported, it could fail
to parse, but the parsing of the file that referenced
it would continue on happily.
A case to hidl_error_test was also added which fails before
this patch and succeeds after.
Change-Id: Ie51a56eff6ef35c7ae5c513252a3a7e6581addb1
Fixes: 64773282
Test: hidl's run_all_host_tests.sh
diff --git a/AST.cpp b/AST.cpp
index 6d26349..17c8e26 100644
--- a/AST.cpp
+++ b/AST.cpp
@@ -342,15 +342,17 @@
addToImportedNamesGranular(fqName);
- AST *importAST;
-
// cases like android.hardware.foo@1.0::IFoo.Internal
// android.hardware.foo@1.0::Abc.Internal
// assume it is an interface, and try to import it.
const FQName interfaceName = fqName.getTopLevelType();
// Do not enforce restrictions on imports.
- importAST = mCoordinator->parse(interfaceName, &mImportedASTs, Coordinator::Enforce::NONE);
+ AST* importAST;
+ status_t err = mCoordinator->parseOptional(interfaceName, &importAST, &mImportedASTs,
+ Coordinator::Enforce::NONE);
+ if (err != OK) return false;
+ // importAST nullptr == file doesn't exist
if (importAST != nullptr) {
// cases like android.hardware.foo@1.0::IFoo.Internal
diff --git a/Coordinator.cpp b/Coordinator.cpp
index ad6bac3..d6e5b7c 100644
--- a/Coordinator.cpp
+++ b/Coordinator.cpp
@@ -30,7 +30,7 @@
#include "AST.h"
#include "Interface.h"
-extern android::status_t parseFile(android::AST *ast);
+extern android::status_t parseFile(android::AST* ast, FILE* file);
static bool existdir(const char *name) {
DIR *dir = opendir(name);
@@ -192,17 +192,34 @@
AST* Coordinator::parse(const FQName& fqName, std::set<AST*>* parsedASTs,
Enforce enforcement) const {
+ AST* ret;
+ status_t err = parseOptional(fqName, &ret, parsedASTs, enforcement);
+ if (err != OK) CHECK(ret == nullptr); // internal consistency
+
+ // only in a handful of places do we want to distinguish between
+ // a missing file and a bad AST. Everywhere else, we just want to
+ // throw an error if we expect an AST to be present but it is not.
+ return ret;
+}
+
+status_t Coordinator::parseOptional(const FQName& fqName, AST** ast, std::set<AST*>* parsedASTs,
+ Enforce enforcement) const {
CHECK(fqName.isFullyQualified());
auto it = mCache.find(fqName);
if (it != mCache.end()) {
- AST *ast = (*it).second;
+ *ast = (*it).second;
- if (ast != nullptr && parsedASTs != nullptr) {
- parsedASTs->insert(ast);
+ if (*ast != nullptr && parsedASTs != nullptr) {
+ parsedASTs->insert(*ast);
}
- return ast;
+ if (*ast == nullptr) {
+ // circular import OR that AST has errors in it
+ return UNKNOWN_ERROR;
+ }
+
+ return OK;
}
// Add this to the cache immediately, so we can discover circular imports.
@@ -214,7 +231,8 @@
// Any interface file implicitly imports its package's types.hal.
FQName typesName = fqName.getTypesForPackage();
// Do not enforce on imports. Do not add imports' imports to this AST.
- typesAST = parse(typesName, nullptr, Enforce::NONE);
+ status_t err = parseOptional(typesName, &typesAST, nullptr, Enforce::NONE);
+ if (err != OK) return err;
// fall through.
}
@@ -224,26 +242,35 @@
path.append(fqName.name());
path.append(".hal");
- AST* ast = new AST(this, &Hash::getHash(path));
+ *ast = new AST(this, &Hash::getHash(path));
if (typesAST != NULL) {
// If types.hal for this AST's package existed, make it's defined
// types available to the (about to be parsed) AST right away.
- ast->addImportedAST(typesAST);
+ (*ast)->addImportedAST(typesAST);
}
- if (parseFile(ast) != OK || ast->postParse() != OK) {
- delete ast;
- ast = nullptr;
+ FILE* file = fopen(path.c_str(), "rb");
- return nullptr;
+ if (file == nullptr) {
+ mCache.erase(fqName); // nullptr in cache is used to find circular imports
+ delete *ast;
+ *ast = nullptr;
+ return OK; // File does not exist, nullptr AST* == file doesn't exist.
}
onFileAccess(path, "r");
+ // parse file takes ownership of file
+ if (parseFile(*ast, file) != OK || (*ast)->postParse() != OK) {
+ delete *ast;
+ *ast = nullptr;
+ return UNKNOWN_ERROR;
+ }
+
status_t err = OK;
- if (ast->package().package() != fqName.package()
- || ast->package().version() != fqName.version()) {
+ if ((*ast)->package().package() != fqName.package() ||
+ (*ast)->package().version() != fqName.version()) {
fprintf(stderr,
"ERROR: File at '%s' does not match expected package and/or "
"version.\n",
@@ -251,16 +278,15 @@
err = UNKNOWN_ERROR;
} else {
- if (ast->isInterface()) {
+ if ((*ast)->isInterface()) {
if (fqName.name() == "types") {
fprintf(stderr,
"ERROR: File at '%s' declares an interface '%s' "
"instead of the expected types common to the package.\n",
- path.c_str(),
- ast->getInterface()->localName().c_str());
+ path.c_str(), (*ast)->getInterface()->localName().c_str());
err = UNKNOWN_ERROR;
- } else if (ast->getInterface()->localName() != fqName.name()) {
+ } else if ((*ast)->getInterface()->localName() != fqName.name()) {
fprintf(stderr,
"ERROR: File at '%s' does not declare interface type "
"'%s'.\n",
@@ -277,7 +303,7 @@
fqName.name().c_str());
err = UNKNOWN_ERROR;
- } else if (ast->containsInterfaces()) {
+ } else if ((*ast)->containsInterfaces()) {
fprintf(stderr,
"ERROR: types.hal file at '%s' declares at least one "
"interface type.\n",
@@ -288,28 +314,29 @@
}
if (err != OK) {
- delete ast;
- ast = nullptr;
-
- return nullptr;
+ delete *ast;
+ *ast = nullptr;
+ return err;
}
- if (parsedASTs != nullptr) { parsedASTs->insert(ast); }
+ if (parsedASTs != nullptr) {
+ parsedASTs->insert(*ast);
+ }
// put it into the cache now, so that enforceRestrictionsOnPackage can
// parse fqName.
- mCache[fqName] = ast;
+ mCache[fqName] = *ast;
// For each .hal file that hidl-gen parses, the whole package will be checked.
err = enforceRestrictionsOnPackage(fqName, enforcement);
if (err != OK) {
mCache[fqName] = nullptr;
- delete ast;
- ast = nullptr;
- return nullptr;
+ delete *ast;
+ *ast = nullptr;
+ return err;
}
- return ast;
+ return OK;
}
const Coordinator::PackageRoot &Coordinator::findPackageRoot(const FQName &fqName) const {
diff --git a/Coordinator.h b/Coordinator.h
index 9179d75..60a9a4f 100644
--- a/Coordinator.h
+++ b/Coordinator.h
@@ -86,6 +86,15 @@
AST* parse(const FQName& fqName, std::set<AST*>* parsedASTs = nullptr,
Enforce enforcement = Enforce::FULL) const;
+ // Same as parse, but it distinguishes between "missing file" and "could not parse AST"
+ // return OK, out *ast:
+ // 0xdeadbeef -> successfully parsed
+ // nullptr -> file not present
+ // return !OK
+ // could not parse AST and file exists
+ status_t parseOptional(const FQName& fqName, AST** ast, std::set<AST*>* parsedASTs = nullptr,
+ Enforce enforcement = Enforce::FULL) const;
+
// Given package-root paths of ["hardware/interfaces",
// "vendor/<something>/interfaces"], package roots of
// ["android.hardware", "vendor.<something>.hardware"], and a
diff --git a/hidl-gen_l.ll b/hidl-gen_l.ll
index ec1a3fc..4b3d7a0 100644
--- a/hidl-gen_l.ll
+++ b/hidl-gen_l.ll
@@ -181,13 +181,7 @@
#pragma clang diagnostic pop
-status_t parseFile(AST *ast) {
- FILE *file = fopen(ast->getFilename().c_str(), "rb");
-
- if (file == nullptr) {
- return -errno;
- }
-
+status_t parseFile(AST* ast, FILE* file) {
yyscan_t scanner;
yylex_init(&scanner);
diff --git a/test/error_test/references_broken_package/1.0/IBar.hal b/test/error_test/references_broken_package/1.0/IBar.hal
new file mode 100644
index 0000000..27dbdeb
--- /dev/null
+++ b/test/error_test/references_broken_package/1.0/IBar.hal
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2018 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.references_broken_package@1.0;
+
+import test.references_broken_package.bad_package@1.0::IFoo;
+
+interface IBar {
+};
diff --git a/test/error_test/references_broken_package/1.0/required_error b/test/error_test/references_broken_package/1.0/required_error
new file mode 100644
index 0000000..4301bae
--- /dev/null
+++ b/test/error_test/references_broken_package/1.0/required_error
@@ -0,0 +1 @@
+missing ; at
\ No newline at end of file
diff --git a/test/error_test/references_broken_package/bad_package/1.0/IFoo.hal b/test/error_test/references_broken_package/bad_package/1.0/IFoo.hal
new file mode 100644
index 0000000..14d4189
--- /dev/null
+++ b/test/error_test/references_broken_package/bad_package/1.0/IFoo.hal
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2018 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.references_broken_package.bad_package@1.0;
+
+interface IFoo {
+} // no semicolon -> error
diff --git a/test/error_test/references_broken_package/bad_package/1.0/types.hal b/test/error_test/references_broken_package/bad_package/1.0/types.hal
new file mode 100644
index 0000000..ea7ed6e
--- /dev/null
+++ b/test/error_test/references_broken_package/bad_package/1.0/types.hal
@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2018 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.references_broken_package.bad_package@1.0;
+
+struct IFoo {};
\ No newline at end of file