Skip to content

some cleanup in rustc_session #159398

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

Open
hkBst wants to merge 1 commit into
base: main
Choose a base branch
from
Open

some cleanup in rustc_session #159398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions compiler/rustc_session/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1945,8 +1945,7 @@ pub fn get_cmd_lint_options(

lint_opts_with_position.sort_by_key(|x| x.0);
let lint_opts = lint_opts_with_position
.iter()
.cloned()
.into_iter()
.map(|(_, lint_name, level)| (lint_name, level))
.collect();

Expand Down
227 changes: 68 additions & 159 deletions compiler/rustc_session/src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,70 +970,38 @@ pub mod parse {

/// Use this for any string option that has a static default.
pub(crate) fn parse_string(slot: &mut String, v: Option<&str>) -> bool {
match v {
Some(s) => {
*slot = s.to_string();
true
}
None => false,
}
v.inspect(|s| *slot = s.to_string()).is_some()
}

/// Use this for any string option that lacks a static default.
pub(crate) fn parse_opt_string(slot: &mut Option<String>, v: Option<&str>) -> bool {
match v {
Some(s) => {
*slot = Some(s.to_string());
true
}
None => false,
}
v.inspect(|s| *slot = Some(s.to_string())).is_some()
}

pub(crate) fn parse_opt_pathbuf(slot: &mut Option<PathBuf>, v: Option<&str>) -> bool {
match v {
Some(s) => {
*slot = Some(PathBuf::from(s));
true
}
None => false,
}
v.inspect(|s| *slot = Some(PathBuf::from(s))).is_some()
}

pub(crate) fn parse_string_push(slot: &mut Vec<String>, v: Option<&str>) -> bool {
match v {
Some(s) => {
slot.push(s.to_string());
true
}
None => false,
}
v.inspect(|s| slot.push(s.to_string())).is_some()
}

pub(crate) fn parse_list(slot: &mut Vec<String>, v: Option<&str>) -> bool {
match v {
Some(s) => {
slot.extend(s.split_whitespace().map(|s| s.to_string()));
true
}
None => false,
}
v.inspect(|s| slot.extend(s.split_whitespace().map(|s| s.to_string()))).is_some()
}

pub(crate) fn parse_list_with_polarity(
slot: &mut Vec<(String, bool)>,
v: Option<&str>,
) -> bool {
match v {
Some(s) => {
for s in s.split(',') {
let Some(pass_name) = s.strip_prefix(&['+', '-'][..]) else { return false };
slot.push((pass_name.to_string(), &s[..1] == "+"));
v.is_some_and(|s| {
s.split(',').all(|s| {
s.starts_with(['+', '-']) && {
slot.push((s[1..].to_string(), &s[..1] == "+"));
true
}
true
}
None => false,
}
})
})
}

pub(crate) fn parse_fmt_debug(opt: &mut FmtDebug, v: Option<&str>) -> bool {
Expand Down Expand Up @@ -1069,27 +1037,21 @@ pub mod parse {
}

pub(crate) fn parse_comma_list(slot: &mut Vec<String>, v: Option<&str>) -> bool {
match v {
Some(s) => {
let mut v: Vec<_> = s.split(',').map(|s| s.to_string()).collect();
v.sort_unstable();
*slot = v;
true
}
None => false,
}
v.inspect(|s| {
let mut v: Vec<_> = s.split(',').map(|s| s.to_string()).collect();
v.sort_unstable();
*slot = v;
})
.is_some()
}

pub(crate) fn parse_opt_comma_list(slot: &mut Option<Vec<String>>, v: Option<&str>) -> bool {
match v {
Some(s) => {
let mut v: Vec<_> = s.split(',').map(|s| s.to_string()).collect();
v.sort_unstable();
*slot = Some(v);
true
}
None => false,
}
v.inspect(|s| {
let mut v: Vec<_> = s.split(',').map(|s| s.to_string()).collect();
v.sort_unstable();
*slot = Some(v);
})
.is_some()
}

pub(crate) fn parse_threads(slot: &mut Option<usize>, v: Option<&str>) -> bool {
Expand All @@ -1113,27 +1075,15 @@ pub mod parse {

/// Use this for any numeric option that has a static default.
pub(crate) fn parse_number<T: Copy + FromStr>(slot: &mut T, v: Option<&str>) -> bool {
match v.and_then(|s| s.parse().ok()) {
Some(i) => {
*slot = i;
true
}
None => false,
}
v.and_then(|s| s.parse().ok()).inspect(|i| *slot = *i).is_some()
}

/// Use this for any numeric option that lacks a static default.
pub(crate) fn parse_opt_number<T: Copy + FromStr>(
slot: &mut Option<T>,
v: Option<&str>,
) -> bool {
match v {
Some(s) => {
*slot = s.parse().ok();
slot.is_some()
}
None => false,
}
v.is_some_and(|s| s.parse().inspect(|s| *slot = Some(*s)).is_ok())
}

pub(crate) fn parse_frame_pointer(slot: &mut FramePointer, v: Option<&str>) -> bool {
Expand Down Expand Up @@ -1231,19 +1181,12 @@ pub mod parse {
}

pub(crate) fn parse_relro_level(slot: &mut Option<RelroLevel>, v: Option<&str>) -> bool {
match v {
Some(s) => match s.parse::<RelroLevel>() {
Ok(level) => *slot = Some(level),
_ => return false,
},
_ => return false,
}
true
v.is_some_and(|s| s.parse::<RelroLevel>().inspect(|level| *slot = Some(*level)).is_ok())
}

pub(crate) fn parse_sanitizers(slot: &mut SanitizerSet, v: Option<&str>) -> bool {
if let Some(v) = v {
for s in v.split(',') {
v.is_some_and(|v| {
v.split(',').all(|s| {
*slot |= match s {
"address" => SanitizerSet::ADDRESS,
"cfi" => SanitizerSet::CFI,
Expand All @@ -1260,12 +1203,10 @@ pub mod parse {
"safestack" => SanitizerSet::SAFESTACK,
"realtime" => SanitizerSet::REALTIME,
_ => return false,
}
}
true
} else {
false
}
};
true
})
})
}

pub(crate) fn parse_sanitizer_memory_track_origins(slot: &mut usize, v: Option<&str>) -> bool {
Expand All @@ -1287,12 +1228,12 @@ pub mod parse {
}

pub(crate) fn parse_strip(slot: &mut Strip, v: Option<&str>) -> bool {
match v {
Some("none") => *slot = Strip::None,
Some("debuginfo") => *slot = Strip::Debuginfo,
Some("symbols") => *slot = Strip::Symbols,
*slot = match v {
Some("none") => Strip::None,
Some("debuginfo") => Strip::Debuginfo,
Some("symbols") => Strip::Symbols,
_ => return false,
}
};
true
}

Expand Down Expand Up @@ -1349,56 +1290,41 @@ pub mod parse {
slot: &mut DebugInfoCompression,
v: Option<&str>,
) -> bool {
match v {
Some("none") => *slot = DebugInfoCompression::None,
Some("zlib") => *slot = DebugInfoCompression::Zlib,
Some("zstd") => *slot = DebugInfoCompression::Zstd,
*slot = match v {
Some("none") => DebugInfoCompression::None,
Some("zlib") => DebugInfoCompression::Zlib,
Some("zstd") => DebugInfoCompression::Zstd,
_ => return false,
};
true
}

pub(crate) fn parse_mir_strip_debuginfo(slot: &mut MirStripDebugInfo, v: Option<&str>) -> bool {
match v {
Some("none") => *slot = MirStripDebugInfo::None,
Some("locals-in-tiny-functions") => *slot = MirStripDebugInfo::LocalsInTinyFunctions,
Some("all-locals") => *slot = MirStripDebugInfo::AllLocals,
*slot = match v {
Some("none") => MirStripDebugInfo::None,
Some("locals-in-tiny-functions") => MirStripDebugInfo::LocalsInTinyFunctions,
Some("all-locals") => MirStripDebugInfo::AllLocals,
_ => return false,
};
true
}

pub(crate) fn parse_linker_flavor(slot: &mut Option<LinkerFlavorCli>, v: Option<&str>) -> bool {
match v.and_then(|v| LinkerFlavorCli::from_str(v).ok()) {
Some(lf) => *slot = Some(lf),
_ => return false,
}
true
v.is_some_and(|v| LinkerFlavorCli::from_str(v).inspect(|lf| *slot = Some(*lf)).is_ok())
}

pub(crate) fn parse_opt_symbol_visibility(
slot: &mut Option<SymbolVisibility>,
v: Option<&str>,
) -> bool {
if let Some(v) = v {
if let Ok(vis) = SymbolVisibility::from_str(v) {
*slot = Some(vis);
} else {
return false;
}
}
true
v.is_some_and(|v| SymbolVisibility::from_str(v).inspect(|vis| *slot = Some(*vis)).is_ok())
}

pub(crate) fn parse_unpretty(slot: &mut Option<String>, v: Option<&str>) -> bool {
match v {
None => false,
Some(s) if s.split('=').count() <= 2 => {
*slot = Some(s.to_string());
true
}
_ => false,
}
v.is_some_and(|s| {
*slot = (s.split('=').count() <= 2).then(|| s.to_string());
slot.is_some()
})
}

pub(crate) fn parse_time_passes_format(slot: &mut TimePassesFormat, v: Option<&str>) -> bool {
Expand Down Expand Up @@ -1432,50 +1358,33 @@ pub mod parse {
}

pub(crate) fn parse_offload(slot: &mut Vec<Offload>, v: Option<&str>) -> bool {
let Some(v) = v else {
*slot = vec![];
return true;
};
let mut v: Vec<&str> = v.split(",").collect();
let mut v: Vec<&str> = v.unwrap_or_default().split(",").collect();
v.sort_unstable();
Comment on lines +1361 to 1362

@hkBst hkBst Jul 16, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The new code does not reset the slot to an empty Vec for a None argument. I'm curious to see if that matters.

View changes since the review

@bjorn3 bjorn3 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That would cause -Zoffload= to no longer override -Zoffload=a,b,c and instead silently get igbored, right? That is inconsistent with other cli flags where the last occurence does win.

@hkBst hkBst Jul 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is true, but at the same time, the empty value is the only one that overrides in this way: -Zoffload=a -Zoffload=b is the same as -Zoffload=a,b not as `-Zoffload=b', so it seems to already be inconsistent.

Anyway, no change was intended, so I'll revert. Thanks for explaining the significance to me!

@bjorn3 bjorn3 Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-Zoffload=a -Zoffload=b is the same as -Zoffload=a,b not as `-Zoffload=b'

Huh, that is inconsistent with other args that use parse_comma_list.

@hkBst hkBst Jul 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, the reset only happens for an empty argument.

@hkBst hkBst Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The function just before doesn't do a reset ever. Should it?

pub(crate) fn parse_dump_mono_stats(slot: &mut DumpMonoStatsFormat, v: Option<&str>) -> bool {
    match v {
        None => true, // hkBst: empty arg, no reset <-----
        Some("json") => {
            *slot = DumpMonoStatsFormat::Json;
            true
        }
        Some("markdown") => {
            *slot = DumpMonoStatsFormat::Markdown;
            true
        }
        Some(_) => false,
    }
}

for &val in v.iter() {
v.iter().all(|val| {

@bjorn3 bjorn3 Jul 17, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think using side-effects inside of an iterator method is an improvement.

View changes since the review

@hkBst hkBst Jul 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree that that is not ideal, but I think that disadvantage is outweighed by the advantage of showing this is an all combinator instead of some random for loop that just happens to be a manual implementation of the all combinator.

// Split each entry on '=' if it has an argument
let (key, arg) = match val.split_once('=') {
Some((k, a)) => (k, Some(a)),
None => (val, None),
None => (*val, None),
};

let variant = match key {
"Host" => {
if let Some(p) = arg {
Offload::Host(p.to_string())
} else {
return false;
}
}
"Device" => {
if let Some(_) = arg {
// Device does not accept a value
return false;
}
Offload::Device
match key {
"Host" => arg.inspect(|p| slot.push(Offload::Host(p.to_string()))).is_some(),
// Device does not accept a value
"Device" if arg.is_none() => {
slot.push(Offload::Device);
true
}
"Test" => {
if let Some(_) = arg {
// Test does not accept a value
return false;
}
Offload::Test
// Test does not accept a value
"Test" if arg.is_none() => {
slot.push(Offload::Test);
true
}
_ => {
// FIXME(ZuseZ4): print an error saying which value is not recognized
return false;
false
}
};
slot.push(variant);
}

true
}
})
}

pub(crate) fn parse_autodiff(slot: &mut Vec<AutoDiff>, v: Option<&str>) -> bool {
Expand Down
Loading