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.

Sign up for GitHub

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

Jump to bottom

Adjust existing Diagnostic API according to the Expressive Diagnostics RFC #5100

Merged
merged 7 commits into from
Sep 11, 2023
Merged

Adjust existing Diagnostic API according to the Expressive Diagnostics RFC #5100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5431568
Adjust existing diagnostics to the Expressive Diagnostics RFC
ironcev Sep 1, 2023
fc68e63
Adapt tests
ironcev Sep 8, 2023
43531ff
Fix fmt and clippy issues
ironcev Sep 8, 2023
591bfb9
Merge branch 'master' into ironcev/5075-adjust-existing-diagnostic-API
ironcev Sep 8, 2023
bff175e
Use `let Some(reason)` instead of `reason().is_some()`
ironcev Sep 8, 2023
81a9c3f
Merge branch 'master' into ironcev/5075-adjust-existing-diagnostic-API
ironcev Sep 8, 2023
cfa48ff
Merge branch 'master' into ironcev/5075-adjust-existing-diagnostic-API
anton-trunov Sep 10, 2023
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
7 changes: 2 additions & 5 deletions forc-util/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,12 +559,9 @@ fn format_diagnostic(diagnostic: &Diagnostic) {

fn get_title_label(diagnostics: &Diagnostic, label: &mut String) {
label.clear();
if diagnostics.reason().is_some() {
label.push_str(diagnostics.reason().unwrap().description());
label.push_str(". ");
if let Some(reason) = diagnostics.reason() {
label.push_str(reason.description());
}
label.push_str(diagnostics.issue().friendly_text());
label.push('.');
}

fn diagnostic_level_to_annotation_type(level: Level) -> AnnotationType {
Expand Down
18 changes: 14 additions & 4 deletions sway-core/src/semantic_analysis/ast_node/expression/typed_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ impl ty::TyExpression {
span: reachable_report.scrutinee.span.clone(),
warning_content: Warning::MatchExpressionUnreachableArm {
match_value: value.span(),
match_type: engines.help_out(type_id).to_string(),
preceding_arms: Either::Right(
arms_reachability[catch_all_arm_position]
.scrutinee
Expand All @@ -679,7 +680,9 @@ impl ty::TyExpression {
//...but still check the arms above it for reachability
check_interior_non_catch_all_arms_for_reachability(
handler,
&value.span(),
engines,
type_id,
&value,
&arms_reachability[..catch_all_arm_position],
);
}
Expand All @@ -690,7 +693,9 @@ impl ty::TyExpression {
// check reachable report for all the arms except the last one
check_interior_non_catch_all_arms_for_reachability(
handler,
&value.span(),
engines,
type_id,
&value,
other_arms_reachability,
);

Expand All @@ -700,6 +705,7 @@ impl ty::TyExpression {
span: last_arm_report.scrutinee.span.clone(),
warning_content: Warning::MatchExpressionUnreachableArm {
match_value: value.span(),
match_type: engines.help_out(type_id).to_string(),
preceding_arms: Either::Left(
other_arms_reachability
.iter()
Expand All @@ -721,6 +727,7 @@ impl ty::TyExpression {
for duplicate in collect_duplicate_match_pattern_variables(scrutinee) {
handler.emit_err(CompileError::MultipleDefinitionsOfMatchArmVariable {
match_value: value.span(),
match_type: engines.help_out(type_id).to_string(),
first_definition: duplicate.first_definition.1,
first_definition_is_struct_field: duplicate.first_definition.0,
duplicate: duplicate.duplicate.1,
Expand Down Expand Up @@ -767,15 +774,18 @@ impl ty::TyExpression {

fn check_interior_non_catch_all_arms_for_reachability(
handler: &Handler,
match_value: &Span,
engines: &Engines,
type_id: TypeId,
match_value: &Expression,
arms_reachability: &[ReachableReport],
) {
for (index, reachable_report) in arms_reachability.iter().enumerate() {
if !reachable_report.reachable {
handler.emit_warn(CompileWarning {
span: reachable_report.scrutinee.span.clone(),
warning_content: Warning::MatchExpressionUnreachableArm {
match_value: match_value.clone(),
match_value: match_value.span(),
match_type: engines.help_out(type_id).to_string(),
preceding_arms: Either::Left(
arms_reachability[..index]
.iter()
Expand Down
10 changes: 4 additions & 6 deletions sway-error/src/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,12 @@ impl Diagnostic {
pub fn labels(&self) -> Vec<&Label> {
let mut labels = Vec::<&Label>::new();

for hint in self.hints.iter().filter(|hint| hint.is_in_source()) {
labels.push(hint);
if self.issue.is_in_source() {
labels.push(&self.issue);
}

// If the issue is in source and there are no hints that override the issue,
// add the issue to the labels.
if self.issue.is_in_source() && self.hints.iter().all(|hint| hint.span != self.issue.span) {
labels.push(&self.issue);
for hint in self.hints.iter().filter(|hint| hint.is_in_source()) {
labels.push(hint);
}

labels
Expand Down
46 changes: 14 additions & 32 deletions sway-error/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ pub enum CompileError {
#[error("Variable \"{}\" is already defined in match arm.", first_definition.as_str())]
MultipleDefinitionsOfMatchArmVariable {
match_value: Span,
match_type: String,
first_definition: Span,
first_definition_is_struct_field: bool,
duplicate: Span,
Expand Down Expand Up @@ -887,7 +888,7 @@ impl ToDiagnostic for CompileError {
// Constant "x" shadows imported constant with the same name
// or
// ...
"{variable_or_constant} \"{name}\" shadows {}constant with the same name",
"{variable_or_constant} \"{name}\" shadows {}constant of the same name.",
if constant_decl.clone() != Span::dummy() { "imported " } else { "" }
)
),
Expand All @@ -899,7 +900,7 @@ impl ToDiagnostic for CompileError {
// Constant "x" is declared here.
// or
// Constant "x" gets imported here.
"Constant \"{name}\" {} here{}.",
"Shadowed constant \"{name}\" {} here{}.",
if constant_decl.clone() != Span::dummy() { "gets imported" } else { "is declared" },
if *is_alias { " as alias" } else { "" }
)
Expand All @@ -909,14 +910,6 @@ impl ToDiagnostic for CompileError {
constant_decl.clone(),
format!("This is the original declaration of the imported constant \"{name}\".")
),
Hint::error(
source_engine,
name.span(),
format!(
"Shadowing via {} \"{name}\" happens here.",
if variable_or_constant == "Variable" { "variable" } else { "new constant" }
)
),
],
help: vec![
format!("Unlike variables, constants cannot be shadowed by other constants or variables."),
Expand All @@ -937,38 +930,28 @@ impl ToDiagnostic for CompileError {
issue: Issue::error(
source_engine,
name.span(),
format!("Constant \"{name}\" shadows variable with the same name")
format!("Constant \"{name}\" shadows variable of the same name.")
),
hints: vec![
Hint::info(
source_engine,
variable_span.clone(),
format!("This is the shadowed variable \"{name}\".")
),
Hint::error(
source_engine,
name.span(),
format!("This is the constant \"{name}\" that shadows the variable.")
),
],
help: vec![
format!("Variables can shadow other variables, but constants cannot."),
format!("Consider renaming either the variable or the constant."),
],
},
MultipleDefinitionsOfMatchArmVariable { match_value, first_definition, first_definition_is_struct_field, duplicate, duplicate_is_struct_field } => Diagnostic {
reason: Some(Reason::new(code(1), "Variable is already defined in match arm".to_string())),
MultipleDefinitionsOfMatchArmVariable { match_value, match_type, first_definition, first_definition_is_struct_field, duplicate, duplicate_is_struct_field } => Diagnostic {
reason: Some(Reason::new(code(1), "Match pattern variable is already defined".to_string())),
issue: Issue::error(
source_engine,
first_definition.clone(),
format!("Variable \"{}\" is already defined in match arm", first_definition.as_str())
duplicate.clone(),
format!("Variable \"{}\" is already defined in this match arm.", first_definition.as_str())
),
hints: vec![
Hint::error(
source_engine,
duplicate.clone(),
format!("Variable \"{}\" is already defined in this match arm pattern.", first_definition.as_str())
),
Hint::help(
source_engine,
if *duplicate_is_struct_field {
Expand All @@ -990,7 +973,7 @@ impl ToDiagnostic for CompileError {
else {
"".to_string()
},
first_definition.as_str()
first_definition.as_str(),
)
),
Hint::help(
Expand All @@ -1001,28 +984,27 @@ impl ToDiagnostic for CompileError {
else {
Span::dummy()
},
format!("Struct field \"{0}\" is just a shorthand notation for `{0}: {0}`. It defines a variable \"{0}\".", first_definition.as_str())
format!("Struct field \"{0}\" is just a shorthand notation for `{0}: {0}`. It defines a variable \"{0}\".", first_definition.as_str()),
),
Hint::info(
source_engine,
match_value.clone(),
"This is the value to match on.".to_string()
format!("`{}`, of type \"{match_type}\", is the value to match on.", match_value.as_str())
),
],
help: vec![
"Variables used in match arm patterns must be unique within a pattern.".to_string(),
format!("Variables used in match arm patterns must be unique within a pattern, except in alternatives."),
match (*first_definition_is_struct_field, *duplicate_is_struct_field) {
(true, true) => format!("Consider declaring a variable with different name for either of the fields. E.g., `{0}: var_{0}`.", first_definition.as_str()),
(true, false) | (false, true) => format!("Consider declaring a variable with different name for the field (e.g., `{0}: var_{0}`), or renaming the variable \"{0}\".", first_definition.as_str()),
(true, false) | (false, true) => format!("Consider declaring a variable for the field \"{0}\" (e.g., `{0}: var_{0}`), or renaming the variable \"{0}\".", first_definition.as_str()),
(false, false) => "Consider renaming either of the variables.".to_string(),
},
],

},
_ => Diagnostic {
// TODO: Temporary we use self here to achieve backward compatibility.
// In general, self must not be used and will not be used once we
// switch to our own #[error] macro. All the values for the formating
// switch to our own #[error] macro. All the values for the formatting
// of a diagnostic must come from the enum variant parameters.
issue: Issue::error(source_engine, self.span(), format!("{}", self)),
..Default::default()
Expand Down
40 changes: 17 additions & 23 deletions sway-error/src/warning.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ pub enum Warning {
},
MatchExpressionUnreachableArm {
match_value: Span,
match_type: String,
// Either preceding non catch-all arms or a single interior catch-all arm.
preceding_arms: Either<Vec<Span>, Span>,
unreachable_arm: Span,
Expand Down Expand Up @@ -267,58 +268,52 @@ impl ToDiagnostic for CompileWarning {
issue: Issue::warning(
source_engine,
name.span(),
format!("Constant \"{name}\" should be SCREAMING_SNAKE_CASE"),
format!("Constant \"{name}\" should be SCREAMING_SNAKE_CASE."),
),
hints: vec![
Hint::warning(
Hint::help(
source_engine,
name.span(),
format!("\"{name}\" should be SCREAMING_SNAKE_CASE, like \"{}\".", to_screaming_snake_case(name.as_str())),
format!("Consider renaming it to, e.g., \"{}\".", to_screaming_snake_case(name.as_str())),
),
],
help: vec![
format!("In Sway, ABIs, structs, traits, and enums are CapitalCase."),
format!("Modules, variables, and functions are snake_case, while constants are SCREAMING_SNAKE_CASE."),
format!("Consider renaming the constant to, e.g., \"{}\".", to_screaming_snake_case(name.as_str())),
],
},
MatchExpressionUnreachableArm { match_value, preceding_arms, unreachable_arm, is_last_arm, is_catch_all_arm } => Diagnostic {
MatchExpressionUnreachableArm { match_value, match_type, preceding_arms, unreachable_arm, is_last_arm, is_catch_all_arm } => Diagnostic {
reason: Some(Reason::new(code(1), "Match arm is unreachable".to_string())),
issue: Issue::warning(
source_engine,
unreachable_arm.clone(),
match (*is_last_arm, *is_catch_all_arm) {
(true, true) => format!("Catch-all pattern \"{}\" in the last match arm will never be matched", unreachable_arm.as_str()),
_ => format!("Pattern \"{}\" will never be matched", unreachable_arm.as_str())
(true, true) => format!("Last catch-all match arm `{}` is unreachable.", unreachable_arm.as_str()),
_ => format!("Match arm `{}` is unreachable.", unreachable_arm.as_str())
}
),
hints: vec![
Hint::warning(
source_engine,
unreachable_arm.clone(),
format!("{} arm \"{}\" is unreachable.", if *is_last_arm && *is_catch_all_arm { "Last catch-all match" } else { "Match" }, unreachable_arm.as_str())
),
Hint::info(
source_engine,
match_value.clone(),
"This is the value to match on.".to_string()
format!("`{}`, of type \"{match_type}\", is the value to match on.", match_value.as_str())
ironcev marked this conversation as resolved.
Show resolved Hide resolved
),
if preceding_arms.is_right() {
Hint::warning(
Hint::help(
source_engine,
preceding_arms.as_ref().unwrap_right().clone(),
format!("Catch-all arm \"{}\" makes all the arms below it unreachable.", preceding_arms.as_ref().unwrap_right().as_str())
format!("Catch-all arm `{}` makes all match arms below it unreachable.", preceding_arms.as_ref().unwrap_right().as_str())
)
}
else {
Hint::info(
source_engine,
Span::join_all(preceding_arms.as_ref().unwrap_left().clone()),
if *is_last_arm {
format!("Preceding match arms already match all possible values of \"{}\".", match_value.as_str())
format!("Preceding match arms already match all possible values of `{}`.", match_value.as_str())
}
else {
format!("Preceding match arms already match all the values that \"{}\" can match.", unreachable_arm.as_str())
format!("Preceding match arms already match all the values that `{}` can match.", unreachable_arm.as_str())
}
)
}
Expand All @@ -327,21 +322,20 @@ impl ToDiagnostic for CompileWarning {
let catch_all_arm = preceding_arms.as_ref().unwrap_right().as_str();
vec![
format!("Catch-all patterns make sense only in last match arms."),
format!("Carefully check matching logic in all the arms and consider:"),
format!(" - removing the catch-all arm \"{catch_all_arm}\" or making it the last arm."),
format!(" - removing the unreachable arms below \"{catch_all_arm}\"."),
format!("Consider removing the catch-all arm `{catch_all_arm}` or making it the last arm."),
format!("Consider removing the unreachable arms below the `{catch_all_arm}` arm."),
]
}
else if *is_last_arm && *is_catch_all_arm {
vec![
format!("Catch-all patterns are often used in last match arms."),
format!("But in this case, the preceding arms already match all possible values of \"{}\".", match_value.as_str()),
format!("Carefully check matching logic in all the arms and consider removing the unreachable last catch-all arm."),
format!("But in this case, the preceding arms already match all possible values of `{}`.", match_value.as_str()),
format!("Consider removing the unreachable last catch-all arm."),
]
}
else {
vec![
format!("Carefully check matching logic in all the arms and consider removing the unreachable arm."),
format!("Consider removing the unreachable arm."),
]
}
},
Expand Down
Loading
Loading