mirror of
https://github.com/astral-sh/ruff.git
synced 2025-08-04 18:58:04 +00:00
ruff_db: tweak how the revealed type diagnostic is rendered
In the new diagnostic data model, we really should have a main diagnostic message *and* a primary span (with an optional message attached to it) for every diagnostic. In this commit, I try to make this true for the "revealed type" diagnostic. Instead of the annotation saying both "revealed type is" and also the revealed type itself, the annotation is now just the revealed type and the main diagnostic message is "Revealed type." I expect this may be controversial. I'm open to doing something different. I tried to avoid redundancy, but maybe this is a special case where we want the redundancy. I'm honestly not sure. I do *like* how it looks with this commit, but I'm not working with Red Knot's type checking daily, so my opinion doesn't count for much. This did also require some tweaking to concise diagnostic formatting in order to preserve the essential information. This commit doesn't update every relevant snapshot. Just a few. I split the rest out into the next commit.
This commit is contained in:
Andrew Gallant
Andrew Gallant
parent
75b15ea2d0
commit
28b64064f5
4 changed files with 111 additions and 16 deletions
|
|
@ -83,13 +83,13 @@ fn config_override_python_platform() -> anyhow::Result<()> {
|
|||
success: true
|
||||
exit_code: 0
|
||||
----- stdout -----
|
||||
info: revealed-type
|
||||
info: revealed-type: Revealed type
|
||||
--> <temp_dir>/test.py:5:1
|
||||
|
|
||||
3 | from typing_extensions import reveal_type
|
||||
4 |
|
||||
5 | reveal_type(sys.platform)
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Revealed type is `Literal["linux"]`
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^ `Literal["linux"]`
|
||||
|
|
||||
|
||||
Found 1 diagnostic
|
||||
|
|
@ -101,13 +101,13 @@ fn config_override_python_platform() -> anyhow::Result<()> {
|
|||
success: true
|
||||
exit_code: 0
|
||||
----- stdout -----
|
||||
info: revealed-type
|
||||
info: revealed-type: Revealed type
|
||||
--> <temp_dir>/test.py:5:1
|
||||
|
|
||||
3 | from typing_extensions import reveal_type
|
||||
4 |
|
||||
5 | reveal_type(sys.platform)
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Revealed type is `LiteralString`
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^ `LiteralString`
|
||||
|
|
||||
|
||||
Found 1 diagnostic
|
||||
|
|
@ -584,12 +584,12 @@ fn exit_code_only_info() -> anyhow::Result<()> {
|
|||
success: true
|
||||
exit_code: 0
|
||||
----- stdout -----
|
||||
info: revealed-type
|
||||
info: revealed-type: Revealed type
|
||||
--> <temp_dir>/test.py:3:1
|
||||
|
|
||||
2 | from typing_extensions import reveal_type
|
||||
3 | reveal_type(1)
|
||||
| ^^^^^^^^^^^^^^ Revealed type is `Literal[1]`
|
||||
| ^^^^^^^^^^^^^^ `Literal[1]`
|
||||
|
|
||||
|
||||
Found 1 diagnostic
|
||||
|
|
@ -614,12 +614,12 @@ fn exit_code_only_info_and_error_on_warning_is_true() -> anyhow::Result<()> {
|
|||
success: true
|
||||
exit_code: 0
|
||||
----- stdout -----
|
||||
info: revealed-type
|
||||
info: revealed-type: Revealed type
|
||||
--> <temp_dir>/test.py:3:1
|
||||
|
|
||||
2 | from typing_extensions import reveal_type
|
||||
3 | reveal_type(1)
|
||||
| ^^^^^^^^^^^^^^ Revealed type is `Literal[1]`
|
||||
| ^^^^^^^^^^^^^^ `Literal[1]`
|
||||
|
|
||||
|
||||
Found 1 diagnostic
|
||||
|
|
@ -1076,7 +1076,7 @@ fn concise_revealed_type() -> anyhow::Result<()> {
|
|||
success: true
|
||||
exit_code: 0
|
||||
----- stdout -----
|
||||
info[revealed-type] <temp_dir>/test.py:5:1: Revealed type is `Literal["hello"]`
|
||||
info[revealed-type] <temp_dir>/test.py:5:1: Revealed type: `Literal["hello"]`
|
||||
Found 1 diagnostic
|
||||
|
||||
----- stderr -----
|
||||
|
|
|
|||
|
|
@ -4105,15 +4105,15 @@ impl<'db> TypeInferenceBuilder<'db> {
|
|||
match known_function {
|
||||
KnownFunction::RevealType => {
|
||||
if let [Some(revealed_type)] = overload.parameter_types() {
|
||||
if let Some(mut reporter) = self.context.report(
|
||||
DiagnosticId::RevealedType,
|
||||
Severity::Info,
|
||||
"",
|
||||
) {
|
||||
if let Some(builder) = self
|
||||
.context
|
||||
.report(DiagnosticId::RevealedType, Severity::Info)
|
||||
{
|
||||
let mut reporter = builder.build("Revealed type");
|
||||
let span = self.context.span(call_expression);
|
||||
reporter.diagnostic().annotate(
|
||||
Annotation::primary(span).message(format_args!(
|
||||
"Revealed type is `{}`",
|
||||
"`{}`",
|
||||
revealed_type.display(self.db())
|
||||
)),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -123,6 +123,13 @@ impl Diagnostic {
|
|||
/// Returns the primary message for this diagnostic.
|
||||
///
|
||||
/// A diagnostic always has a message, but it may be empty.
|
||||
///
|
||||
/// NOTE: At present, this routine will return the first primary
|
||||
/// annotation's message as the primary message when the main diagnostic
|
||||
/// message is empty. This is meant to facilitate an incremental migration
|
||||
/// in Red Knot over to the new diagnostic data model. (The old data model
|
||||
/// didn't distinguish between messages on the entire diagnostic and
|
||||
/// messages attached to a particular span.)
|
||||
pub fn primary_message(&self) -> &str {
|
||||
if !self.inner.message.is_empty() {
|
||||
return &self.inner.message;
|
||||
|
|
@ -140,6 +147,44 @@ impl Diagnostic {
|
|||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// Introspects this diagnostic and returns what kind of "primary" message
|
||||
/// it contains for concise formatting.
|
||||
///
|
||||
/// When we concisely format diagnostics, we likely want to not only
|
||||
/// include the primary diagnostic message but also the message attached
|
||||
/// to the primary annotation. In particular, the primary annotation often
|
||||
/// contains *essential* information or context for understanding the
|
||||
/// diagnostic.
|
||||
///
|
||||
/// The reason why we don't just always return both the main diagnostic
|
||||
/// message and the primary annotation message is because this was written
|
||||
/// in the midst of an incremental migration of Red Knot over to the new
|
||||
/// diagnostic data model. At time of writing, diagnostics were still
|
||||
/// constructed in the old model where the main diagnostic message and the
|
||||
/// primary annotation message were not distinguished from each other. So
|
||||
/// for now, we carefully return what kind of messages this diagnostic
|
||||
/// contains. In effect, if this diagnostic has a non-empty main message
|
||||
/// *and* a non-empty primary annotation message, then the diagnostic is
|
||||
/// 100% using the new diagnostic data model and we can format things
|
||||
/// appropriately.
|
||||
///
|
||||
/// The type returned implements the `std::fmt::Display` trait. In most
|
||||
/// cases, just converting it to a string (or printing it) will do what
|
||||
/// you want.
|
||||
pub fn concise_message(&self) -> ConciseMessage {
|
||||
let main = &self.inner.message;
|
||||
let annotation = self
|
||||
.primary_annotation()
|
||||
.and_then(|ann| ann.message.as_deref())
|
||||
.unwrap_or_default();
|
||||
match (main.is_empty(), annotation.is_empty()) {
|
||||
(false, true) => ConciseMessage::MainDiagnostic(main),
|
||||
(true, false) => ConciseMessage::PrimaryAnnotation(annotation),
|
||||
(false, false) => ConciseMessage::Both { main, annotation },
|
||||
(true, true) => ConciseMessage::Empty,
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns the severity of this diagnostic.
|
||||
///
|
||||
/// Note that this may be different than the severity of sub-diagnostics.
|
||||
|
|
@ -315,6 +360,11 @@ impl Annotation {
|
|||
let message = Some(message.to_string().into_boxed_str());
|
||||
Annotation { message, ..self }
|
||||
}
|
||||
|
||||
/// Returns the message attached to this annotation, if one exists.
|
||||
pub fn get_message(&self) -> Option<&str> {
|
||||
self.message.as_deref()
|
||||
}
|
||||
}
|
||||
|
||||
/// A string identifier for a lint rule.
|
||||
|
|
@ -608,6 +658,51 @@ pub enum DiagnosticFormat {
|
|||
Concise,
|
||||
}
|
||||
|
||||
/// A representation of the kinds of messages inside a diagnostic.
|
||||
pub enum ConciseMessage<'a> {
|
||||
/// A diagnostic contains a non-empty main message and an empty
|
||||
/// primary annotation message.
|
||||
///
|
||||
/// This strongly suggests that the diagnostic is using the
|
||||
/// "new" data model.
|
||||
MainDiagnostic(&'a str),
|
||||
/// A diagnostic contains an empty main message and a non-empty
|
||||
/// primary annotation message.
|
||||
///
|
||||
/// This strongly suggests that the diagnostic is using the
|
||||
/// "old" data model.
|
||||
PrimaryAnnotation(&'a str),
|
||||
/// A diagnostic contains a non-empty main message and a non-empty
|
||||
/// primary annotation message.
|
||||
///
|
||||
/// This strongly suggests that the diagnostic is using the
|
||||
/// "new" data model.
|
||||
Both { main: &'a str, annotation: &'a str },
|
||||
/// A diagnostic contains an empty main message and an empty
|
||||
/// primary annotation message.
|
||||
///
|
||||
/// This indicates that the diagnostic is probably using the old
|
||||
/// model.
|
||||
Empty,
|
||||
}
|
||||
|
||||
impl std::fmt::Display for ConciseMessage<'_> {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
|
||||
match *self {
|
||||
ConciseMessage::MainDiagnostic(main) => {
|
||||
write!(f, "{main}")
|
||||
}
|
||||
ConciseMessage::PrimaryAnnotation(annotation) => {
|
||||
write!(f, "{annotation}")
|
||||
}
|
||||
ConciseMessage::Both { main, annotation } => {
|
||||
write!(f, "{main}: {annotation}")
|
||||
}
|
||||
ConciseMessage::Empty => Ok(()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Creates a `Diagnostic` from a parse error.
|
||||
///
|
||||
/// This should _probably_ be a method on `ruff_python_parser::ParseError`, but
|
||||
|
|
|
|||
|
|
@ -76,7 +76,7 @@ impl std::fmt::Display for DisplayDiagnostic<'_> {
|
|||
}
|
||||
write!(f, ":")?;
|
||||
}
|
||||
return writeln!(f, " {message}", message = self.diag.primary_message());
|
||||
return writeln!(f, " {}", self.diag.concise_message());
|
||||
}
|
||||
|
||||
let resolved = Resolved::new(&self.resolver, self.diag);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue