add -Winout-parameter to avoid inout parameter
This is somewhat confusing for clients because although the parameters
are in, they look like out parameters.
Bug: 168028537
Test: aidl_unittests
Change-Id: I6509c376a9142249e948760fd84dd35897d0136d
diff --git a/Android.bp b/Android.bp
index 79b6cca..3ccbc33 100644
--- a/Android.bp
+++ b/Android.bp
@@ -135,6 +135,7 @@
"ast_cpp_unittest.cpp",
"ast_java_unittest.cpp",
"code_writer_unittest.cpp",
+ "diagnostics_unittest.cpp",
"generate_cpp_unittest.cpp",
"io_delegate_unittest.cpp",
"options_unittest.cpp",
diff --git a/aidl.cpp b/aidl.cpp
index 5c9da7c..0b1b6e0 100644
--- a/aidl.cpp
+++ b/aidl.cpp
@@ -547,12 +547,13 @@
case DiagnosticSeverity::DISABLED:
return AidlErrorLog(AidlErrorLog::NO_OP, loc);
case DiagnosticSeverity::WARNING:
- return AidlErrorLog(AidlErrorLog::WARNING, loc);
+ return AidlErrorLog(AidlErrorLog::WARNING, loc, Suffix(id));
case DiagnosticSeverity::ERROR:
error_count_++;
- return AidlErrorLog(AidlErrorLog::ERROR, loc);
+ return AidlErrorLog(AidlErrorLog::ERROR, loc, Suffix(id));
}
}
+ std::string Suffix(DiagnosticID id) const { return "[-W" + kDiagnosticsNames.at(id) + "]"; }
size_t ErrorCount() const { return error_count_; }
private:
diff --git a/aidl_language.cpp b/aidl_language.cpp
index 3de3e06..e90bc29 100644
--- a/aidl_language.cpp
+++ b/aidl_language.cpp
@@ -1526,6 +1526,13 @@
AIDL_ERROR(arg) << "Argument name cannot begin with '_aidl'";
return false;
}
+
+ if (arg->GetDirection() == AidlArgument::INOUT_DIR) {
+ diag.Report(arg->GetLocation(), DiagnosticID::inout_parameter)
+ << arg->GetName()
+ << " is 'inout'. Avoid inout parameters. This is somewhat confusing for clients "
+ "because although the parameters are 'in', they look out 'out' parameters.";
+ }
}
auto it = method_names.find(m->GetName());
@@ -1577,6 +1584,15 @@
AidlImport::AidlImport(const AidlLocation& location, const std::string& needed_class)
: AidlNode(location), needed_class_(needed_class) {}
+bool AidlDocument::CheckValid(const AidlTypenames& typenames, DiagnosticsContext& diag) const {
+ for (const auto& t : defined_types_) {
+ if (!t->CheckValid(typenames, diag)) {
+ return false;
+ }
+ }
+ return true;
+}
+
// Resolves unresolved type name to fully qualified typename to import
// case #1: SimpleName --> import p.SimpleName
// case #2: Outer.Inner --> import p.Outer
diff --git a/aidl_language.h b/aidl_language.h
index c4a61fc..4f24da7 100644
--- a/aidl_language.h
+++ b/aidl_language.h
@@ -1081,6 +1081,7 @@
AidlDocument& operator=(const AidlDocument&) = delete;
AidlDocument& operator=(AidlDocument&&) = delete;
+ bool CheckValid(const AidlTypenames& typenames, DiagnosticsContext& diag) const;
std::optional<std::string> ResolveName(const std::string& unresolved_type) const;
const std::vector<std::unique_ptr<AidlImport>>& Imports() const { return imports_; }
const std::vector<std::unique_ptr<AidlDefinedType>>& DefinedTypes() const {
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp
index 2ee75df..36d82c9 100644
--- a/aidl_unittest.cpp
+++ b/aidl_unittest.cpp
@@ -4323,7 +4323,7 @@
" -Weverything -o out -h out p/Foo.aidl");
CaptureStderr();
EXPECT_EQ(0, aidl::compile_aidl(options, io_delegate_));
- EXPECT_EQ("WARNING: p/Foo.aidl:1.1-10: Interface names should start with I.\n",
+ EXPECT_EQ("WARNING: p/Foo.aidl:1.1-10: Interface names should start with I.[-Winterface-name]\n",
GetCapturedStderr());
}
@@ -4333,17 +4333,7 @@
" -Weverything -Werror -o out -h out p/Foo.aidl");
CaptureStderr();
EXPECT_EQ(1, aidl::compile_aidl(options, io_delegate_));
- EXPECT_EQ("ERROR: p/Foo.aidl:1.1-10: Interface names should start with I.\n",
- GetCapturedStderr());
-}
-
-TEST_P(AidlTest, ErrorEnumZero) {
- io_delegate_.SetFileContents("p/Foo.aidl", "enum Foo { FOO = 1 }");
- auto options = Options::From("aidl --lang " + Options::LanguageToString(GetLanguage()) +
- " -Weverything -Werror -o out -h out p/Foo.aidl");
- CaptureStderr();
- EXPECT_EQ(1, aidl::compile_aidl(options, io_delegate_));
- EXPECT_EQ("ERROR: p/Foo.aidl:1.11-15: The first enumerator 'FOO' should be 0, but it is 1.\n",
+ EXPECT_EQ("ERROR: p/Foo.aidl:1.1-10: Interface names should start with I.[-Winterface-name]\n",
GetCapturedStderr());
}
diff --git a/diagnostics.cpp b/diagnostics.cpp
index b205296..9cd39c2 100644
--- a/diagnostics.cpp
+++ b/diagnostics.cpp
@@ -24,5 +24,11 @@
#undef DIAG
};
+const std::map<DiagnosticID, std::string> kDiagnosticsNames = {
+#define DIAG(ENUM, NAME, ENABLED) {DiagnosticID::ENUM, NAME},
+#include "diagnostics.inc"
+#undef DIAG
+};
+
} // namespace aidl
} // namespace android
\ No newline at end of file
diff --git a/diagnostics.h b/diagnostics.h
index cb63642..0106550 100644
--- a/diagnostics.h
+++ b/diagnostics.h
@@ -33,7 +33,6 @@
enum class DiagnosticID {
#define DIAG(ENUM, NAME, ENABLED) ENUM,
-
#include "diagnostics.inc"
#undef DIAG
};
@@ -52,6 +51,11 @@
};
extern const std::map<std::string, DiagnosticOption> kAllDiagnostics;
+extern const std::map<DiagnosticID, std::string> kDiagnosticsNames;
+
+inline std::ostream& operator<<(std::ostream& os, DiagnosticID id) {
+ return os << kDiagnosticsNames.at(id);
+}
} // namespace aidl
} // namespace android
\ No newline at end of file
diff --git a/diagnostics.inc b/diagnostics.inc
index 6cf9375..0d840bd 100644
--- a/diagnostics.inc
+++ b/diagnostics.inc
@@ -1,3 +1,4 @@
// DIAG(enum, name, enable-by-default)
-DIAG(interface_name, "interface-name", false)
-DIAG(enum_zero, "enum-zero", false)
\ No newline at end of file
+DIAG(enum_zero, "enum-zero", false)
+DIAG(inout_parameter, "inout-parameter", false)
+DIAG(interface_name, "interface-name", false)
\ No newline at end of file
diff --git a/diagnostics_unittest.cpp b/diagnostics_unittest.cpp
new file mode 100644
index 0000000..29eaf4b
--- /dev/null
+++ b/diagnostics_unittest.cpp
@@ -0,0 +1,79 @@
+/*
+ * Copyright (C) 2020, 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.
+ */
+#include "diagnostics.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <string>
+#include <vector>
+
+#include "aidl.h"
+#include "parser.h"
+#include "tests/fake_io_delegate.h"
+
+using android::aidl::AidlError;
+using android::aidl::AidlTypenames;
+using android::aidl::DiagnosticID;
+using android::aidl::DiagnosticsContext;
+using android::aidl::kDiagnosticsNames;
+using android::aidl::Options;
+using android::aidl::internals::load_and_validate_aidl;
+using android::aidl::test::FakeIoDelegate;
+using testing::internal::CaptureStderr;
+using testing::internal::GetCapturedStderr;
+
+struct DiagnosticsTest : testing::Test {
+ void ParseFiles(std::vector<std::pair<std::string, std::string>>&& files) {
+ ASSERT_TRUE(files.size() > 0);
+ const std::string main = files.begin()->first;
+ for (const auto& [file, contents] : files) {
+ io.SetFileContents(file, contents);
+ }
+ // emit diagnostics as warnings.
+ // "java" has no specific meaning here because we're testing CheckValid()
+ const Options options = Options::From("aidl -I . --lang java -o out -Weverything " + main);
+ CaptureStderr();
+ EXPECT_EQ(AidlError::OK, load_and_validate_aidl(main, options, io, &typenames, nullptr));
+ const std::string err = GetCapturedStderr();
+ if (expect_diagnostics.empty()) {
+ EXPECT_EQ("", err);
+ } else {
+ for (const auto id : expect_diagnostics) {
+ EXPECT_THAT(err, testing::HasSubstr("-W" + kDiagnosticsNames.at(id)));
+ }
+ }
+ }
+
+ AidlTypenames typenames;
+ FakeIoDelegate io;
+ std::vector<DiagnosticID> expect_diagnostics;
+};
+
+TEST_F(DiagnosticsTest, interface_name) {
+ expect_diagnostics = {DiagnosticID::interface_name};
+ ParseFiles({{"Foo.aidl", "interface Foo { }"}});
+}
+
+TEST_F(DiagnosticsTest, enum_zero) {
+ expect_diagnostics = {DiagnosticID::enum_zero};
+ ParseFiles({{"Enum.aidl", "enum Enum { A = 1 }"}});
+}
+
+TEST_F(DiagnosticsTest, inout_parameter) {
+ expect_diagnostics = {DiagnosticID::inout_parameter};
+ ParseFiles({{"IFoo.aidl", "interface IFoo { void foo(inout Bar bar); }"},
+ {"Bar.aidl", "parcelable Bar {}"}});
+}
diff --git a/logging.h b/logging.h
index 5f7685b..5b9c234 100644
--- a/logging.h
+++ b/logging.h
@@ -25,8 +25,8 @@
public:
enum Severity { NO_OP, WARNING, ERROR, FATAL };
- AidlErrorLog(Severity severity, const AidlLocation& location)
- : os_(&std::cerr), severity_(severity), location_(location) {
+ AidlErrorLog(Severity severity, const AidlLocation& location, const std::string& suffix = "")
+ : os_(&std::cerr), severity_(severity), location_(location), suffix_(suffix) {
sHadError |= severity_ >= ERROR;
if (severity_ != NO_OP) {
(*os_) << (severity_ == WARNING ? "WARNING: " : "ERROR: ");
@@ -42,7 +42,7 @@
AidlErrorLog(Severity severity, const std::unique_ptr<T>& node) : AidlErrorLog(severity, *node) {}
~AidlErrorLog() {
if (severity_ == NO_OP) return;
- (*os_) << std::endl;
+ (*os_) << suffix_ << std::endl;
if (severity_ == FATAL) abort();
if (location_.IsInternal()) {
(*os_) << "Logging an internal location should not happen. Offending location: " << location_
@@ -74,6 +74,7 @@
std::ostream* os_;
Severity severity_;
const AidlLocation location_;
+ const std::string suffix_;
static bool sHadError;
};