Skip to content

Commit 9e5ef9c

Browse files
aibaarsaibaars
authored
Merge pull request github#12216 from aibaars/diagnostics-2
Ruby: improve diagnostic messages
2 parents 7705d5f + 2c611d3 commit 9e5ef9c

File tree

4 files changed

+164
-76
lines changed

4 files changed

+164
-76
lines changed

‎ruby/extractor/src/diagnostics.rs‎

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ pub struct LogWriter {
8888
}
8989

9090
impl LogWriter {
91-
pub fn message(&self, id: &str, name: &str) -> DiagnosticMessage {
91+
pub fn new_entry(&self, id: &str, name: &str) -> DiagnosticMessage {
9292
DiagnosticMessage {
9393
timestamp: chrono::Utc::now(),
9494
source: Source {
@@ -199,9 +199,29 @@ impl DiagnosticLoggers {
199199
}
200200
}
201201

202+
fn longest_backtick_sequence_length(text: &str) -> usize {
203+
let mut result = 0;
204+
let mut count = 0;
205+
for c in text.chars() {
206+
if c == '`' {
207+
count += 1;
208+
} else {
209+
if count > result {
210+
result = count;
211+
}
212+
count = 0;
213+
}
214+
}
215+
result
216+
}
202217
impl DiagnosticMessage {
203218
pub fn full_error_message(&self) -> String {
204219
match &self.location {
220+
Some(Location {
221+
file: Some(path),
222+
start_line: None,
223+
..
224+
}) => format!("{}: {}", path, self.plaintext_message),
205225
Some(Location {
206226
file: Some(path),
207227
start_line: Some(line),
@@ -211,12 +231,38 @@ impl DiagnosticMessage {
211231
}
212232
}
213233

214-
pub fn text(&mut self, text: &str) -> &mut Self {
234+
fn text(&mut self, text: &str) -> &mut Self {
215235
self.plaintext_message = text.to_owned();
216236
self
217237
}
218238

219-
#[allow(unused)]
239+
pub fn message(&mut self, text: &str, args: &[&str]) -> &mut Self {
240+
let parts = text.split("{}");
241+
let args = args.iter().chain(std::iter::repeat(&""));
242+
let mut plain = String::with_capacity(2 * text.len());
243+
let mut markdown = String::with_capacity(2 * text.len());
244+
for (p, a) in parts.zip(args) {
245+
plain.push_str(p);
246+
plain.push_str(a);
247+
markdown.push_str(p);
248+
if a.len() > 0 {
249+
let count = longest_backtick_sequence_length(a) + 1;
250+
markdown.push_str(&"`".repeat(count));
251+
if count > 1 {
252+
markdown.push_str(" ");
253+
}
254+
markdown.push_str(a);
255+
if count > 1 {
256+
markdown.push_str(" ");
257+
}
258+
markdown.push_str(&"`".repeat(count));
259+
}
260+
}
261+
self.text(&plain);
262+
self.markdown(&markdown);
263+
self
264+
}
265+
220266
pub fn markdown(&mut self, text: &str) -> &mut Self {
221267
self.markdown_message = text.to_owned();
222268
self
@@ -249,6 +295,11 @@ impl DiagnosticMessage {
249295
self.visibility.telemetry = true;
250296
self
251297
}
298+
pub fn file(&mut self, path: &str) -> &mut Self {
299+
let loc = self.location.get_or_insert(Default::default());
300+
loc.file = Some(path.to_owned());
301+
self
302+
}
252303
pub fn location(
253304
&mut self,
254305
path: &str,
@@ -266,3 +317,23 @@ impl DiagnosticMessage {
266317
self
267318
}
268319
}
320+
321+
#[test]
322+
fn test_message() {
323+
let mut m = DiagnosticLoggers::new("foo")
324+
.logger()
325+
.new_entry("id", "name");
326+
m.message("hello: {}", &["hello"]);
327+
assert_eq!("hello: hello", m.plaintext_message);
328+
assert_eq!("hello: `hello`", m.markdown_message);
329+
330+
let mut m = DiagnosticLoggers::new("foo")
331+
.logger()
332+
.new_entry("id", "name");
333+
m.message("hello with backticks: {}", &["oh `hello`!"]);
334+
assert_eq!("hello with backticks: oh `hello`!", m.plaintext_message);
335+
assert_eq!(
336+
"hello with backticks: `` oh `hello`! ``",
337+
m.markdown_message
338+
);
339+
}

‎ruby/extractor/src/extractor.rs‎

Lines changed: 70 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,13 @@ impl<'a> Visitor<'a> {
274274
);
275275
}
276276

277-
fn record_parse_error_for_node(&mut self, error_message: String, node: Node) {
277+
fn record_parse_error_for_node(
278+
&mut self,
279+
message: &str,
280+
args: &[&str],
281+
node: Node,
282+
status_page: bool,
283+
) {
278284
let (start_line, start_column, end_line, end_column) = location_for(self, node);
279285
let loc = location(
280286
self.trap_writer,
@@ -284,26 +290,38 @@ impl<'a> Visitor<'a> {
284290
end_line,
285291
end_column,
286292
);
287-
self.record_parse_error(
288-
loc,
289-
self.diagnostics_writer
290-
.message("parse-error", "Parse error")
291-
.severity(diagnostics::Severity::Error)
292-
.location(self.path, start_line, start_column, end_line, end_column)
293-
.text(&error_message),
294-
);
293+
let mut mesg = self
294+
.diagnostics_writer
295+
.new_entry("parse-error", "Parse error");
296+
&mesg
297+
.severity(diagnostics::Severity::Error)
298+
.location(self.path, start_line, start_column, end_line, end_column)
299+
.message(message, args);
300+
if status_page {
301+
&mesg.status_page();
302+
}
303+
self.record_parse_error(loc, &mesg);
295304
}
296305

297306
fn enter_node(&mut self, node: Node) -> bool {
298-
if node.is_error() || node.is_missing() {
299-
let error_message = if node.is_missing() {
300-
format!("parse error: expecting '{}'", node.kind())
301-
} else {
302-
"parse error".to_string()
303-
};
304-
self.record_parse_error_for_node(error_message, node);
307+
if node.is_missing() {
308+
self.record_parse_error_for_node(
309+
"A parse error occurred (expected {} symbol). Check the syntax of the file using the {} command. If the file is invalid, correct the error or exclude the file from analysis.",
310+
&[node.kind(), "ruby -c"],
311+
node,
312+
true,
313+
);
305314
return false;
306315
}
316+
if node.is_error() {
317+
self.record_parse_error_for_node(
318+
"A parse error occurred. Check the syntax of the file using the {} command. If the file is invalid, correct the error or exclude the file from analysis.",
319+
&["ruby -c"],
320+
node,
321+
true,
322+
);
323+
return false;
324+
};
307325

308326
let id = self.trap_writer.fresh_id();
309327

@@ -383,15 +401,13 @@ impl<'a> Visitor<'a> {
383401
}
384402
}
385403
_ => {
386-
let error_message = format!("unknown table type: '{}'", node.kind());
387404
self.record_parse_error(
388405
loc,
389406
self.diagnostics_writer
390-
.message("parse-error", "Parse error")
407+
.new_entry("parse-error", "Parse error")
391408
.severity(diagnostics::Severity::Error)
392409
.location(self.path, start_line, start_column, end_line, end_column)
393-
.text(&error_message)
394-
.status_page(),
410+
.message("Unknown table type: {}", &[node.kind()]),
395411
);
396412

397413
valid = false;
@@ -439,23 +455,29 @@ impl<'a> Visitor<'a> {
439455
values.push(trap::Arg::Label(child_node.label));
440456
}
441457
} else if field.name.is_some() {
442-
let error_message = format!(
443-
"type mismatch for field {}::{} with type {:?} != {:?}",
444-
node.kind(),
445-
child_node.field_name.unwrap_or("child"),
446-
child_node.type_name,
447-
field.type_info
458+
self.record_parse_error_for_node(
459+
"Type mismatch for field {}::{} with type {} != {}",
460+
&[
461+
node.kind(),
462+
child_node.field_name.unwrap_or("child"),
463+
&format!("{:?}", child_node.type_name),
464+
&format!("{:?}", field.type_info),
465+
],
466+
*node,
467+
false,
448468
);
449-
self.record_parse_error_for_node(error_message, *node);
450469
}
451470
} else if child_node.field_name.is_some() || child_node.type_name.named {
452-
let error_message = format!(
453-
"value for unknown field: {}::{} and type {:?}",
454-
node.kind(),
455-
&child_node.field_name.unwrap_or("child"),
456-
&child_node.type_name
471+
self.record_parse_error_for_node(
472+
"Value for unknown field: {}::{} and type {}",
473+
&[
474+
node.kind(),
475+
&child_node.field_name.unwrap_or("child"),
476+
&format!("{:?}", child_node.type_name),
477+
],
478+
*node,
479+
false,
457480
);
458-
self.record_parse_error_for_node(error_message, *node);
459481
}
460482
}
461483
let mut args = Vec::new();
@@ -471,14 +493,14 @@ impl<'a> Visitor<'a> {
471493
let error_message = format!(
472494
"{} for field: {}::{}",
473495
if child_values.is_empty() {
474-
"missing value"
496+
"Missing value"
475497
} else {
476-
"too many values"
498+
"Too many values"
477499
},
478500
node.kind(),
479501
column_name
480502
);
481-
self.record_parse_error_for_node(error_message, *node);
503+
self.record_parse_error_for_node(&error_message, &[], *node, false);
482504
}
483505
}
484506
Storage::Table {
@@ -488,13 +510,12 @@ impl<'a> Visitor<'a> {
488510
} => {
489511
for (index, child_value) in child_values.iter().enumerate() {
490512
if !*has_index && index > 0 {
491-
let error_message = format!(
492-
"too many values for field: {}::{}",
493-
node.kind(),
494-
table_name,
513+
self.record_parse_error_for_node(
514+
"Too many values for field: {}::{}",
515+
&[node.kind(), table_name],
516+
*node,
517+
false,
495518
);
496-
497-
self.record_parse_error_for_node(error_message, *node);
498519
break;
499520
}
500521
let mut args = vec![trap::Arg::Label(parent_id)];
@@ -591,9 +612,8 @@ fn location_for(visitor: &mut Visitor, n: Node) -> (usize, usize, usize, usize)
591612
visitor.diagnostics_writer.write(
592613
visitor
593614
.diagnostics_writer
594-
.message("internal-error", "Internal error")
595-
.text("expecting a line break symbol, but none found while correcting end column value")
596-
.status_page()
615+
.new_entry("internal-error", "Internal error")
616+
.message("Expecting a line break symbol, but none found while correcting end column value", &[])
597617
.severity(diagnostics::Severity::Error),
598618
);
599619
}
@@ -607,13 +627,11 @@ fn location_for(visitor: &mut Visitor, n: Node) -> (usize, usize, usize, usize)
607627
visitor.diagnostics_writer.write(
608628
visitor
609629
.diagnostics_writer
610-
.message("internal-error", "Internal error")
611-
.text(&format!(
612-
"cannot correct end column value: end_byte index {} is not in range [1,{}]",
613-
index,
614-
source.len()
615-
))
616-
.status_page()
630+
.new_entry("internal-error", "Internal error")
631+
.message(
632+
"Cannot correct end column value: end_byte index {} is not in range [1,{}]",
633+
&[&index.to_string(), &source.len().to_string()],
634+
)
617635
.severity(diagnostics::Severity::Error),
618636
);
619637
}

‎ruby/extractor/src/main.rs‎

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,8 @@ fn main() -> std::io::Result<()> {
7373
Err(e) => {
7474
main_thread_logger.write(
7575
main_thread_logger
76-
.message("configuration-error", "Configuration error")
77-
.text(&format!("{}; defaulting to 1 thread.", e))
78-
.status_page()
76+
.new_entry("configuration-error", "Configuration error")
77+
.message("{}; defaulting to 1 thread.", &[&e])
7978
.severity(diagnostics::Severity::Warning),
8079
);
8180
1
@@ -95,9 +94,8 @@ fn main() -> std::io::Result<()> {
9594
Err(e) => {
9695
main_thread_logger.write(
9796
main_thread_logger
98-
.message("configuration-error", "Configuration error")
99-
.text(&format!("{}; using gzip.", e))
100-
.status_page()
97+
.new_entry("configuration-error", "Configuration error")
98+
.message("{}; using gzip.", &[&e])
10199
.severity(diagnostics::Severity::Warning),
102100
);
103101
trap::Compression::Gzip
@@ -199,17 +197,17 @@ fn main() -> std::io::Result<()> {
199197
needs_conversion = false;
200198
diagnostics_writer.write(
201199
diagnostics_writer
200+
.new_entry(
201+
"character-decoding-error",
202+
"Character decoding error",
203+
)
204+
.file(&path.to_string_lossy())
202205
.message(
203-
"character-encoding-error",
204-
"Character encoding error",
206+
"Could not decode the file contents as {}: {}. The contents of the file must match the character encoding specified in the {} directive.",
207+
&[&encoding_name, &msg, "encoding:"],
205208
)
206-
.text(&format!(
207-
"{}: character decoding failure: {} ({})",
208-
&path.to_string_lossy(),
209-
msg,
210-
&encoding_name
211-
))
212209
.status_page()
210+
.help_link("https://docs.ruby-lang.org/en/master/syntax/comments_rdoc.html#label-encoding+Directive")
213211
.severity(diagnostics::Severity::Warning),
214212
);
215213
}
@@ -218,13 +216,14 @@ fn main() -> std::io::Result<()> {
218216
} else {
219217
diagnostics_writer.write(
220218
diagnostics_writer
221-
.message("character-encoding-error", "Character encoding error")
222-
.text(&format!(
223-
"{}: unknown character encoding: '{}'",
224-
&path.to_string_lossy(),
225-
&encoding_name
226-
))
219+
.new_entry("unknown-character-encoding", "Unknown character encoding")
220+
.file(&path.to_string_lossy())
221+
.message(
222+
"Unknown character encoding {} in {} directive.",
223+
&[&encoding_name, "#encoding:"],
224+
)
227225
.status_page()
226+
.help_link("https://docs.ruby-lang.org/en/master/syntax/comments_rdoc.html#label-encoding+Directive")
228227
.severity(diagnostics::Severity::Warning),
229228
);
230229
}

‎ruby/ql/test/query-tests/diagnostics/ExtractionErrors.expected‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
| src/not_ruby.rb:5:25:5:26 | parse error | Extraction failed in src/not_ruby.rb with error parse error | 2 |
1+
| src/not_ruby.rb:5:25:5:26 | A parse error occurred. Check the syntax of the file using the ruby -c command. If the file is invalid, correct the error or exclude the file from analysis. | Extraction failed in src/not_ruby.rb with error A parse error occurred. Check the syntax of the file using the ruby -c command. If the file is invalid, correct the error or exclude the file from analysis. | 2 |

0 commit comments

Comments
 (0)