From a560810a69a09452b4cd0c3173b6af98496dba35 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 10:54:56 +1000 Subject: [PATCH 1/4] Don't include inner attribute ranges in `CaptureState`. The current code is this: ``` self.capture_state.replace_ranges.push((start_pos..end_pos, Some(target))); self.capture_state.replace_ranges.extend(inner_attr_replace_ranges); ``` What's not obvious is that every range in `inner_attr_replace_ranges` must be a strict sub-range of `start_pos..end_pos`. Which means, in `LazyAttrTokenStreamImpl::to_attr_token_stream`, they will be done first, and then the `start_pos..end_pos` replacement will just overwrite them. So they aren't needed. --- compiler/rustc_parse/src/parser/attr_wrapper.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index dc5f98f7be8b6..1fd0654e0aba9 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -337,8 +337,7 @@ impl<'a> Parser<'a> { // When parsing `m`: // - `start_pos..end_pos` is `0..34` (`mod m`, excluding the `#[cfg_eval]` attribute). // - `inner_attr_replace_ranges` is empty. - // - `replace_range_start..replace_ranges_end` has two entries. - // - One to delete the inner attribute (`17..27`), obtained when parsing `g` (see above). + // - `replace_range_start..replace_ranges_end` has one entry. // - One `AttrsTarget` (added below when parsing `g`) to replace all of `g` (`3..33`, // including its outer attribute), with: // - `attrs`: includes the outer and the inner attr. @@ -369,12 +368,10 @@ impl<'a> Parser<'a> { // What is the status here when parsing the example code at the top of this method? // - // When parsing `g`, we add two entries: + // When parsing `g`, we add one entry: // - The `start_pos..end_pos` (`3..33`) entry has a new `AttrsTarget` with: // - `attrs`: includes the outer and the inner attr. // - `tokens`: lazy tokens for `g` (with its inner attr deleted). - // - `inner_attr_replace_ranges` contains the one entry to delete the inner attr's - // tokens (`17..27`). // // When parsing `m`, we do nothing here. @@ -384,7 +381,6 @@ impl<'a> Parser<'a> { let start_pos = if has_outer_attrs { attrs.start_pos } else { start_pos }; let target = AttrsTarget { attrs: ret.attrs().iter().cloned().collect(), tokens }; self.capture_state.replace_ranges.push((start_pos..end_pos, Some(target))); - self.capture_state.replace_ranges.extend(inner_attr_replace_ranges); } else if matches!(self.capture_state.capturing, Capturing::No) { // Only clear the ranges once we've finished capturing entirely, i.e. we've finished // the outermost call to this method. From 6e87858f26bcb9fadbd60f9e8ee24816a55faa80 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 14:02:15 +1000 Subject: [PATCH 2/4] Fix a comment. Imagine you have replace ranges (2..20,X) and (5..15,Y), and these tokens: ``` a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x ``` If we replace (5..15,Y) first, then (2..20,X) we get this sequence ``` a,b,c,d,e,Y,_,_,_,_,_,_,_,_,_,p,q,r,s,t,u,v,w,x a,b,X,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,u,v,w,x ``` which is what we want. If we do it in the other order, we get this: ``` a,b,X,_,_,_,_,_,_,_,_,_,_,_,_,p,q,r,s,t,u,v,w,x a,b,X,_,_,Y,_,_,_,_,_,_,_,_,_,_,_,_,_,_,u,v,w,x ``` which is wrong. So it's true that we need the `.rev()` but the comment is wrong about why. --- compiler/rustc_parse/src/parser/attr_wrapper.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index 1fd0654e0aba9..a90bfb574e2ee 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -137,9 +137,9 @@ impl ToAttrTokenStream for LazyAttrTokenStreamImpl { // `#[cfg(FALSE)] struct Foo { #[cfg(FALSE)] field: bool }` // // By starting processing from the replace range with the greatest - // start position, we ensure that any replace range which encloses - // another replace range will capture the *replaced* tokens for the inner - // range, not the original tokens. + // start position, we ensure that any (outer) replace range which + // encloses another (inner) replace range will fully overwrite the + // inner range's replacement. for (range, target) in replace_ranges.into_iter().rev() { assert!(!range.is_empty(), "Cannot replace an empty range: {range:?}"); From 6ea2da5a28580dba214b32831d0a6952e2ebce91 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 13:20:27 +1000 Subject: [PATCH 3/4] Tweak a loop. A fully imperative style is easier to read than a half-iterator, half-imperative style. Also, rename `inner_attr` as `attr` because it might be an outer attribute. --- compiler/rustc_parse/src/parser/attr_wrapper.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index a90bfb574e2ee..abee668cc6cb0 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -297,11 +297,13 @@ impl<'a> Parser<'a> { // with `None`, which means the relevant tokens will be removed. (More // details below.) let mut inner_attr_replace_ranges = Vec::new(); - for inner_attr in ret.attrs().iter().filter(|a| a.style == ast::AttrStyle::Inner) { - if let Some(attr_range) = self.capture_state.inner_attr_ranges.remove(&inner_attr.id) { - inner_attr_replace_ranges.push((attr_range, None)); - } else { - self.dcx().span_delayed_bug(inner_attr.span, "Missing token range for attribute"); + for attr in ret.attrs() { + if attr.style == ast::AttrStyle::Inner { + if let Some(attr_range) = self.capture_state.inner_attr_ranges.remove(&attr.id) { + inner_attr_replace_ranges.push((attr_range, None)); + } else { + self.dcx().span_delayed_bug(attr.span, "Missing token range for attribute"); + } } } From 55d37ae711b1a1ab82ef02d0594f92167e18af90 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 14:00:29 +1000 Subject: [PATCH 4/4] Remove an unnecessary block. --- .../rustc_parse/src/parser/attr_wrapper.rs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index abee668cc6cb0..389e4a6267d2e 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -114,17 +114,15 @@ impl ToAttrTokenStream for LazyAttrTokenStreamImpl { replace_ranges.sort_by_key(|(range, _)| range.start); #[cfg(debug_assertions)] - { - for [(range, tokens), (next_range, next_tokens)] in replace_ranges.array_windows() { - assert!( - range.end <= next_range.start || range.end >= next_range.end, - "Replace ranges should either be disjoint or nested: ({:?}, {:?}) ({:?}, {:?})", - range, - tokens, - next_range, - next_tokens, - ); - } + for [(range, tokens), (next_range, next_tokens)] in replace_ranges.array_windows() { + assert!( + range.end <= next_range.start || range.end >= next_range.end, + "Replace ranges should either be disjoint or nested: ({:?}, {:?}) ({:?}, {:?})", + range, + tokens, + next_range, + next_tokens, + ); } // Process the replace ranges, starting from the highest start