Fix trace! args overflow

[samtay]

Fixes #5987

[ytmimi]

@samtay thanks for jumping in and working on this one! I don't think we can move forward with the PR as is given that this would constitute a breaking formatting change for how we handle trace!. We'd need to add a version gate for this fix.

from what I can tell we can grab a reference to the &Config via the overflow::Contexts context field, which is a RewriteContext.

We'd need to pass the &Config to maybe_get_args_offset so it can pass the config to special_cases.

[ytmimi]

I ran our Diff-Check Job and it failed indicating that we need to version gate these changes.

[samtay]

@ytmimi makes sense! Done in af4929c. I just did the simple thing which was to return the special cases as a Vec, now that the cases are dynamic. If that is undesirable here, another option would be to have an additional static slice for v2 and chain the iterators - something like this.

[ytmimi]

@samtay thanks! I was going to suggest using chain. Could you make that change when you get a chance. Also, I think a doc comment on special_cases_v2 explaining that these are special cases that would cause breaking formatting changes for v1 would be helpful. Maybe we should define a SPECIAL_CASE_MACROS_V2 list so in the future we don't need to modify special_cases_v2 and can instead just add to that list.

[ytmimi]

@samtay thanks for updating the code! Just running an additional job to verify that the version gated code doesn't produce the same breaking formatting changes.

[samtay]

Huh.. I see the diff check job failed 🤔 Can you help me understand why that is? Don't the tests that I added demonstrate that the new trace! formatting only occurs when using version 2?

[ytmimi]

@samtay that's because I forgot to pass Version=One to your version of rustfmt. That should show that the current master and your feature branch produce the same diff for the stable version of rustfmt. Here's the link to the new job

I suspect that the job failed only for projects that use Version=Two like miri for example. In which case the new trace! formatting kicked in and produced a diff. Anyway, explicitly passing Version=One should help us prove that these changes won't break stable formatting.

[ytmimi]

hmmm 🤔 thinking about it a little more, passing version=One is going to potentially lead to diffs in other places outside of trace! formatting.

I guess our Diff-Check job isn't well suited for checking against changes for unstable formatting, which seems reasonable that unstable formatting might break.

[ytmimi]

I think the second Diff-Check job failing is a false alarm. After re reviewing the output, all the diffs only affect how trace! is formatted, and they only affect rust-lang/rust, and miri, both of which are setting Version=Two.

Comparing the output of the two jobs for rust-bindgen, which doesn't set a rustfmt version so it automatically defaults to Version=One, shows that the version gate is working as expected.

Here's the first Diff-Check Job

running rustfmt (master) on rust-bindgen
running rustfmt (feature) on rust-bindgen
checking diff
Diff in /tmp/rust-bindgen-ZkDOAXGv/bindgen/codegen/struct_layout.rs at line 299:
 
         trace!(
             "need a tail padding field for {}: offset {} -> size {}",
-            comp_name,
-            self.latest_offset,
-            comp_layout.size
+            comp_name, self.latest_offset, comp_layout.size
         );
         let size = comp_layout.size - self.latest_offset;
         Some(self.padding_field(Layout::new(size, 0)))
Diff in /tmp/rust-bindgen-ZkDOAXGv/bindgen/ir/analysis/template_params.rs at line 293:
             trace!(
                 "      instantiation's argument {:?} is used if definition's \
                  parameter {:?} is used",
-                arg,
-                param
+                arg, param
             );
 
             if used_by_def.contains(&param.into()) {
Diff in /tmp/rust-bindgen-ZkDOAXGv/bindgen/ir/analysis/derive.rs at line 113:
         let id = id.into();
         trace!(
             "inserting {:?} can_derive<{}>={:?}",
-            id,
-            self.derive_trait,
-            can_derive
+            id, self.derive_trait, can_derive
         );
 
         if let CanDerive::Yes = can_derive {
Diff in /tmp/rust-bindgen-ZkDOAXGv/bindgen/ir/analysis/derive.rs at line 241:
                     trace!(
                         "    arrays of T for which we cannot derive {} \
                          also cannot derive {}",
-                        self.derive_trait,
-                        self.derive_trait
+                        self.derive_trait, self.derive_trait
                     );
                     return CanDerive::No;
                 }
Diff in /tmp/rust-bindgen-ZkDOAXGv/bindgen/ir/analysis/derive.rs at line [280](https://github.com/rust-lang/rustfmt/actions/runs/7305152074/job/19908359632#step:4:281):
                     trace!(
                         "    vectors of T for which we cannot derive {} \
                          also cannot derive {}",
-                        self.derive_trait,
-                        self.derive_trait
+                        self.derive_trait, self.derive_trait
                     );
                     return CanDerive::No;
                 }

vs the second Diff-Check job

running rustfmt (master) on rust-bindgen
running rustfmt (feature) on rust-bindgen
checking diff
no diff detected between rustfmt and the feture branch
removing tmp_dir /tmp/rust-bindgen-5v0FDg9s

[ytmimi]

@samtay thanks again for your help on this one!

[ytmimi]

Huh.. I see the diff check job failed 🤔 Can you help me understand why that is? Don't the tests that I added demonstrate that the new trace! formatting only occurs when using version 2?

Oh and to more directly answer your question, yes your tests were sufficient in showing that the version gate logic was working. We've semi-recently introduced this Diff-Check job into our PR workflow as an extra precautionary step to test the code changes out on real projects before merging. Anyway, like I said the failure was a false alarm and mostly a shortcoming of the current Diff-Check job.

[samtay]

@ytmimi gotcha, makes sense. Happy to help out and thanks for the guidance on this one!