Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Cargo's JSON diagnostics for suggestions #767

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions ui/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions ui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ fork-bomb-prevention = []
[dependencies]
bodyparser = "0.8.0"
corsware = "0.2.0"
cargo_metadata = "0.14.1"
dotenv = "0.15.0"
env_logger = "0.9.0"
iron = "0.6.0"
Expand Down
9 changes: 5 additions & 4 deletions ui/frontend/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export enum ActionType {
CompileWasmFailed = 'COMPILE_WASM_FAILED',
EditCode = 'EDIT_CODE',
AddMainFunction = 'ADD_MAIN_FUNCTION',
AddImport = 'ADD_IMPORT',
ApplySuggestion = 'ADD_SUGGESTION',
EnableFeatureGate = 'ENABLE_FEATURE_GATE',
GotoPosition = 'GOTO_POSITION',
SelectText = 'SELECT_TEXT',
Expand Down Expand Up @@ -460,8 +460,9 @@ export const editCode = (code: string) =>
export const addMainFunction = () =>
createAction(ActionType.AddMainFunction);

export const addImport = (code: string) =>
createAction(ActionType.AddImport, { code });
export const applySuggestion =
(startline: number, startcol: number, endline: number, endcol: number, suggestion: string) =>
createAction(ActionType.ApplySuggestion, { startline, startcol, endline, endcol, suggestion });

export const enableFeatureGate = (featureGate: string) =>
createAction(ActionType.EnableFeatureGate, { featureGate });
Expand Down Expand Up @@ -841,7 +842,7 @@ export type Action =
| ReturnType<typeof receiveCompileWasmFailure>
| ReturnType<typeof editCode>
| ReturnType<typeof addMainFunction>
| ReturnType<typeof addImport>
| ReturnType<typeof applySuggestion>
| ReturnType<typeof enableFeatureGate>
| ReturnType<typeof gotoPosition>
| ReturnType<typeof selectText>
Expand Down
19 changes: 13 additions & 6 deletions ui/frontend/highlighting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export function configureRustErrors({
getChannel,
gotoPosition,
selectText,
addImport,
applySuggestion,
reExecuteWithBacktrace,
}) {
Prism.languages.rust_errors = {
Expand All @@ -30,9 +30,9 @@ export function configureRustErrors({
},
'error-location': /-->\s+(\/playground\/)?src\/.*\n/,
'import-suggestion-outer': {
pattern: /\|\s+use\s+([^;]+);/,
pattern: /\[\[Line\s\d+\sCol\s\d+\s-\sLine\s\d+\sCol\s\d+:\s[.\s\S]*?\]\]/,
inside: {
'import-suggestion': /use\s+.*/,
'import-suggestion': /\[\[Line\s\d+\sCol\s\d+\s-\sLine\s\d+\sCol\s\d+:\s[.\s\S]*?\]\]/,
},
},
'rust-errors-help': {
Expand Down Expand Up @@ -87,9 +87,16 @@ export function configureRustErrors({
env.attributes['data-col'] = col;
}
if (env.type === 'import-suggestion') {
const errorMatch = /\[\[Line\s(\d+)\sCol\s(\d+)\s-\sLine\s(\d+)\sCol\s(\d+):\s([.\s\S]*?)\]\]/.exec(env.content);
const [_, startLine, startCol, endLine, endCol, importSuggestion] = errorMatch;
env.tag = 'a';
env.attributes.href = '#';
env.attributes['data-suggestion'] = env.content;
env.attributes['data-startline'] = startLine;
env.attributes['data-startcol'] = startCol;
env.attributes['data-endline'] = endLine;
env.attributes['data-endcol'] = endCol;
env.attributes['data-suggestion'] = importSuggestion;
env.content = 'Apply \"' + importSuggestion.trim() + '\"\n';
}
if (env.type === 'feature-gate') {
const [_, featureGate] = /feature\((.*?)\)/.exec(env.content);
Expand Down Expand Up @@ -134,10 +141,10 @@ export function configureRustErrors({

const importSuggestions = env.element.querySelectorAll('.import-suggestion');
Array.from(importSuggestions).forEach((link: HTMLAnchorElement) => {
const { suggestion } = link.dataset;
const { startline, startcol, endline, endcol, suggestion } = link.dataset;
link.onclick = (e) => {
e.preventDefault();
addImport(suggestion + '\n');
applySuggestion(startline, startcol, endline, endcol, suggestion);
};
});

Expand Down
5 changes: 3 additions & 2 deletions ui/frontend/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
enableFeatureGate,
gotoPosition,
selectText,
addImport,
applySuggestion,
performCratesLoad,
performVersionsLoad,
reExecuteWithBacktrace,
Expand Down Expand Up @@ -61,7 +61,8 @@ configureRustErrors({
enableFeatureGate: featureGate => store.dispatch(enableFeatureGate(featureGate)),
gotoPosition: (line, col) => store.dispatch(gotoPosition(line, col)),
selectText: (start, end) => store.dispatch(selectText(start, end)),
addImport: (code) => store.dispatch(addImport(code)),
applySuggestion: (startline, startcol, endline, endcol, suggestion) =>
store.dispatch(applySuggestion(startline, startcol, endline, endcol, suggestion)),
reExecuteWithBacktrace: () => store.dispatch(reExecuteWithBacktrace()),
getChannel: () => store.getState().configuration.channel,
});
Expand Down
26 changes: 24 additions & 2 deletions ui/frontend/reducers/code.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,30 @@ export default function code(state = DEFAULT, action: Action): State {
case ActionType.AddMainFunction:
return `${state}\n\n${DEFAULT}`;

case ActionType.AddImport:
return action.code + state;
case ActionType.ApplySuggestion:
const state_lines = state.split('\n');
const startline = action.startline - 1;
const endline = action.endline - 1;
const startcol = action.startcol - 1;
const endcol = action.endcol - 1;
if (startline == endline) {
state_lines[startline] = state_lines[startline].substring(0, startcol) +
state_lines[startline].substring(endcol);
} else {
if (state_lines.length > startline) {
state_lines[startline] = state_lines[startline].substring(0, startcol);
}
if (state_lines.length > endline) {
state_lines[endline] = state_lines[endline].substring(endcol);
}
if (endline - startline > 1) {
state_lines.splice(startline + 1, endline - startline - 1);
}
}
state_lines[startline] = state_lines[startline].substring(0, startcol) +
action.suggestion + state_lines[startline].substring(startcol);
state = state_lines.join('\n');
return state;
Comment on lines +28 to +45
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is tricky enough that I can't just glance at it and feel very confident about what it's doing. It'd be helpful to extract out the core logic here and write some unit tests to exercise the various ifs. Something like this sketch:

    case ActionType.ApplySuggestion:
       const startline = action.startline - 1;
       const endline = action.endline - 1;
       const startcol = action.startcol - 1;
       const endcol = action.endcol - 1;
       return someLogic(state);


case ActionType.EnableFeatureGate:
return `#![feature(${action.featureGate})]\n${state}`;
Expand Down
89 changes: 84 additions & 5 deletions ui/src/sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ pub enum Error {
UnableToReadOutput { source: io::Error },
#[snafu(display("Unable to read crate information: {}", source))]
UnableToParseCrateInformation { source: ::serde_json::Error },
#[snafu(display("Unable to parse cargo output: {}", source))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[snafu(display("Unable to parse cargo output: {}", source))]
#[snafu(display("Unable to parse Cargo output: {}", source))]

UnableToParseCargoOutput { source: io::Error },
#[snafu(display("Output was not valid UTF-8: {}", source))]
OutputNotUtf8 { source: string::FromUtf8Error },
#[snafu(display("Output was missing"))]
Expand Down Expand Up @@ -146,8 +148,9 @@ impl Sandbox {
.map(|entry| entry.path())
.find(|path| path.extension() == Some(req.target.extension()));

let stdout = vec_to_str(output.stdout)?;
let (stdout, stderr_tail) = parse_json_output(output.stdout)?;
let mut stderr = vec_to_str(output.stderr)?;
stderr.push_str(&stderr_tail);

let mut code = match file {
Some(file) => read(&file)?.unwrap_or_else(String::new),
Expand Down Expand Up @@ -193,10 +196,14 @@ impl Sandbox {

let output = run_command_with_timeout(command)?;

let (stdout, stderr_tail) = parse_json_output(output.stdout)?;
let mut stderr = vec_to_str(output.stderr)?;
stderr.push_str(&stderr_tail);

Ok(ExecuteResponse {
success: output.status.success(),
stdout: vec_to_str(output.stdout)?,
stderr: vec_to_str(output.stderr)?,
stdout,
stderr,
})
}

Expand Down Expand Up @@ -526,8 +533,14 @@ fn build_execution_command(
(Some(Wasm), _, _) => cmd.push("wasm"),
(Some(_), _, _) => cmd.push("rustc"),
(_, _, true) => cmd.push("test"),
(_, Library(_), _) => cmd.push("build"),
(_, _, _) => cmd.push("run"),
(_, Library(_), _) => {
cmd.push("build");
cmd.push("--message-format=json");
}
(_, _, _) => {
cmd.push("run");
cmd.push("--message-format=json")
}
}

if mode == Release {
Expand Down Expand Up @@ -571,6 +584,72 @@ fn build_execution_command(
cmd
}

fn parse_json_output(output: Vec<u8>) -> Result<(String, String)> {
let mut composed_stderr_string = String::new();
let mut composed_stdout_string = String::new();

let metadata_stream = cargo_metadata::Message::parse_stream(&output[..]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have expected you could elide the [..]:

Suggested change
let metadata_stream = cargo_metadata::Message::parse_stream(&output[..]);
let metadata_stream = cargo_metadata::Message::parse_stream(&output);


for msg in metadata_stream {
let message = msg.context(UnableToParseCargoOutputSnafu)?;

match message {
cargo_metadata::Message::TextLine(line) => {
composed_stdout_string.push_str(&(line + "\n"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same idea for using write! can be applied here.

}

cargo_metadata::Message::CompilerMessage(cargo_metadata::CompilerMessage {
message,
..
}) => {
composed_stderr_string.push_str(&parse_diagnostic(message, true));
}

_ => {}
}
}

Ok((composed_stdout_string, composed_stderr_string))
}

fn parse_diagnostic(
diagnostic: cargo_metadata::diagnostic::Diagnostic,
should_output_message: bool,
) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of building a brand new string here, it'd be a bit more efficient to accept a &mut String argument and append to it directly.

let mut diagnostic_string = String::new();

if should_output_message {
if let Some(rendered_msg) = diagnostic.rendered {
diagnostic_string.push_str(&rendered_msg);
} else {
diagnostic_string.push_str(&diagnostic.message);
}
Comment on lines +622 to +626
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a required changed, but this could be something along the lines of this (maybe with some .as_ref or as_deref calls added):

Suggested change
if let Some(rendered_msg) = diagnostic.rendered {
diagnostic_string.push_str(&rendered_msg);
} else {
diagnostic_string.push_str(&diagnostic.message);
}
let x = diagnostic.rendered.or(&diagnostic.message)
diagnostic_string.push_str(x);

}

for span in diagnostic.spans {
if span.file_name != "src/lib.rs" && span.file_name != "src/main.rs" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My brain is slow at parsing this. Maybe we could use

Suggested change
if span.file_name != "src/lib.rs" && span.file_name != "src/main.rs" {
if !matches!("src/lib.rs" | "src/main.rs", span.file_name) {

continue;
}

let label = if let Some(label) = span.suggested_replacement {
label
} else {
continue;
};

diagnostic_string.push_str(&format!(
"\n[[Line {} Col {} - Line {} Col {}: {}]]",
span.line_start, span.column_start, span.line_end, span.column_end, label
));
Comment on lines +640 to +643
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating the intermediate String via format!, you can format directly to the &mut String using write!. See also How can I append a formatted string to an existing String?

}

for children in diagnostic.children {
diagnostic_string.push_str(&parse_diagnostic(children, false));
}

diagnostic_string
}

fn set_execution_environment(
cmd: &mut Command,
target: Option<CompileTarget>,
Expand Down