From 54ce1dda3a453c5f90b07ea2c89a86f8c39149eb Mon Sep 17 00:00:00 2001 From: Chayim Refael Friedman Date: Mon, 16 Dec 2024 07:45:25 +0200 Subject: [PATCH] Report unresolved idents for implicit captures in `format_args!()` And also a bit of cleanup by storing the capture's span with the open quote included. --- crates/hir-def/src/body.rs | 52 ++++++++++++++----- crates/hir-def/src/body/lower.rs | 20 +++++-- crates/hir-def/src/body/lower/asm.rs | 10 ++-- crates/hir-def/src/hir/format_args.rs | 38 +++++++------- crates/hir/src/diagnostics.rs | 21 +++++--- crates/hir/src/semantics.rs | 16 +++--- .../src/handlers/unresolved_ident.rs | 16 +++--- 7 files changed, 110 insertions(+), 63 deletions(-) diff --git a/crates/hir-def/src/body.rs b/crates/hir-def/src/body.rs index 867bee95be..433a956ff9 100644 --- a/crates/hir-def/src/body.rs +++ b/crates/hir-def/src/body.rs @@ -18,6 +18,7 @@ use smallvec::SmallVec; use span::{Edition, MacroFileId}; use syntax::{ast, AstPtr, SyntaxNodePtr}; use triomphe::Arc; +use tt::TextRange; use crate::{ db::DefDatabase, @@ -143,15 +144,7 @@ pub struct BodySourceMap { pub types: TypesSourceMap, - // FIXME: Make this a sane struct. - template_map: Option< - Box<( - // format_args! - FxHashMap)>, - // asm! - FxHashMap>>, - )>, - >, + template_map: Option>, expansions: FxHashMap>, MacroFileId>, @@ -160,6 +153,20 @@ pub struct BodySourceMap { diagnostics: Vec, } +#[derive(Default, Debug, Eq, PartialEq)] +struct FormatTemplate { + /// A map from `format_args!()` expressions to their captures. + format_args_to_captures: FxHashMap)>, + /// A map from `asm!()` expressions to their captures. + asm_to_captures: FxHashMap>>, + /// A map from desugared expressions of implicit captures to their source. + /// + /// The value stored for each capture is its template literal and offset inside it. The template literal + /// is from the `format_args[_nl]!()` macro and so needs to be mapped up once to go to the user-written + /// template. + implicit_capture_to_source: FxHashMap, TextRange)>>, +} + #[derive(Debug, Eq, PartialEq)] pub enum BodyDiagnostic { InactiveCode { node: InFile, cfg: CfgExpr, opts: CfgOptions }, @@ -798,18 +805,29 @@ impl BodySourceMap { node: InFile<&ast::FormatArgsExpr>, ) -> Option<(HygieneId, &[(syntax::TextRange, Name)])> { let src = node.map(AstPtr::new).map(AstPtr::upcast::); - let (hygiene, names) = - self.template_map.as_ref()?.0.get(&self.expr_map.get(&src)?.as_expr()?)?; + let (hygiene, names) = self + .template_map + .as_ref()? + .format_args_to_captures + .get(&self.expr_map.get(&src)?.as_expr()?)?; Some((*hygiene, &**names)) } + pub fn format_args_implicit_capture( + &self, + capture_expr: ExprId, + ) -> Option, TextRange)>> { + self.template_map.as_ref()?.implicit_capture_to_source.get(&capture_expr).copied() + } + pub fn asm_template_args( &self, node: InFile<&ast::AsmExpr>, ) -> Option<(ExprId, &[Vec<(syntax::TextRange, usize)>])> { let src = node.map(AstPtr::new).map(AstPtr::upcast::); let expr = self.expr_map.get(&src)?.as_expr()?; - Some(expr).zip(self.template_map.as_ref()?.1.get(&expr).map(std::ops::Deref::deref)) + Some(expr) + .zip(self.template_map.as_ref()?.asm_to_captures.get(&expr).map(std::ops::Deref::deref)) } /// Get a reference to the body source map's diagnostics. @@ -835,8 +853,14 @@ impl BodySourceMap { types, } = self; if let Some(template_map) = template_map { - template_map.0.shrink_to_fit(); - template_map.1.shrink_to_fit(); + let FormatTemplate { + format_args_to_captures, + asm_to_captures, + implicit_capture_to_source, + } = &mut **template_map; + format_args_to_captures.shrink_to_fit(); + asm_to_captures.shrink_to_fit(); + implicit_capture_to_source.shrink_to_fit(); } expr_map.shrink_to_fit(); expr_map_back.shrink_to_fit(); diff --git a/crates/hir-def/src/body/lower.rs b/crates/hir-def/src/body/lower.rs index 3b73d40963..eed9f9468f 100644 --- a/crates/hir-def/src/body/lower.rs +++ b/crates/hir-def/src/body/lower.rs @@ -1957,8 +1957,10 @@ impl ExprCollector<'_> { _ => None, }); let mut mappings = vec![]; - let (fmt, hygiene) = match template.and_then(|it| self.expand_macros_to_string(it)) { - Some((s, is_direct_literal)) => { + let (fmt, hygiene) = match template.and_then(|template| { + self.expand_macros_to_string(template.clone()).map(|it| (it, template)) + }) { + Some(((s, is_direct_literal), template)) => { let call_ctx = self.expander.syntax_context(); let hygiene = self.hygiene_id_for(s.syntax().text_range().start()); let fmt = format_args::parse( @@ -1966,8 +1968,18 @@ impl ExprCollector<'_> { fmt_snippet, args, is_direct_literal, - |name| { + |name, range| { let expr_id = self.alloc_expr_desugared(Expr::Path(Path::from(name))); + if let Some(range) = range { + self.source_map + .template_map + .get_or_insert_with(Default::default) + .implicit_capture_to_source + .insert( + expr_id, + self.expander.in_file((AstPtr::new(&template), range)), + ); + } if !hygiene.is_root() { self.body.expr_hygiene.insert(expr_id, hygiene); } @@ -2139,7 +2151,7 @@ impl ExprCollector<'_> { self.source_map .template_map .get_or_insert_with(Default::default) - .0 + .format_args_to_captures .insert(idx, (hygiene, mappings)); idx } diff --git a/crates/hir-def/src/body/lower/asm.rs b/crates/hir-def/src/body/lower/asm.rs index c1b58dbdd0..68c7173d1e 100644 --- a/crates/hir-def/src/body/lower/asm.rs +++ b/crates/hir-def/src/body/lower/asm.rs @@ -6,7 +6,7 @@ use syntax::{ ast::{self, HasName, IsString}, AstNode, AstPtr, AstToken, T, }; -use tt::{TextRange, TextSize}; +use tt::TextRange; use crate::{ body::lower::{ExprCollector, FxIndexSet}, @@ -224,7 +224,7 @@ impl ExprCollector<'_> { TextRange::new( inner_span.start.try_into().unwrap(), inner_span.end.try_into().unwrap(), - ) - TextSize::from(str_style.map(|it| it + 1).unwrap_or(0) as u32 + 1) + ) }) }; for piece in unverified_pieces { @@ -268,7 +268,11 @@ impl ExprCollector<'_> { Expr::InlineAsm(InlineAsm { operands: operands.into_boxed_slice(), options }), syntax_ptr, ); - self.source_map.template_map.get_or_insert_with(Default::default).1.insert(idx, mappings); + self.source_map + .template_map + .get_or_insert_with(Default::default) + .asm_to_captures + .insert(idx, mappings); idx } } diff --git a/crates/hir-def/src/hir/format_args.rs b/crates/hir-def/src/hir/format_args.rs index e1c3bd25bc..e64e498c17 100644 --- a/crates/hir-def/src/hir/format_args.rs +++ b/crates/hir-def/src/hir/format_args.rs @@ -1,5 +1,6 @@ //! Parses `format_args` input. +use either::Either; use hir_expand::name::Name; use intern::Symbol; use rustc_parse_format as parse; @@ -7,7 +8,7 @@ use span::SyntaxContextId; use stdx::TupleExt; use syntax::{ ast::{self, IsString}, - TextRange, TextSize, + TextRange, }; use crate::hir::ExprId; @@ -33,7 +34,7 @@ pub enum FormatArgsPiece { Placeholder(FormatPlaceholder), } -#[derive(Copy, Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct FormatPlaceholder { /// Index into [`FormatArgs::arguments`]. pub argument: FormatArgPosition, @@ -45,11 +46,11 @@ pub struct FormatPlaceholder { pub format_options: FormatOptions, } -#[derive(Copy, Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct FormatArgPosition { /// Which argument this position refers to (Ok), /// or would've referred to if it existed (Err). - pub index: Result, + pub index: Result>, /// What kind of position this is. See [`FormatArgPositionKind`]. pub kind: FormatArgPositionKind, /// The span of the name or number. @@ -88,7 +89,7 @@ pub enum FormatTrait { UpperHex, } -#[derive(Copy, Clone, Default, Debug, PartialEq, Eq)] +#[derive(Clone, Default, Debug, PartialEq, Eq)] pub struct FormatOptions { /// The width. E.g. `{:5}` or `{:width$}`. pub width: Option, @@ -133,7 +134,7 @@ pub enum FormatAlignment { Center, } -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub enum FormatCount { /// `{:5}` or `{:.5}` Literal(usize), @@ -173,7 +174,7 @@ pub(crate) fn parse( fmt_snippet: Option, mut args: FormatArgumentsCollector, is_direct_literal: bool, - mut synth: impl FnMut(Name) -> ExprId, + mut synth: impl FnMut(Name, Option) -> ExprId, mut record_usage: impl FnMut(Name, Option), call_ctx: SyntaxContextId, ) -> FormatArgs { @@ -192,7 +193,6 @@ pub(crate) fn parse( } None => None, }; - let mut parser = parse::Parser::new(&text, str_style, fmt_snippet, false, parse::ParseMode::Format); @@ -217,7 +217,6 @@ pub(crate) fn parse( let to_span = |inner_span: parse::InnerSpan| { is_source_literal.then(|| { TextRange::new(inner_span.start.try_into().unwrap(), inner_span.end.try_into().unwrap()) - - TextSize::from(str_style.map(|it| it + 1).unwrap_or(0) as u32 + 1) }) }; @@ -245,8 +244,8 @@ pub(crate) fn parse( Ok(index) } else { // Doesn't exist as an explicit argument. - invalid_refs.push((index, span, used_as, kind)); - Err(index) + invalid_refs.push((Either::Left(index), span, used_as, kind)); + Err(Either::Left(index)) } } ArgRef::Name(name, span) => { @@ -265,14 +264,17 @@ pub(crate) fn parse( // For the moment capturing variables from format strings expanded from macros is // disabled (see RFC #2795) // FIXME: Diagnose + invalid_refs.push((Either::Right(name.clone()), span, used_as, kind)); + Err(Either::Right(name)) + } else { + record_usage(name.clone(), span); + Ok(args.add(FormatArgument { + kind: FormatArgumentKind::Captured(name.clone()), + // FIXME: This is problematic, we might want to synthesize a dummy + // expression proper and/or desugar these. + expr: synth(name, span), + })) } - record_usage(name.clone(), span); - Ok(args.add(FormatArgument { - kind: FormatArgumentKind::Captured(name.clone()), - // FIXME: This is problematic, we might want to synthesize a dummy - // expression proper and/or desugar these. - expr: synth(name), - })) } } }; diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 612c6adb20..cbb1ed95ed 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -262,7 +262,7 @@ pub struct UnresolvedAssocItem { #[derive(Debug)] pub struct UnresolvedIdent { - pub expr_or_pat: InFile>>, + pub node: InFile<(AstPtr>, Option)>, } #[derive(Debug)] @@ -550,11 +550,10 @@ impl AnyDiagnostic { source_map: &hir_def::body::BodySourceMap, ) -> Option { let expr_syntax = |expr| { - source_map.expr_syntax(expr).inspect_err(|_| tracing::error!("synthetic syntax")).ok() - }; - let pat_syntax = |pat| { - source_map.pat_syntax(pat).inspect_err(|_| tracing::error!("synthetic syntax")).ok() + source_map.expr_syntax(expr).inspect_err(|_| stdx::never!("synthetic syntax")).ok() }; + let pat_syntax = + |pat| source_map.pat_syntax(pat).inspect_err(|_| stdx::never!("synthetic syntax")).ok(); let expr_or_pat_syntax = |id| match id { ExprOrPatId::ExprId(expr) => expr_syntax(expr).map(|it| it.map(AstPtr::wrap_left)), ExprOrPatId::PatId(pat) => pat_syntax(pat), @@ -626,8 +625,16 @@ impl AnyDiagnostic { UnresolvedAssocItem { expr_or_pat }.into() } &InferenceDiagnostic::UnresolvedIdent { id } => { - let expr_or_pat = expr_or_pat_syntax(id)?; - UnresolvedIdent { expr_or_pat }.into() + let node = match id { + ExprOrPatId::ExprId(id) => match source_map.expr_syntax(id) { + Ok(syntax) => syntax.map(|it| (it.wrap_left(), None)), + Err(SyntheticSyntax) => source_map + .format_args_implicit_capture(id)? + .map(|(node, range)| (node.wrap_left(), Some(range))), + }, + ExprOrPatId::PatId(id) => pat_syntax(id)?.map(|it| (it, None)), + }; + UnresolvedIdent { node }.into() } &InferenceDiagnostic::BreakOutsideOfLoop { expr, is_break, bad_value_break } => { let expr = expr_syntax(expr)?; diff --git a/crates/hir/src/semantics.rs b/crates/hir/src/semantics.rs index b896cda9dd..f030c1862a 100644 --- a/crates/hir/src/semantics.rs +++ b/crates/hir/src/semantics.rs @@ -38,7 +38,7 @@ use span::{AstIdMap, EditionedFileId, FileId, HirFileIdRepr, SyntaxContextId}; use stdx::TupleExt; use syntax::{ algo::skip_trivia_token, - ast::{self, HasAttrs as _, HasGenericParams, IsString as _}, + ast::{self, HasAttrs as _, HasGenericParams}, AstNode, AstToken, Direction, SyntaxKind, SyntaxNode, SyntaxNodePtr, SyntaxToken, TextRange, TextSize, }; @@ -643,8 +643,7 @@ impl<'db> SemanticsImpl<'db> { &self, string: &ast::String, ) -> Option>)>> { - let quote = string.open_quote_text_range()?; - + let string_start = string.syntax().text_range().start(); let token = self.wrap_token_infile(string.syntax().clone()).into_real_file().ok()?; self.descend_into_macros_breakable(token, |token, _| { (|| { @@ -658,7 +657,7 @@ impl<'db> SemanticsImpl<'db> { let format_args = self.wrap_node_infile(format_args); let res = source_analyzer .as_format_args_parts(self.db, format_args.as_ref())? - .map(|(range, res)| (range + quote.end(), res.map(Either::Left))) + .map(|(range, res)| (range + string_start, res.map(Either::Left))) .collect(); Some(res) } else { @@ -672,7 +671,7 @@ impl<'db> SemanticsImpl<'db> { .iter() .map(|&(range, index)| { ( - range + quote.end(), + range + string_start, Some(Either::Right(InlineAsmOperand { owner, expr, index })), ) }) @@ -690,17 +689,16 @@ impl<'db> SemanticsImpl<'db> { original_token: SyntaxToken, offset: TextSize, ) -> Option<(TextRange, Option>)> { - let original_string = ast::String::cast(original_token.clone())?; + let string_start = original_token.text_range().start(); let original_token = self.wrap_token_infile(original_token).into_real_file().ok()?; - let quote = original_string.open_quote_text_range()?; self.descend_into_macros_breakable(original_token, |token, _| { (|| { let token = token.value; self.resolve_offset_in_format_args( ast::String::cast(token)?, - offset.checked_sub(quote.end())?, + offset.checked_sub(string_start)?, ) - .map(|(range, res)| (range + quote.end(), res)) + .map(|(range, res)| (range + string_start, res)) })() .map_or(ControlFlow::Continue(()), ControlFlow::Break) }) diff --git a/crates/ide-diagnostics/src/handlers/unresolved_ident.rs b/crates/ide-diagnostics/src/handlers/unresolved_ident.rs index 68f14a97f5..4f64dabeb5 100644 --- a/crates/ide-diagnostics/src/handlers/unresolved_ident.rs +++ b/crates/ide-diagnostics/src/handlers/unresolved_ident.rs @@ -7,20 +7,19 @@ pub(crate) fn unresolved_ident( ctx: &DiagnosticsContext<'_>, d: &hir::UnresolvedIdent, ) -> Diagnostic { - Diagnostic::new_with_syntax_node_ptr( - ctx, - DiagnosticCode::RustcHardError("E0425"), - "no such value in this scope", - d.expr_or_pat.map(Into::into), - ) - .experimental() + let mut range = + ctx.sema.diagnostics_display_range(d.node.map(|(node, _)| node.syntax_node_ptr())); + if let Some(in_node_range) = d.node.value.1 { + range.range = in_node_range + range.range.start(); + } + Diagnostic::new(DiagnosticCode::RustcHardError("E0425"), "no such value in this scope", range) + .experimental() } #[cfg(test)] mod tests { use crate::tests::check_diagnostics; - // FIXME: This should show a diagnostic #[test] fn feature() { check_diagnostics( @@ -28,6 +27,7 @@ mod tests { //- minicore: fmt fn main() { format_args!("{unresolved}"); + // ^^^^^^^^^^ error: no such value in this scope } "#, )