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

Properly record metavar spans for other expansions other than TT #134478

Merged
merged 3 commits into from
Jan 22, 2025
Merged
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
20 changes: 10 additions & 10 deletions compiler/rustc_expand/src/mbe/transcribe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,13 @@ pub(super) fn transcribe<'a>(
}
MatchedSingle(ParseNtResult::Ident(ident, is_raw)) => {
marker.visit_span(&mut sp);
with_metavar_spans(|mspans| mspans.insert(ident.span, sp));
let kind = token::NtIdent(*ident, *is_raw);
TokenTree::token_alone(kind, sp)
}
MatchedSingle(ParseNtResult::Lifetime(ident, is_raw)) => {
marker.visit_span(&mut sp);
with_metavar_spans(|mspans| mspans.insert(ident.span, sp));
let kind = token::NtLifetime(*ident, *is_raw);
TokenTree::token_alone(kind, sp)
}
Expand All @@ -295,6 +297,8 @@ pub(super) fn transcribe<'a>(
// `Delimiter::Invisible` to maintain parsing priorities.
// `Interpolated` is currently used for such groups in rustc parser.
marker.visit_span(&mut sp);
let use_span = nt.use_span();
with_metavar_spans(|mspans| mspans.insert(use_span, sp));
TokenTree::token_alone(token::Interpolated(Lrc::clone(nt)), sp)
}
MatchedSeq(..) => {
Expand Down Expand Up @@ -410,19 +414,15 @@ fn maybe_use_metavar_location(
return orig_tt.clone();
}

let insert = |mspans: &mut FxHashMap<_, _>, s, ms| match mspans.try_insert(s, ms) {
Ok(_) => true,
Err(err) => *err.entry.get() == ms, // Tried to insert the same span, still success
};
marker.visit_span(&mut metavar_span);
let no_collision = match orig_tt {
TokenTree::Token(token, ..) => {
with_metavar_spans(|mspans| insert(mspans, token.span, metavar_span))
with_metavar_spans(|mspans| mspans.insert(token.span, metavar_span))
}
TokenTree::Delimited(dspan, ..) => with_metavar_spans(|mspans| {
insert(mspans, dspan.open, metavar_span)
&& insert(mspans, dspan.close, metavar_span)
&& insert(mspans, dspan.entire(), metavar_span)
mspans.insert(dspan.open, metavar_span)
&& mspans.insert(dspan.close, metavar_span)
&& mspans.insert(dspan.entire(), metavar_span)
Comment on lines -423 to +425
Copy link
Contributor

Choose a reason for hiding this comment

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

Orthogonal to this PR, but It's not clear to me that it's good keeping the previously inserted spans in the map if one of the later ones in this chain fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

The more spans the better, so the best logic here would be

let b1 = mspans.insert(dspan.open, metavar_span);
let b2 = mspans.insert(dspan.close, metavar_span);
let b3 = mspans.insert(dspan.entire(), metavar_span);
b1 && b2 && b3

But it probably doesn't matter anyway, because b{1,2,3} always (or almost always) have the same value in practice, and the metavar span retrieval is best effort anyway.

}),
};
if no_collision || psess.source_map().is_imported(metavar_span) {
Expand All @@ -434,14 +434,14 @@ fn maybe_use_metavar_location(
match orig_tt {
TokenTree::Token(Token { kind, span }, spacing) => {
let span = metavar_span.with_ctxt(span.ctxt());
with_metavar_spans(|mspans| insert(mspans, span, metavar_span));
with_metavar_spans(|mspans| mspans.insert(span, metavar_span));
TokenTree::Token(Token { kind: kind.clone(), span }, *spacing)
}
TokenTree::Delimited(dspan, dspacing, delimiter, tts) => {
let open = metavar_span.with_ctxt(dspan.open.ctxt());
let close = metavar_span.with_ctxt(dspan.close.ctxt());
with_metavar_spans(|mspans| {
insert(mspans, open, metavar_span) && insert(mspans, close, metavar_span)
mspans.insert(open, metavar_span) && mspans.insert(close, metavar_span)
});
let dspan = DelimSpan::from_pair(open, close);
TokenTree::Delimited(dspan, *dspacing, *delimiter, tts.clone())
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_middle/src/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_hir::*;
use rustc_hir_pretty as pprust_hir;
use rustc_middle::hir::nested_filter;
use rustc_span::def_id::StableCrateId;
use rustc_span::{ErrorGuaranteed, Ident, Span, Symbol, kw, sym};
use rustc_span::{ErrorGuaranteed, Ident, Span, Symbol, kw, sym, with_metavar_spans};

use crate::hir::ModuleItems;
use crate::middle::debugger_visualizer::DebuggerVisualizerFile;
Expand Down Expand Up @@ -1116,6 +1116,9 @@ pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh {
// the fly in the resolver, storing only their accumulated hash in `ResolverGlobalCtxt`,
// and combining it with other hashes here.
resolutions.visibilities_for_hashing.hash_stable(&mut hcx, &mut stable_hasher);
with_metavar_spans(|mspans| {
mspans.freeze_and_get_read_spans().hash_stable(&mut hcx, &mut stable_hasher);
});
stable_hasher.finish()
});

Expand Down
8 changes: 2 additions & 6 deletions compiler/rustc_parse/src/validate_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use rustc_session::errors::report_lit_error;
use rustc_session::lint::BuiltinLintDiag;
use rustc_session::lint::builtin::{ILL_FORMED_ATTRIBUTE_INPUT, UNSAFE_ATTR_OUTSIDE_UNSAFE};
use rustc_session::parse::ParseSess;
use rustc_span::{BytePos, Span, Symbol, sym};
use rustc_span::{Span, Symbol, sym};

use crate::{errors, parse_in};

Expand Down Expand Up @@ -164,11 +164,7 @@ pub fn check_attribute_safety(psess: &ParseSess, safety: AttributeSafety, attr:
// wrapping it in `unsafe(...)`. Otherwise, we suggest putting the
// `unsafe(`, `)` right after and right before the opening and closing
// square bracket respectively.
let diag_span = if attr_item.span().can_be_used_for_suggestions() {
attr_item.span()
} else {
attr.span.with_lo(attr.span.lo() + BytePos(2)).with_hi(attr.span.hi() - BytePos(1))
};
let diag_span = attr_item.span();

if attr.span.at_least_rust_2024() {
psess.dcx().emit_err(errors::UnsafeAttrOutsideUnsafe {
Expand Down
45 changes: 39 additions & 6 deletions compiler/rustc_span/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#![feature(hash_set_entry)]
#![feature(if_let_guard)]
#![feature(let_chains)]
#![feature(map_try_insert)]
#![feature(negative_impls)]
#![feature(read_buf)]
#![feature(round_char_boundary)]
Expand Down Expand Up @@ -85,9 +86,9 @@ use std::str::FromStr;
use std::{fmt, iter};

use md5::{Digest, Md5};
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{Hash64, Hash128, HashStable, StableHasher};
use rustc_data_structures::sync::{FreezeLock, FreezeWriteGuard, Lock, Lrc};
use rustc_data_structures::unord::UnordMap;
use sha1::Sha1;
use sha2::Sha256;

Expand All @@ -103,7 +104,7 @@ pub struct SessionGlobals {
span_interner: Lock<span_encoding::SpanInterner>,
/// Maps a macro argument token into use of the corresponding metavariable in the macro body.
/// Collisions are possible and processed in `maybe_use_metavar_location` on best effort basis.
metavar_spans: Lock<FxHashMap<Span, Span>>,
metavar_spans: MetavarSpansMap,
hygiene_data: Lock<hygiene::HygieneData>,

/// The session's source map, if there is one. This field should only be
Expand Down Expand Up @@ -177,9 +178,42 @@ pub fn create_default_session_globals_then<R>(f: impl FnOnce() -> R) -> R {
// deserialization.
scoped_tls::scoped_thread_local!(static SESSION_GLOBALS: SessionGlobals);

#[derive(Default)]
pub struct MetavarSpansMap(FreezeLock<UnordMap<Span, (Span, bool)>>);

impl MetavarSpansMap {
pub fn insert(&self, span: Span, var_span: Span) -> bool {
match self.0.write().try_insert(span, (var_span, false)) {
Ok(_) => true,
Err(entry) => entry.entry.get().0 == var_span,
}
}

/// Read a span and record that it was read.
pub fn get(&self, span: Span) -> Option<Span> {
if let Some(mut mspans) = self.0.try_write() {
if let Some((var_span, read)) = mspans.get_mut(&span) {
*read = true;
Some(*var_span)
} else {
None
}
} else {
if let Some((span, true)) = self.0.read().get(&span) { Some(*span) } else { None }
}
}

/// Freeze the set, and return the spans which have been read.
///
/// After this is frozen, no spans that have not been read can be read.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// After this is frozen, no spans that have not been read can be read.
/// After this is frozen, only spans that have already been read can be read.

pub fn freeze_and_get_read_spans(&self) -> UnordMap<Span, Span> {
self.0.freeze().items().filter(|(_, (_, b))| *b).map(|(s1, (s2, _))| (*s1, *s2)).collect()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we ever use this twice? if not, could also poison the lock after extracting the spans we care about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Poison the lock on the whole map?
freeze_and_get_read_spans is called once, but we still need to read the map after that, cannot panic on those later reads.

}
}

#[inline]
pub fn with_metavar_spans<R>(f: impl FnOnce(&mut FxHashMap<Span, Span>) -> R) -> R {
with_session_globals(|session_globals| f(&mut session_globals.metavar_spans.lock()))
pub fn with_metavar_spans<R>(f: impl FnOnce(&MetavarSpansMap) -> R) -> R {
with_session_globals(|session_globals| f(&session_globals.metavar_spans))
}

// FIXME: We should use this enum or something like it to get rid of the
Expand Down Expand Up @@ -872,8 +906,7 @@ impl Span {

/// Check if you can select metavar spans for the given spans to get matching contexts.
fn try_metavars(a: SpanData, b: SpanData, a_orig: Span, b_orig: Span) -> (SpanData, SpanData) {
let get = |mspans: &FxHashMap<_, _>, s| mspans.get(&s).copied();
match with_metavar_spans(|mspans| (get(mspans, a_orig), get(mspans, b_orig))) {
match with_metavar_spans(|mspans| (mspans.get(a_orig), mspans.get(b_orig))) {
(None, None) => {}
(Some(meta_a), None) => {
let meta_a = meta_a.data();
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/drop/lint-if-let-rescope-with-macro.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: `if let` assigns a shorter lifetime since Edition 2024
--> $DIR/lint-if-let-rescope-with-macro.rs:12:12
|
LL | if let $p = $e { $($conseq)* } else { $($alt)* }
| ^^^
| ^^^^^^^^^^^
...
LL | / edition_2021_if_let! {
LL | | Some(_value),
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/expr/if/if-let.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ warning: irrefutable `if let` pattern
--> $DIR/if-let.rs:6:16
|
LL | if let $p = $e $b
| ^^^
| ^^^^^^^^^^^
...
LL | / foo!(a, 1, {
LL | | println!("irrefutable pattern");
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/for-loop-while/while-let-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ warning: irrefutable `while let` pattern
--> $DIR/while-let-2.rs:7:19
|
LL | while let $p = $e $b
| ^^^
| ^^^^^^^^^^^
...
LL | / foo!(_a, 1, {
LL | | println!("irrefutable pattern");
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/lint/wide_pointer_comparisons.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ warning: ambiguous wide pointer comparison, the comparison includes metadata whi
--> $DIR/wide_pointer_comparisons.rs:169:37
|
LL | ($a:expr, $b:expr) => { $a == $b }
| ^^
| ^^^^^^^^
...
LL | cmp!(&a, &b);
| ------------ in this macro invocation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ error: `mut` must be followed by a named binding
--> $DIR/issue-65122-mac-invoc-in-mut-patterns.rs:13:13
|
LL | let mut $eval = ();
| ^^^
| ^^^^
...
LL | mac2! { does_not_exist!() }
| --------------------------- in this macro invocation
Expand All @@ -40,7 +40,7 @@ LL | mac2! { does_not_exist!() }
help: remove the `mut` prefix
|
LL - let mut $eval = ();
LL + let $eval = ();
LL + let $eval = ();
|

error: cannot find macro `does_not_exist` in this scope
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ macro_rules! meta2 {
}
}

macro_rules! with_cfg_attr {
() => {
#[cfg_attr(all(), unsafe(link_section = ".custom_section"))]
//~^ ERROR: unsafe attribute used without unsafe
//~| WARN this is accepted in the current edition
pub extern "C" fn abc() {}
};
}

tt!([unsafe(no_mangle)]);
//~^ ERROR: unsafe attribute used without unsafe
//~| WARN this is accepted in the current edition
Expand All @@ -52,6 +61,8 @@ meta2!(unsafe(export_name = "baw"));
//~| WARN this is accepted in the current edition
ident2!(export_name, "bars");

with_cfg_attr!();

#[unsafe(no_mangle)]
//~^ ERROR: unsafe attribute used without unsafe
//~| WARN this is accepted in the current edition
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ macro_rules! meta2 {
}
}

macro_rules! with_cfg_attr {
() => {
#[cfg_attr(all(), link_section = ".custom_section")]
//~^ ERROR: unsafe attribute used without unsafe
//~| WARN this is accepted in the current edition
pub extern "C" fn abc() {}
};
}

tt!([no_mangle]);
//~^ ERROR: unsafe attribute used without unsafe
//~| WARN this is accepted in the current edition
Expand All @@ -52,6 +61,8 @@ meta2!(export_name = "baw");
//~| WARN this is accepted in the current edition
ident2!(export_name, "bars");

with_cfg_attr!();

#[no_mangle]
//~^ ERROR: unsafe attribute used without unsafe
//~| WARN this is accepted in the current edition
Expand Down
27 changes: 22 additions & 5 deletions tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: unsafe attribute used without unsafe
--> $DIR/unsafe-attributes-fix.rs:43:6
--> $DIR/unsafe-attributes-fix.rs:52:6
|
LL | tt!([no_mangle]);
| ^^^^^^^^^ usage of unsafe attribute
Expand Down Expand Up @@ -34,7 +34,7 @@ LL | #[unsafe($e)]
| +++++++ +

error: unsafe attribute used without unsafe
--> $DIR/unsafe-attributes-fix.rs:47:7
--> $DIR/unsafe-attributes-fix.rs:56:7
|
LL | meta!(no_mangle);
| ^^^^^^^^^ usage of unsafe attribute
Expand All @@ -47,7 +47,7 @@ LL | meta!(unsafe(no_mangle));
| +++++++ +

error: unsafe attribute used without unsafe
--> $DIR/unsafe-attributes-fix.rs:50:8
--> $DIR/unsafe-attributes-fix.rs:59:8
|
LL | meta2!(export_name = "baw");
| ^^^^^^^^^^^ usage of unsafe attribute
Expand Down Expand Up @@ -77,7 +77,24 @@ LL | #[unsafe($e = $l)]
| +++++++ +

error: unsafe attribute used without unsafe
--> $DIR/unsafe-attributes-fix.rs:55:3
--> $DIR/unsafe-attributes-fix.rs:45:27
|
LL | #[cfg_attr(all(), link_section = ".custom_section")]
| ^^^^^^^^^^^^ usage of unsafe attribute
...
LL | with_cfg_attr!();
| ---------------- in this macro invocation
|
= warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2024!
= note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/unsafe-attributes.html>
= note: this error originates in the macro `with_cfg_attr` (in Nightly builds, run with -Z macro-backtrace for more info)
help: wrap the attribute in `unsafe(...)`
|
LL | #[cfg_attr(all(), unsafe(link_section = ".custom_section"))]
| +++++++ +

error: unsafe attribute used without unsafe
--> $DIR/unsafe-attributes-fix.rs:66:3
|
LL | #[no_mangle]
| ^^^^^^^^^ usage of unsafe attribute
Expand All @@ -89,5 +106,5 @@ help: wrap the attribute in `unsafe(...)`
LL | #[unsafe(no_mangle)]
| +++++++ +

error: aborting due to 6 previous errors
error: aborting due to 7 previous errors

Loading