Tidy should not check line lengths in tests

[Anthuang]

Tidy will not check line lengths in tests even without the // ignore-tidy-linelength annotations. This PR also removes all the annotations which are now unnecessary.

Closes: #77548

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Mark-Simulacrum]

This looks great, it looks like it might've accidentally deleted a file though? src/test/ui/raw-str.rs in particular.

[petrochenkov]

I responded to this suggestion previously in #75987 (comment).
I think applying tidy to test code has the same value as applying it to any other code.

If the rule is relaxed, the the relaxation should at least be applied only to lines with //~ XXX annotations.
To detect such annotations we have regex here - https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/runtest.rs#L3393-L3396.

[Mark-Simulacrum]

Note that we're just silencing the line length warning. That's silenced in most of our code anyway due to rustfmt "taking care of it". I am okay with limiting this to just the //~ lines though, that seems reasonable.

I think there's not too much value in truncating those to 100 characters (unlike regular comments/code).

cc @RalfJung

[jyn514]

Please also exclude // @has and // @!has lines, those come up all the time for rustdoc.

[Anthuang]

I've made it check only for lines containing the text you've mentioned, but there are many other lines that are too long (e.g. // gdbr-check, // lldbg-check, // CHECK, // @matches, etc.). Should I ignore these too? Or is it better to just leave the // ignore-tidy-linelength in the file?

I noticed that some of the lines are long intentionally, such as the line in src/test/ui/slowparse-string.rs, so we may have to leave the // ignore-tidy-linelength in some of the files.

[Mark-Simulacrum]

I think we want to allow any directives that contain error substrings (i.e., things that cannot be wrapped) via tidy changes, and any tests that remain after that should indeed retain ignore-tidy-linelength.

[Mark-Simulacrum]

I would prefer that we use the regex in compiletest in addition to the ANNOTATIONS_TO_IGNORE list; it looks like the regex covers more cases than just those enumerated. Please also add a comment to both locations pointing at the other location to make sure they get updated mutually as needed.

Also, it would be great if you could rebase your changes such that you have a single commit that changes code and another commit updating the tests for the fallout from that change (i.e., removing linelength annotations from a subset of the tests). If you don't know how to do this, I can try to explain, or I can do it for you, no worries there.

It also looks like CI is failing due to a lack of re-blessing, but I would hold off on doing so for now since it's likely I'll have further feedback after this round of review :)

[Anthuang]

Could you point me to where you're talking about? I found this line https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/runtest.rs#L3396 but not sure if this was what you were referring to.

I should be able to manage rebasing!

[Mark-Simulacrum]

Yes, sorry, I should have linked to it, I meant https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/runtest.rs#L3393-L3396 (same as you linked). I think I had implicitly linked it in my mind since I was reading @petrochenkov's comment earlier :)

[Anthuang]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Going to change the label without rebasing, since Mark said I can hold off on rebasing.

[Mark-Simulacrum]

r=me with a rebase (CI needs fixing, but I think that's part of the rebase in some sense)

[Anthuang]

I've just rebased! @Mark-Simulacrum

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[bors]

💔 Test failed - checks-actions

[Dylan-DPC-zz]

failed in rollup as well

@bors r-

[jyn514]

@bors rollup=never

[Mark-Simulacrum]

@Anthuang It looks like the tests in question failed because we're hard coding line numbers into them -- I imagine you can bump those back by one?

    [codegen] codegen\async-fn-debug-msvc.rs
    [codegen] codegen\generator-debug-msvc.rs

[Mark-Simulacrum]

 tidy error: /checkout/src/test/ui/associated-type-bounds/ambiguous-associated-type2.rs: ignoring line length unnecessarily
tidy error: /checkout/src/test/ui/simd-type-generic-monomorphisation.rs: ignoring line length unnecessarily
tidy error: /checkout/src/test/ui/simd-type.rs: ignoring line length unnecessarily
tidy error: /checkout/src/test/codegen/simd-intrinsic/simd-intrinsic-transmute-array.rs: ignoring line length unnecessarily
tidy error: /checkout/src/test/rustdoc/inline_cross/impl_trait.rs: ignoring line length unnecessarily

@bors p=5 for next approval

Let me know if you'd like me to fix these on the next go around instead of the back and forth or feel free to ping me on Zulip and I can try to fast-track an r+ for you :)

[jyn514]

@Mark-Simulacrum does it make sense to disable the "ignoring length unnecessarily" lint temporarily so this can land? Then once this lands people will stop adding the ignore and it will be easier to turn the lint back on.

[Mark-Simulacrum]

Yeah, that'd be fine with me as well, we can definitely do this in two parts.

[bors]

☔ The latest upstream changes (presumably #79637) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[crlf0710]

@Anthuang Ping from triage: any updates on this?

[jyn514]

@crlf0710 see #77675 (comment) - this fails every time it's rolled up or takes more than ~an hour to be approved because almost every single other PR has semantic conflicts. It needs either a treeclosed or to disable the lint altogether temporarily (#77675 (comment)) in order to be merged.

[JohnCSimon]

@Anthuang Ping from triage: unfortunately I'm closing this this due to inactivity - please feel free to reopen when you're ready to continue with this.