Skip to content

Commit bdfd1e4

Browse files
szuendCommit Bot
szuend
authored and
Commit Bot
committed
[torque] More flexibel and uniform error reporting
This CL changes the existing TorqueError struct into a more general TorqueMessage by adding a "kind" enum. The contextual for lint errors is removed and replaced by a list of TorqueMessages. A MessageBuilder is introduced to help with the different combinations of present information and method of reporting. A lint error with custom SourcePosition can be reported like this: Lint("naming convention error").Position(<src_pos_var>); While a fatal error, with CurrentSourcePosition can be thrown like this: Error("something went horrible wrong").Throw(); This approach is both backwards compatible and should prove flexible enough to add more information to messages or add other message kinds. Bug: v8:7793 Change-Id: Ib04fa188e34b3e8e9a6526a086f80da8f690a6f5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1617245 Commit-Queue: Simon Zünd <szuend@chromium.org> Reviewed-by: Tobias Tebbi <tebbi@chromium.org> Reviewed-by: Sigurd Schneider <sigurds@chromium.org> Cr-Commit-Position: refs/heads/master@{#61696}
1 parent 7b38d42 commit bdfd1e4

13 files changed

Lines changed: 151 additions & 113 deletions

‎src/torque/contextual.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ class ContextualVariable {
6767

6868
private:
6969
V8_EXPORT_PRIVATE static VarType*& Top();
70+
71+
static bool HasScope() { return Top() != nullptr; }
72+
friend class MessageBuilder;
7073
};
7174

7275
// Usage: DECLARE_CONTEXTUAL_VARIABLE(VarName, VarType)

‎src/torque/ls/json-parser.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,13 +184,15 @@ JsonParserResult ParseJson(const std::string& input) {
184184
// Torque needs a CurrentSourceFile scope during parsing.
185185
// As JSON lives in memory only, a unknown file scope is created.
186186
SourceFileMap::Scope source_map_scope;
187+
TorqueMessages::Scope messages_scope;
187188
CurrentSourceFile::Scope unkown_file(SourceFileMap::AddSource("<json>"));
188189

189190
JsonParserResult result;
190191
try {
191192
result.value = (*JsonGrammar().Parse(input)).Cast<JsonValue>();
192-
} catch (TorqueError& error) {
193-
result.error = error;
193+
} catch (TorqueAbortCompilation&) {
194+
CHECK(!TorqueMessages::Get().empty());
195+
result.error = TorqueMessages::Get().front();
194196
}
195197
return result;
196198
}

‎src/torque/ls/json-parser.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ namespace ls {
1717

1818
struct JsonParserResult {
1919
JsonValue value;
20-
base::Optional<TorqueError> error;
20+
base::Optional<TorqueMessage> error;
2121
};
2222

2323
V8_EXPORT_PRIVATE JsonParserResult ParseJson(const std::string& input);

‎src/torque/ls/message-handler.cc‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -86,32 +86,21 @@ void ResetCompilationErrorDiagnostics(MessageWriter writer) {
8686
// 2) send one notification per entry (per file).
8787
class DiagnosticCollector {
8888
public:
89-
void AddTorqueError(const TorqueError& error) {
90-
SourceId id = error.position ? error.position->source : SourceId::Invalid();
89+
void AddTorqueMessage(const TorqueMessage& message) {
90+
SourceId id =
91+
message.position ? message.position->source : SourceId::Invalid();
9192
auto& notification = GetOrCreateNotificationForSource(id);
9293

9394
Diagnostic diagnostic = notification.params().add_diagnostics();
94-
diagnostic.set_severity(Diagnostic::kError);
95-
diagnostic.set_message(error.message);
95+
diagnostic.set_severity(ServerityFor(message.kind));
96+
diagnostic.set_message(message.message);
9697
diagnostic.set_source("Torque Compiler");
9798

98-
if (error.position) {
99-
PopulateRangeFromSourcePosition(diagnostic.range(), *error.position);
99+
if (message.position) {
100+
PopulateRangeFromSourcePosition(diagnostic.range(), *message.position);
100101
}
101102
}
102103

103-
void AddLintError(const LintError& error) {
104-
auto& notification =
105-
GetOrCreateNotificationForSource(error.position.source);
106-
107-
Diagnostic diagnostic = notification.params().add_diagnostics();
108-
diagnostic.set_severity(Diagnostic::kWarning);
109-
diagnostic.set_message(error.message);
110-
diagnostic.set_source("Torque Compiler");
111-
112-
PopulateRangeFromSourcePosition(diagnostic.range(), error.position);
113-
}
114-
115104
std::map<SourceId, PublishDiagnosticsNotification>& notifications() {
116105
return notifications_;
117106
}
@@ -139,16 +128,25 @@ class DiagnosticCollector {
139128
range.end().set_character(position.end.column);
140129
}
141130

131+
Diagnostic::DiagnosticSeverity ServerityFor(TorqueMessage::Kind kind) {
132+
switch (kind) {
133+
case TorqueMessage::Kind::kError:
134+
return Diagnostic::kError;
135+
case TorqueMessage::Kind::kLint:
136+
return Diagnostic::kWarning;
137+
}
138+
}
139+
142140
std::map<SourceId, PublishDiagnosticsNotification> notifications_;
143141
};
144142

145143
void SendCompilationDiagnostics(const TorqueCompilerResult& result,
146144
MessageWriter writer) {
147145
DiagnosticCollector collector;
148-
if (result.error) collector.AddTorqueError(*result.error);
149146

150-
for (const LintError& error : result.lint_errors) {
151-
collector.AddLintError(error);
147+
// TODO(szuend): Split up messages by SourceId and sort them by line number.
148+
for (const TorqueMessage& message : result.messages) {
149+
collector.AddTorqueMessage(message);
152150
}
153151

154152
for (auto& pair : collector.notifications()) {

‎src/torque/torque-compiler.cc‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ void ReadAndParseTorqueFile(const std::string& path) {
3939
}
4040

4141
if (!maybe_content) {
42-
ReportErrorWithoutPosition("Cannot open file path/uri: ", path);
42+
Error("Cannot open file path/uri: ", path).Throw();
4343
}
4444

4545
ParseTorque(*maybe_content);
@@ -102,20 +102,21 @@ TorqueCompilerResult CompileTorque(const std::string& source,
102102
SourceFileMap::Scope source_map_scope;
103103
CurrentSourceFile::Scope no_file_scope(SourceFileMap::AddSource("<torque>"));
104104
CurrentAst::Scope ast_scope;
105-
LintErrors::Scope lint_errors_scope;
105+
TorqueMessages::Scope messages_scope;
106106
LanguageServerData::Scope server_data_scope;
107107

108108
TorqueCompilerResult result;
109109
try {
110110
ParseTorque(source);
111111
CompileCurrentAst(options);
112-
} catch (TorqueError& error) {
113-
result.error = error;
112+
} catch (TorqueAbortCompilation&) {
113+
// Do nothing. The relevant TorqueMessage is part of the
114+
// TorqueMessages contextual.
114115
}
115116

116117
result.source_file_map = SourceFileMap::Get();
117118
result.language_server_data = std::move(LanguageServerData::Get());
118-
result.lint_errors = LintErrors::Get();
119+
result.messages = std::move(TorqueMessages::Get());
119120

120121
return result;
121122
}
@@ -125,20 +126,21 @@ TorqueCompilerResult CompileTorque(std::vector<std::string> files,
125126
SourceFileMap::Scope source_map_scope;
126127
CurrentSourceFile::Scope unknown_source_file_scope(SourceId::Invalid());
127128
CurrentAst::Scope ast_scope;
128-
LintErrors::Scope lint_errors_scope;
129+
TorqueMessages::Scope messages_scope;
129130
LanguageServerData::Scope server_data_scope;
130131

131132
TorqueCompilerResult result;
132133
try {
133134
for (const auto& path : files) ReadAndParseTorqueFile(path);
134135
CompileCurrentAst(options);
135-
} catch (TorqueError& error) {
136-
result.error = error;
136+
} catch (TorqueAbortCompilation&) {
137+
// Do nothing. The relevant TorqueMessage is part of the
138+
// TorqueMessages contextual.
137139
}
138140

139141
result.source_file_map = SourceFileMap::Get();
140142
result.language_server_data = std::move(LanguageServerData::Get());
141-
result.lint_errors = LintErrors::Get();
143+
result.messages = std::move(TorqueMessages::Get());
142144

143145
return result;
144146
}

‎src/torque/torque-compiler.h‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,8 @@ struct TorqueCompilerResult {
3535
// Set the corresponding options flag to enable.
3636
LanguageServerData language_server_data;
3737

38-
// Lint errors collected during compilation. These are
39-
// mainly naming convention violations.
40-
std::vector<LintError> lint_errors;
41-
42-
// If any error occurred during either parsing or compilation,
43-
// this field will be set.
44-
base::Optional<TorqueError> error;
38+
// Errors collected during compilation.
39+
std::vector<TorqueMessage> messages;
4540
};
4641

4742
V8_EXPORT_PRIVATE TorqueCompilerResult

‎src/torque/torque-parser.cc‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ void CheckNotDeferredStatement(Statement* statement) {
249249
CurrentSourcePosition::Scope source_position(statement->pos);
250250
if (BlockStatement* block = BlockStatement::DynamicCast(statement)) {
251251
if (block->deferred) {
252-
ReportLintError(
252+
Lint(
253253
"cannot use deferred with a statement block here, it will have no "
254254
"effect");
255255
}
@@ -657,14 +657,10 @@ class AnnotationSet {
657657
auto list = iter->NextAs<std::vector<Identifier*>>();
658658
for (const Identifier* i : list) {
659659
if (allowed.find(i->value) == allowed.end()) {
660-
std::stringstream s;
661-
s << "Annotation " << i->value << " is not allowed here";
662-
ReportLintError(s.str(), i->pos);
660+
Lint("Annotation ", i->value, " is not allowed here").Position(i->pos);
663661
}
664662
if (!set_.insert(i->value).second) {
665-
std::stringstream s;
666-
s << "Duplicate annotation " << i->value;
667-
ReportLintError(s.str(), i->pos);
663+
Lint("Duplicate annotation ", i->value).Position(i->pos);
668664
}
669665
}
670666
}

‎src/torque/torque.cc‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,15 @@ namespace v8 {
99
namespace internal {
1010
namespace torque {
1111

12+
std::string ErrorPrefixFor(TorqueMessage::Kind kind) {
13+
switch (kind) {
14+
case TorqueMessage::Kind::kError:
15+
return "Torque Error";
16+
case TorqueMessage::Kind::kLint:
17+
return "Lint error";
18+
}
19+
}
20+
1221
int WrappedMain(int argc, const char** argv) {
1322
std::string output_directory;
1423
std::vector<std::string> files;
@@ -35,19 +44,16 @@ int WrappedMain(int argc, const char** argv) {
3544
// resolve the file name. Needed to report errors and lint warnings.
3645
SourceFileMap::Scope source_file_map_scope(result.source_file_map);
3746

38-
if (result.error) {
39-
TorqueError& error = *result.error;
40-
if (error.position) std::cerr << PositionAsString(*error.position) << ": ";
41-
std::cerr << "Torque error: " << error.message << "\n";
42-
v8::base::OS::Abort();
43-
}
47+
for (const TorqueMessage& message : result.messages) {
48+
if (message.position) {
49+
std::cerr << *message.position << ": ";
50+
}
4451

45-
for (const LintError& error : result.lint_errors) {
46-
std::cerr << PositionAsString(error.position)
47-
<< ": Lint error: " << error.message << "\n";
52+
std::cerr << ErrorPrefixFor(message.kind) << ": " << message.message
53+
<< "\n";
4854
}
4955

50-
if (!result.lint_errors.empty()) v8::base::OS::Abort();
56+
if (!result.messages.empty()) v8::base::OS::Abort();
5157

5258
return 0;
5359
}

‎src/torque/types.cc‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -341,12 +341,12 @@ void ClassType::Finalize() const {
341341
if (const ClassType* super_class = ClassType::DynamicCast(parent())) {
342342
if (super_class->HasIndexedField()) flags_ |= ClassFlag::kHasIndexedField;
343343
if (!super_class->IsAbstract() && !HasSameInstanceTypeAsParent()) {
344-
ReportLintError(
344+
Lint(
345345
"Super class must either be abstract (annotate super class with "
346346
"@abstract) "
347347
"or this class must have the same instance type as the super class "
348-
"(annotate this class with @hasSameInstanceTypeAsParent).",
349-
this->decl_->name->pos);
348+
"(annotate this class with @hasSameInstanceTypeAsParent).")
349+
.Position(this->decl_->name->pos);
350350
}
351351
}
352352
}
@@ -359,11 +359,10 @@ void ClassType::Finalize() const {
359359
if (!field_type->IsSubtypeOf(TypeOracle::GetObjectType()) ||
360360
field_type->IsSubtypeOf(TypeOracle::GetSmiType()) ||
361361
field_type->IsSubtypeOf(TypeOracle::GetNumberType())) {
362-
std::stringstream s;
363-
s << "Generation of C++ class for Torque class " << name()
364-
<< " is not supported yet, because the type of field "
365-
<< f.name_and_type.name << " cannot be handled yet";
366-
ReportLintError(s.str(), f.pos);
362+
Lint("Generation of C++ class for Torque class ", name(),
363+
" is not supported yet, because the type of field ",
364+
f.name_and_type.name, " cannot be handled yet")
365+
.Position(f.pos);
367366
}
368367
}
369368
}

‎src/torque/utils.cc‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ namespace v8 {
1515
namespace internal {
1616
namespace torque {
1717

18-
DEFINE_CONTEXTUAL_VARIABLE(LintErrors)
18+
DEFINE_CONTEXTUAL_VARIABLE(TorqueMessages)
1919

2020
std::string StringLiteralUnquote(const std::string& s) {
2121
DCHECK(('"' == s.front() && '"' == s.back()) ||
@@ -124,23 +124,27 @@ std::string CurrentPositionAsString() {
124124
return PositionAsString(CurrentSourcePosition::Get());
125125
}
126126

127-
[[noreturn]] void ThrowTorqueError(const std::string& message,
128-
bool include_position) {
129-
TorqueError error(message);
130-
if (include_position) error.position = CurrentSourcePosition::Get();
131-
throw error;
127+
void NamingConventionError(const std::string& type, const std::string& name,
128+
const std::string& convention) {
129+
Lint(type, " \"", name, "\" does not follow \"", convention,
130+
"\" naming convention.");
132131
}
133132

134-
void ReportLintError(const std::string& error, SourcePosition pos) {
135-
LintErrors::Get().push_back({error, pos});
133+
MessageBuilder::MessageBuilder(const std::string& message,
134+
TorqueMessage::Kind kind) {
135+
base::Optional<SourcePosition> position;
136+
if (CurrentSourcePosition::HasScope()) {
137+
position = CurrentSourcePosition::Get();
138+
}
139+
message_ = TorqueMessage{message, position, kind};
136140
}
137141

138-
void NamingConventionError(const std::string& type, const std::string& name,
139-
const std::string& convention) {
140-
std::stringstream sstream;
141-
sstream << type << " \"" << name << "\" does not follow \"" << convention
142-
<< "\" naming convention.";
143-
ReportLintError(sstream.str());
142+
void MessageBuilder::Report() const {
143+
TorqueMessages::Get().push_back(message_);
144+
}
145+
146+
[[noreturn]] void MessageBuilder::Throw() const {
147+
throw TorqueAbortCompilation{};
144148
}
145149

146150
namespace {

0 commit comments

Comments
 (0)