Skip to content

WIP: rustfix-mode flag #353

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.

Sign up for GitHub

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

Draft
smoelius wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

WIP: rustfix-mode flag #353

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c1e4122
Reduce noise
smoelius Nov 22, 2025
b31ba8c
WIP
smoelius Nov 22, 2025
638d5a2
Update branch
smoelius Nov 29, 2025
32b25da
Replace hashes with `$HASH` placeholder
smoelius Dec 2, 2025
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
16 changes: 16 additions & 0 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,26 @@ impl Config {
config
.custom_comments
.insert("no-rustfix", |parser, _args, span| {
dbg!(&span);
// args are ignored (can be used as comment)
parser.set_custom_once("no-rustfix", (), span);
});

config
.custom_comments
.insert("rustfix-mode", |parser, args, span| {
dbg!(&span);
eprintln!("{}", std::backtrace::Backtrace::force_capture());
match args.content.parse::<RustfixMode>() {
Ok(mode) => {
parser.set_custom_once("rustfix-mode", mode, span);
Copy link
Owner

@oli-obk oli-obk Nov 22, 2025

Choose a reason for hiding this comment

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

If you set rustfix instead of rustfix-mode the other rustfix flag (line 158) gets overridden if the flag is set instead of both of the flags getting run

Copy link
Contributor Author

@smoelius smoelius Nov 22, 2025

Choose a reason for hiding this comment

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

Just to be clear, this rustflix flag?

ui_test/src/config.rs

Lines 156 to 158 in 2a32015

comment_defaults
.base()
.add_custom("rustfix", RustfixMode::MachineApplicable);

Could you point me to where that flag is checked?

Copy link
Owner

@oli-obk oli-obk Nov 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Flags are automatically handled. I'd need to look it up but likely you can grep for Box<dyn Flag>

Copy link
Contributor Author

@smoelius smoelius Nov 23, 2025

Choose a reason for hiding this comment

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

Flags are automatically handled. I'd need to look it up but likely you can grep for Box

I apologize. I am genuinely trying to understand what is going on here. But automatically handled by what? ui_test or something else?

If I grep for Box, I get 123 hits. Grepping for Box::new reduces that to 54. But is there anything more specific I could look for?

Copy link
Owner

@oli-obk oli-obk Nov 23, 2025

Choose a reason for hiding this comment

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

Oh lmao sorry. Github ate the generic params because it thought they are HTML. Edited

Copy link
Contributor Author

@smoelius smoelius Dec 1, 2025

Choose a reason for hiding this comment

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

@oli-obk Can I just confirm that this is how you want/expect ui_test to work? I'm just asking because I don't think I've seen a .fixed file with a $HASH annotation before.

Copy link
Contributor

@kirtchev-adacore kirtchev-adacore Dec 2, 2025

Choose a reason for hiding this comment

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

I think you should use the $HASH in Cargo.stdout files, such as the one you linked.

Copy link
Contributor Author

@smoelius smoelius Dec 2, 2025

Choose a reason for hiding this comment

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

Thanks, @kirtchev-adacore. My question to @oli-obk stands, though.

Copy link
Owner

@oli-obk oli-obk Dec 2, 2025

Choose a reason for hiding this comment

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

@oli-obk Can I just confirm that this is how you want/expect ui_test to work? I'm just asking because I don't think I've seen a .fixed file with a $HASH annotation before.

Well, I like dumping lots of info normally to users, so printing the command somewhere is useful. Not sure the current style is the thing I want long term, but just replacing output hashes with $HASH is what I do whenever it shows up in stdout/stderr.

Copy link
Contributor Author

@smoelius smoelius Dec 2, 2025

Choose a reason for hiding this comment

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

Thanks for your suggestion, @kirtchev-adacore. I pushed that change. I think there may still be some non-determinism in the ordering of the fields, though. For example, I see this in the diff between the previous version and the one I just pushed:

Screenshot From 2025-12-02 13-19-16

Regardless, I suspect this is tangential to my real problem: I cannot tell whether the .fixed files are being generated and checked. I wish there was an easy way to make that determination.

}
Err(error) => {
parser.error(args.span(), error.to_string());
}
}
});

config
.custom_comments
.insert("edition", |parser, args, _span| {
Expand Down
20 changes: 20 additions & 0 deletions src/custom_flags/rustfix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ use crate::{
per_test_config::{Comments, Revisioned, TestConfig},
Error, Errored, TestOk,
};
use anyhow::bail;
use rustfix::{CodeFix, Filter, Suggestion};
use spanned::{Span, Spanned};
use std::{
collections::HashSet,
path::{Path, PathBuf},
process::Output,
str::FromStr,
sync::Arc,
};

Expand Down Expand Up @@ -47,6 +49,7 @@ impl Flag for RustfixMode {
output: &Output,
build_manager: &BuildManager,
) -> Result<(), Errored> {
dbg!(*self);
let global_rustfix = match config.exit_status()? {
Some(Spanned {
content: 101 | 0, ..
Expand All @@ -56,6 +59,7 @@ impl Flag for RustfixMode {
let output = output.clone();
let no_run_rustfix = config.find_one_custom("no-rustfix")?;
Copy link
Owner

@oli-obk oli-obk Nov 22, 2025

Choose a reason for hiding this comment

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

This is how the value is loaded.

Copy link
Contributor Author

@smoelius smoelius Nov 22, 2025

Choose a reason for hiding this comment

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

When this function is entered, *self is already set to some RustfixMode. Should it be obvious to me how *self was determined?

Copy link
Owner

@oli-obk oli-obk Nov 22, 2025

Choose a reason for hiding this comment

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

Oh I forgot this was how it's done. See my other comment for what's happening

let fixes = if no_run_rustfix.is_none() && global_rustfix.enabled() {
dbg!(global_rustfix);
fix(&output.stderr, config.status.path(), global_rustfix).map_err(|err| Errored {
command: format!("rustfix {}", display(config.status.path())),
errors: vec![Error::Rustfix(err)],
Expand Down Expand Up @@ -99,11 +103,25 @@ impl Flag for RustfixMode {
}
}

impl FromStr for RustfixMode {
type Err = anyhow::Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"disabled" => Ok(RustfixMode::Disabled),
"everything" => Ok(RustfixMode::Everything),
"machine-applicable" => Ok(RustfixMode::MachineApplicable),
_ => bail!("unknown `RustfixMode`: {s}"),
}
}
}

fn fix(stderr: &[u8], path: &Path, mode: RustfixMode) -> anyhow::Result<Vec<String>> {
dbg!(path);
let suggestions = std::str::from_utf8(stderr)
.unwrap()
.lines()
.filter_map(|line| {
// dbg!(line);
if !line.starts_with('{') {
return None;
}
Expand All @@ -121,6 +139,8 @@ fn fix(stderr: &[u8], path: &Path, mode: RustfixMode) -> anyhow::Result<Vec<Stri
)
})
.collect::<Vec<_>>();
// dbg!(mode);
// dbg!(&suggestions);
if suggestions.is_empty() {
return Ok(Vec::new());
}
Expand Down
3 changes: 3 additions & 0 deletions src/per_test_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ impl TestConfig {
}

pub(crate) fn run_test(&mut self, build_manager: &Arc<BuildManager>) -> TestResult {
dbg!();
self.patch_out_dir();

let mut cmd = self.build_command(build_manager)?;
Expand All @@ -384,7 +385,9 @@ impl TestConfig {
let output = self.check_test_result(&cmd, output)?;

for rev in self.comments() {
dbg!(); // rev);
for custom in rev.custom.values() {
dbg!(&custom.content);
for flag in &custom.content {
flag.post_test_action(self, &output, build_manager)?;
}
Expand Down
Loading
Loading