[fidlc] Improved support for anonymous names
Since we have anonymous names which do not come from source files, e.g.
the name of the built-in uint32 type, we must handle absence of location
all the way down to how we report errors.
One benefit is the removal of the is_anonymous() method on Name:
callers should always assume that names could be anonymous, and this
should be hidden by the Name object model.
Change-Id: Ib47d23c93d893c3fefc6093d9162264ec492e3ef
diff --git a/zircon/system/host/fidl/include/fidl/error_reporter.h b/zircon/system/host/fidl/include/fidl/error_reporter.h
index bfac003..4e8b3a6 100644
--- a/zircon/system/host/fidl/include/fidl/error_reporter.h
+++ b/zircon/system/host/fidl/include/fidl/error_reporter.h
@@ -36,12 +36,18 @@
void ReportErrorWithSquiggle(const SourceLocation& location,
StringView message);
- void ReportError(const SourceLocation& location, StringView message);
+ void ReportError(const SourceLocation& location, StringView message) {
+ ReportError(&location, message);
+ }
+ void ReportError(const SourceLocation* maybe_location, StringView message);
void ReportError(const Token& token, StringView message);
void ReportError(StringView message);
void ReportWarningWithSquiggle(const SourceLocation& location,
StringView message);
- void ReportWarning(const SourceLocation& location, StringView message);
+ void ReportWarning(const SourceLocation& location, StringView message) {
+ ReportWarning(&location, message);
+ }
+ void ReportWarning(const SourceLocation* maybe_location, StringView message);
void PrintReports();
Counts Checkpoint() const { return Counts(this); }
const std::vector<std::string>& errors() const { return errors_; }
diff --git a/zircon/system/host/fidl/include/fidl/flat_ast.h b/zircon/system/host/fidl/include/fidl/flat_ast.h
index 2683fd6..fc2a987 100644
--- a/zircon/system/host/fidl/include/fidl/flat_ast.h
+++ b/zircon/system/host/fidl/include/fidl/flat_ast.h
@@ -65,13 +65,12 @@
Name(Name&&) = default;
Name& operator=(Name&&) = default;
- bool is_anonymous() const { return name_from_source_ == nullptr; }
const Library* library() const { return library_; }
- const SourceLocation& source_location() const {
- return *name_from_source_.get();
+ const SourceLocation* maybe_location() const {
+ return name_from_source_.get();
}
const StringView name_part() const {
- if (is_anonymous())
+ if (!name_from_source_)
return *anonymous_name_.get();
return name_from_source_->data();
}
@@ -892,7 +891,7 @@
const Name* name() const { return &name_; }
- virtual bool Create(const SourceLocation& location,
+ virtual bool Create(const SourceLocation* maybe_location,
const Type* arg_type,
const types::HandleSubtype* handle_subtype,
const Size* size,
@@ -900,12 +899,22 @@
std::unique_ptr<Type>* out_type) const = 0;
protected:
- bool MustBeParameterized(const SourceLocation& location) const { return Fail(location, "must be parametrized"); }
- bool MustHaveSize(const SourceLocation& location) const { return Fail(location, "must have size"); }
- bool CannotBeParameterized(const SourceLocation& location) const { return Fail(location, "cannot be parametrized"); }
- bool CannotHaveSize(const SourceLocation& location) const { return Fail(location, "cannot have size"); }
- bool CannotBeNullable(const SourceLocation& location) const { return Fail(location, "cannot be nullable"); }
- bool Fail(const SourceLocation& location, const std::string& content) const;
+ bool MustBeParameterized(const SourceLocation* maybe_location) const {
+ return Fail(maybe_location, "must be parametrized");
+ }
+ bool MustHaveSize(const SourceLocation* maybe_location) const {
+ return Fail(maybe_location, "must have size");
+ }
+ bool CannotBeParameterized(const SourceLocation* maybe_location) const {
+ return Fail(maybe_location, "cannot be parametrized");
+ }
+ bool CannotHaveSize(const SourceLocation* maybe_location) const {
+ return Fail(maybe_location, "cannot have size");
+ }
+ bool CannotBeNullable(const SourceLocation* maybe_location) const {
+ return Fail(maybe_location, "cannot be nullable");
+ }
+ bool Fail(const SourceLocation* maybe_location, const std::string& content) const;
Typespace* typespace_;
@@ -1090,12 +1099,12 @@
friend class TypeAliasTypeTemplate;
bool Fail(StringView message);
- bool Fail(const SourceLocation& location, StringView message);
+ bool Fail(const SourceLocation& location, StringView message) {
+ return Fail(&location, message);
+ }
+ bool Fail(const SourceLocation* maybe_location, StringView message);
bool Fail(const Name& name, StringView message) {
- if (name.is_anonymous()) {
- return Fail(message);
- }
- return Fail(name.source_location(), message);
+ return Fail(name.maybe_location(), message);
}
bool Fail(const Decl& decl, StringView message) { return Fail(decl.name, message); }
@@ -1104,8 +1113,13 @@
void ValidateAttributesConstraints(const Decl* decl,
const raw::AttributeList* attributes);
+ // TODO(FIDL-596): Rationalize the use of names. Here, a simple name is
+ // one that is not scoped, it is just text. An anonymous name is one that
+ // is guaranteed to be unique within the library, and a derived name is one
+ // that is library scoped but derived from the concatenated components using
+ // underscores as delimiters.
+ SourceLocation GeneratedSimpleName(const std::string& name);
Name NextAnonymousName();
- Name GeneratedName(const std::string& name);
Name DerivedName(const std::vector<StringView>& components);
bool CompileCompoundIdentifier(const raw::CompoundIdentifier* compound_identifier,
diff --git a/zircon/system/host/fidl/include/fidl/json_generator.h b/zircon/system/host/fidl/include/fidl/json_generator.h
index ebbb20f..cd3287f 100644
--- a/zircon/system/host/fidl/include/fidl/json_generator.h
+++ b/zircon/system/host/fidl/include/fidl/json_generator.h
@@ -16,11 +16,16 @@
namespace fidl {
struct NameLocation {
- explicit NameLocation(const SourceLocation& source_location)
- : filename(source_location.source_file().filename()) {
- source_location.SourceLine(&position);
+ explicit NameLocation(const SourceLocation& location)
+ : filename(location.source_file().filename()) {
+ location.SourceLine(&position);
}
- explicit NameLocation(const flat::Name& name) : NameLocation(name.source_location()) {}
+
+ // TODO(FIDL-596): We are incorrectly assuming that the provided name is not
+ // anonymous, and relying on callers to avoid derefencing a nullptr
+ // location.
+ explicit NameLocation(const flat::Name& name) : NameLocation(*name.maybe_location()) {}
+
const std::string filename;
SourceFile::Position position;
};
diff --git a/zircon/system/host/fidl/include/fidl/token.h b/zircon/system/host/fidl/include/fidl/token.h
index 42142ec..bfb7d5c 100644
--- a/zircon/system/host/fidl/include/fidl/token.h
+++ b/zircon/system/host/fidl/include/fidl/token.h
@@ -69,7 +69,7 @@
}
StringView data() const { return location_.data(); }
- SourceLocation location() const { return location_; }
+ const SourceLocation& location() const { return location_; }
void set_previous_end(SourceLocation location) { previous_end_ = location; }
SourceLocation previous_end() const { return previous_end_; }
Kind kind() const { return kind_and_subkind_.kind(); }
diff --git a/zircon/system/host/fidl/lib/error_reporter.cpp b/zircon/system/host/fidl/lib/error_reporter.cpp
index 28d56d6..5fcc421 100644
--- a/zircon/system/host/fidl/lib/error_reporter.cpp
+++ b/zircon/system/host/fidl/lib/error_reporter.cpp
@@ -24,8 +24,16 @@
return squiggle;
}
-std::string Format(std::string qualifier, const SourceLocation& location,
+std::string Format(std::string qualifier, const SourceLocation* maybe_location,
StringView message, size_t squiggle_size = 0u) {
+ if (!maybe_location) {
+ std::string error = qualifier;
+ error.append(": ");
+ error.append(message);
+ return error;
+ }
+
+ const auto& location = *maybe_location;
SourceFile::Position position;
std::string surrounding_line = location.SourceLine(&position);
@@ -53,7 +61,6 @@
error.push_back('\n');
error.append(surrounding_line);
error.append(squiggle);
-
return error;
}
@@ -75,8 +82,8 @@
// filename:line:col: error: message
// sourceline
// ^
-void ErrorReporter::ReportError(const SourceLocation& location, StringView message) {
- auto error = Format("error", location, message);
+void ErrorReporter::ReportError(const SourceLocation* maybe_location, StringView message) {
+ auto error = Format("error", maybe_location, message);
AddError(std::move(error));
}
@@ -89,7 +96,7 @@
void ErrorReporter::ReportErrorWithSquiggle(
const SourceLocation& location, StringView message) {
auto token_data = location.data();
- auto error = Format("error", location, message, token_data.size());
+ auto error = Format("error", &location, message, token_data.size());
AddError(std::move(error));
}
@@ -118,8 +125,8 @@
// filename:line:col: warning: message
// sourceline
// ^
-void ErrorReporter::ReportWarning(const SourceLocation& location, StringView message) {
- auto warning = Format("warning", location, message);
+void ErrorReporter::ReportWarning(const SourceLocation* maybe_location, StringView message) {
+ auto warning = Format("warning", maybe_location, message);
AddWarning(std::move(warning));
}
@@ -132,7 +139,7 @@
void ErrorReporter::ReportWarningWithSquiggle(
const SourceLocation& location, StringView message) {
auto token_data = location.data();
- auto warning = Format("warning", location, message, token_data.size());
+ auto warning = Format("warning", &location, message, token_data.size());
AddWarning(std::move(warning));
}
diff --git a/zircon/system/host/fidl/lib/flat_ast.cpp b/zircon/system/host/fidl/lib/flat_ast.cpp
index 481dd7a..9ce526a9 100644
--- a/zircon/system/host/fidl/lib/flat_ast.cpp
+++ b/zircon/system/host/fidl/lib/flat_ast.cpp
@@ -400,15 +400,16 @@
// return it rather than create a new one. Lookup must be by name,
// arg_type, size, and nullability.
- auto const& location = name.source_location();
+ auto maybe_location = name.maybe_location();
auto type_template = LookupTemplate(name);
if (type_template == nullptr) {
std::string message("unknown type ");
message.append(name.name_part());
- error_reporter_->ReportError(location, message);
+ error_reporter_->ReportError(maybe_location, message);
return false;
}
- return type_template->Create(location, arg_type, handle_subtype, size, nullability, out_type);
+ return type_template->Create(maybe_location, arg_type, handle_subtype, size,
+ nullability, out_type);
}
@@ -429,11 +430,11 @@
return nullptr;
}
-bool TypeTemplate::Fail(const SourceLocation& location, const std::string& content) const {
+bool TypeTemplate::Fail(const SourceLocation* maybe_location, const std::string& content) const {
std::string message(NameName(name_, ".", "/"));
message.append(" ");
message.append(content);
- error_reporter_->ReportError(location, message);
+ error_reporter_->ReportError(maybe_location, message);
return false;
}
@@ -444,18 +445,18 @@
: TypeTemplate(Name(nullptr, name), typespace, error_reporter),
subtype_(subtype) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* maybe_arg_type,
const types::HandleSubtype* handle_subtype,
const Size* maybe_size,
types::Nullability nullability,
std::unique_ptr<Type>* out_type) const {
if (maybe_arg_type != nullptr)
- return CannotBeParameterized(location);
+ return CannotBeParameterized(maybe_location);
if (maybe_size != nullptr)
- return CannotHaveSize(location);
+ return CannotHaveSize(maybe_location);
if (nullability == types::Nullability::kNullable)
- return CannotBeNullable(location);
+ return CannotBeNullable(maybe_location);
*out_type = std::make_unique<PrimitiveType>(subtype_);
return true;
@@ -471,14 +472,14 @@
: TypeTemplate(Name(nullptr, "bytes"), typespace, error_reporter),
uint8_type_(types::PrimitiveSubtype::kUint8) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* maybe_arg_type,
const types::HandleSubtype* handle_subtype,
const Size* size,
types::Nullability nullability,
std::unique_ptr<Type>* out_type) const {
if (maybe_arg_type != nullptr)
- return CannotBeParameterized(location);
+ return CannotBeParameterized(maybe_location);
if (size == nullptr)
size = &max_size;
@@ -496,18 +497,18 @@
ArrayTypeTemplate(Typespace* typespace, ErrorReporter* error_reporter)
: TypeTemplate(Name(nullptr, "array"), typespace, error_reporter) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* arg_type,
const types::HandleSubtype* handle_subtype,
const Size* size,
types::Nullability nullability,
std::unique_ptr<Type>* out_type) const {
if (arg_type == nullptr)
- return MustBeParameterized(location);
+ return MustBeParameterized(maybe_location);
if (size == nullptr)
- return MustHaveSize(location);
+ return MustHaveSize(maybe_location);
if (nullability == types::Nullability::kNullable)
- return CannotBeNullable(location);
+ return CannotBeNullable(maybe_location);
*out_type = std::make_unique<ArrayType>(arg_type, size);
return true;
@@ -519,14 +520,14 @@
VectorTypeTemplate(Typespace* typespace, ErrorReporter* error_reporter)
: TypeTemplate(Name(nullptr, "vector"), typespace, error_reporter) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* arg_type,
const types::HandleSubtype* handle_subtype,
const Size* size,
types::Nullability nullability,
std::unique_ptr<Type>* out_type) const {
if (arg_type == nullptr)
- return MustBeParameterized(location);
+ return MustBeParameterized(maybe_location);
if (size == nullptr)
size = &max_size;
@@ -543,14 +544,14 @@
StringTypeTemplate(Typespace* typespace, ErrorReporter* error_reporter)
: TypeTemplate(Name(nullptr, "string"), typespace, error_reporter) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* arg_type,
const types::HandleSubtype* handle_subtype,
const Size* size,
types::Nullability nullability,
std::unique_ptr<Type>* out_type) const {
if (arg_type != nullptr)
- return CannotBeParameterized(location);
+ return CannotBeParameterized(maybe_location);
if (size == nullptr)
size = &max_size;
@@ -567,7 +568,7 @@
HandleTypeTemplate(Typespace* typespace, ErrorReporter* error_reporter)
: TypeTemplate(Name(nullptr, "handle"), typespace, error_reporter) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* maybe_arg_type,
const types::HandleSubtype* maybe_handle_subtype,
const Size* maybe_size,
@@ -576,7 +577,7 @@
assert(maybe_arg_type == nullptr);
if (maybe_size != nullptr)
- return CannotHaveSize(location);
+ return CannotHaveSize(maybe_location);
auto handle_subtype = types::HandleSubtype::kHandle;
if (maybe_handle_subtype != nullptr)
@@ -592,21 +593,21 @@
RequestTypeTemplate(Typespace* typespace, ErrorReporter* error_reporter)
: TypeTemplate(Name(nullptr, "request"), typespace, error_reporter) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* arg_type,
const types::HandleSubtype* handle_subtype,
const Size* maybe_size,
types::Nullability nullability,
std::unique_ptr<Type>* out_type) const {
if (arg_type == nullptr)
- return MustBeParameterized(location);
+ return MustBeParameterized(maybe_location);
if (arg_type->kind != Type::Kind::kIdentifier)
- return Fail(location, "must be an interface");
+ return Fail(maybe_location, "must be an interface");
auto interface_type = static_cast<const IdentifierType*>(arg_type);
if (interface_type->type_decl->kind != Decl::Kind::kInterface)
- return Fail(location, "must be an interface");
+ return Fail(maybe_location, "must be an interface");
if (maybe_size != nullptr)
- return CannotHaveSize(location);
+ return CannotHaveSize(maybe_location);
*out_type = std::make_unique<RequestHandleType>(interface_type, nullability);
return true;
@@ -625,7 +626,7 @@
: TypeTemplate(std::move(name), typespace, error_reporter),
library_(library), type_decl_(type_decl) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* arg_type,
const types::HandleSubtype* handle_subtype,
const Size* size,
@@ -654,7 +655,7 @@
case Decl::Kind::kTable:
if (nullability == types::Nullability::kNullable)
- return CannotBeNullable(location);
+ return CannotBeNullable(maybe_location);
break;
default:
@@ -684,7 +685,7 @@
: TypeTemplate(std::move(name), typespace, error_reporter),
library_(library), partial_type_ctor_(std::move(partial_type_ctor)) {}
- bool Create(const SourceLocation& location,
+ bool Create(const SourceLocation* maybe_location,
const Type* maybe_arg_type,
const types::HandleSubtype* no_handle_subtype,
const Size* maybe_size,
@@ -695,7 +696,7 @@
const Type* arg_type = nullptr;
if (partial_type_ctor_->maybe_arg_type_ctor) {
if (maybe_arg_type) {
- return Fail(location, "cannot parametrize twice");
+ return Fail(maybe_location, "cannot parametrize twice");
}
if (!partial_type_ctor_->maybe_arg_type_ctor->type) {
if (!library_->CompileTypeConstructor(
@@ -711,10 +712,10 @@
const Size* size = nullptr;
if (partial_type_ctor_->maybe_size) {
if (maybe_size) {
- return Fail(location, "cannot bound twice");
+ return Fail(maybe_location, "cannot bound twice");
}
if (!library_->ResolveConstant(partial_type_ctor_->maybe_size.get(), &library_->kSizeType))
- return Fail(location, "unable to parse size bound");
+ return Fail(maybe_location, "unable to parse size bound");
size = static_cast<const Size*>(&partial_type_ctor_->maybe_size->Value());
} else {
size = maybe_size;
@@ -723,7 +724,7 @@
types::Nullability nullability;
if (partial_type_ctor_->nullability == types::Nullability::kNullable) {
if (maybe_nullability == types::Nullability::kNullable) {
- return Fail(location, "cannot indicate nullability twice");
+ return Fail(maybe_location, "cannot indicate nullability twice");
}
nullability = types::Nullability::kNullable;
} else {
@@ -1000,7 +1001,7 @@
(error_primitive->subtype != types::PrimitiveSubtype::kInt32 &&
error_primitive->subtype != types::PrimitiveSubtype::kUint32)) {
error_reporter->ReportError(
- decl->name.source_location(),
+ decl->name.maybe_location(),
"invalid error type: must be int32, uint32 or an enum therof");
return false;
}
@@ -1053,7 +1054,7 @@
first = false;
out << t;
}
- error_reporter->ReportError(decl->name.source_location(), out.str());
+ error_reporter->ReportError(decl->name.maybe_location(), out.str());
return false;
}
}
@@ -1262,8 +1263,8 @@
return false;
}
-bool Library::Fail(const SourceLocation& location, StringView message) {
- error_reporter_->ReportError(location, message);
+bool Library::Fail(const SourceLocation* maybe_location, StringView message) {
+ error_reporter_->ReportError(maybe_location, message);
return false;
}
@@ -1291,22 +1292,22 @@
}
}
+SourceLocation Library::GeneratedSimpleName(const std::string& name) {
+ return generated_source_file_.AddLine(name);
+}
+
Name Library::NextAnonymousName() {
- // TODO(pascallouis): Improve anonymous name generation. We want to be
+ // TODO(FIDL-596): Improve anonymous name generation. We want to be
// specific about how these names are generated once they appear in the
// JSON IR, and are exposed to the backends.
std::ostringstream data;
data << "SomeLongAnonymousPrefix";
data << anon_counter_++;
- return GeneratedName(data.str());
-}
-
-Name Library::GeneratedName(const std::string& name) {
- return Name(this, generated_source_file_.AddLine(name));
+ return Name(this, GeneratedSimpleName(data.str()));
}
Name Library::DerivedName(const std::vector<StringView>& components) {
- return GeneratedName(StringJoin(components, "_"));
+ return Name(this, GeneratedSimpleName(StringJoin(components, "_")));
}
bool Library::CompileCompoundIdentifier(const raw::CompoundIdentifier* compound_identifier,
@@ -1581,11 +1582,11 @@
// error type.
Union::Member response_member{
IdentifierTypeForDecl(in_response, types::Nullability::kNonnullable),
- GeneratedName("response").source_location(),
+ GeneratedSimpleName("response"),
nullptr};
Union::Member error_member{
std::move(error_type_ctor),
- GeneratedName("err").source_location(),
+ GeneratedSimpleName("err"),
nullptr};
SourceLocation method_name = method->identifier->location();
Name result_name = DerivedName({interface_name.name_part(), method_name.data(), "Result"});
@@ -1607,7 +1608,7 @@
// result union.
std::vector<Struct::Member> response_members;
response_members.push_back(Struct::Member(IdentifierTypeForDecl(result_decl, types::Nullability::kNonnullable),
- GeneratedName("result").source_location(), nullptr, nullptr));
+ GeneratedSimpleName("result"), nullptr, nullptr));
struct_declarations_.push_back(std::make_unique<Struct>(nullptr, NextAnonymousName(), std::move(response_members), true));
*out_response = struct_declarations_.back().get();
if (!RegisterDecl(*out_response))
@@ -2843,8 +2844,9 @@
return Fail(name, message);
}
auto superinterface = static_cast<const Interface*>(decl);
- assert(!superinterface->name.is_anonymous());
- if (method_scope.interfaces.Insert(superinterface, superinterface->name.source_location()).ok()) {
+ auto maybe_location = superinterface->name.maybe_location();
+ assert(maybe_location);
+ if (method_scope.interfaces.Insert(superinterface, *maybe_location).ok()) {
if (!Visitor(superinterface, Visitor))
return false;
} else {
@@ -3117,7 +3119,6 @@
}
bool Library::CompileTypeConstructor(TypeConstructor* type_ctor, TypeShape* out_typeshape) {
- auto const& location = type_ctor->name.source_location();
const Type* maybe_arg_type = nullptr;
if (type_ctor->maybe_arg_type_ctor != nullptr) {
if (!CompileTypeConstructor(type_ctor->maybe_arg_type_ctor.get(), nullptr))
@@ -3127,7 +3128,7 @@
const Size* size = nullptr;
if (type_ctor->maybe_size != nullptr) {
if (!ResolveConstant(type_ctor->maybe_size.get(), &kSizeType))
- return Fail(location, "unable to parse size bound");
+ return Fail(type_ctor->name.maybe_location(), "unable to parse size bound");
size = static_cast<const Size*>(&type_ctor->maybe_size->Value());
}
if (!typespace_->Create(type_ctor->name, maybe_arg_type, type_ctor->maybe_handle_subtype.get(),
diff --git a/zircon/system/host/fidl/lib/lexer.cpp b/zircon/system/host/fidl/lib/lexer.cpp
index d34462b..0e9ea9e 100644
--- a/zircon/system/host/fidl/lib/lexer.cpp
+++ b/zircon/system/host/fidl/lib/lexer.cpp
@@ -182,7 +182,7 @@
std::string msg("invalid identifier '");
msg.append(identifier_data);
msg.append("'");
- error_reporter_->ReportError(location, msg);
+ error_reporter_->ReportError(&location, msg);
}
auto subkind = Token::Subkind::kNone;
auto lookup = keyword_table_.find(identifier_data);
@@ -368,7 +368,7 @@
std::string msg("invalid character '");
msg.append(location.data());
msg.append("'");
- error_reporter_->ReportError(location, msg);
+ error_reporter_->ReportError(&location, msg);
continue;
}
} // switch
@@ -410,7 +410,7 @@
std::string msg("invalid character '");
msg.append(location.data());
msg.append("'");
- error_reporter_->ReportError(location, msg);
+ error_reporter_->ReportError(&location, msg);
continue;
}
} // switch
