Skip to content

Commit

Permalink
Rollup merge of #102971 - est31:tidy_duplicate_lang_features, r=jyn514
Browse files Browse the repository at this point in the history 
tidy: error if a lang feature is already present

If a lang feature gets declared twice, like for example as a result of a mistake during stabilization, emit an error in tidy. Library features already have this logic.

Inspired by a mistake done during `half_open_range_patterns` stabilization: https://github.com/rust-lang/rust/pull/102275/files#r991292215

The PR requires #102883 to be merged before CI turns green because the check is doing its job.

For reviewers, I suggest [turning off whitespace changes](https://github.com/rust-lang/rust/pull/102971/files?w=1) in the diff by adding `?w=1` to the url, as a large part of the diff is just about removing one level of indentation.
  • Loading branch information
@JohnTitor
JohnTitor authored Oct 13, 2022
2 parents 36ea7cd + c278700 commit 18b1342
Showing 1 changed file with 137 additions and 126 deletions.
263 changes: 137 additions & 126 deletions src/tools/tidy/src/features.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//! * Language features in a group are sorted by feature name.
use crate::walk::{filter_dirs, walk, walk_many};
use std::collections::HashMap;
use std::collections::hash_map::{Entry, HashMap};
use std::fmt;
use std::fs;
use std::num::NonZeroU32;
Expand Down Expand Up @@ -280,13 +280,14 @@ fn test_filen_gate(filen_underscore: &str, features: &mut Features) -> bool {
}

pub fn collect_lang_features(base_compiler_path: &Path, bad: &mut bool) -> Features {
let mut all = collect_lang_features_in(base_compiler_path, "active.rs", bad);
all.extend(collect_lang_features_in(base_compiler_path, "accepted.rs", bad));
all.extend(collect_lang_features_in(base_compiler_path, "removed.rs", bad));
all
let mut features = Features::new();
collect_lang_features_in(&mut features, base_compiler_path, "active.rs", bad);
collect_lang_features_in(&mut features, base_compiler_path, "accepted.rs", bad);
collect_lang_features_in(&mut features, base_compiler_path, "removed.rs", bad);
features
}

fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features {
fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, bad: &mut bool) {
let path = base.join("rustc_feature").join("src").join(file);
let contents = t!(fs::read_to_string(&path));

Expand All @@ -298,135 +299,145 @@ fn collect_lang_features_in(base: &Path, file: &str, bad: &mut bool) -> Features
let mut in_feature_group = false;
let mut prev_names = vec![];

contents
.lines()
.zip(1..)
.filter_map(|(line, line_number)| {
let line = line.trim();

// Within -start and -end, the tracking issue can be omitted.
match line {
"// no-tracking-issue-start" => {
next_feature_omits_tracking_issue = true;
return None;
}
"// no-tracking-issue-end" => {
next_feature_omits_tracking_issue = false;
return None;
}
_ => {}
let lines = contents.lines().zip(1..);
for (line, line_number) in lines {
let line = line.trim();

// Within -start and -end, the tracking issue can be omitted.
match line {
"// no-tracking-issue-start" => {
next_feature_omits_tracking_issue = true;
continue;
}
"// no-tracking-issue-end" => {
next_feature_omits_tracking_issue = false;
continue;
}
_ => {}
}

if line.starts_with(FEATURE_GROUP_START_PREFIX) {
if in_feature_group {
tidy_error!(
bad,
"{}:{}: \
if line.starts_with(FEATURE_GROUP_START_PREFIX) {
if in_feature_group {
tidy_error!(
bad,
"{}:{}: \
new feature group is started without ending the previous one",
path.display(),
line_number,
);
}

in_feature_group = true;
prev_names = vec![];
return None;
} else if line.starts_with(FEATURE_GROUP_END_PREFIX) {
in_feature_group = false;
prev_names = vec![];
return None;
path.display(),
line_number,
);
}

let mut parts = line.split(',');
let level = match parts.next().map(|l| l.trim().trim_start_matches('(')) {
Some("active") => Status::Unstable,
Some("incomplete") => Status::Unstable,
Some("removed") => Status::Removed,
Some("accepted") => Status::Stable,
_ => return None,
};
let name = parts.next().unwrap().trim();

let since_str = parts.next().unwrap().trim().trim_matches('"');
let since = match since_str.parse() {
Ok(since) => Some(since),
Err(err) => {
tidy_error!(
bad,
"{}:{}: failed to parse since: {} ({:?})",
path.display(),
line_number,
since_str,
err,
);
None
}
};
if in_feature_group {
if prev_names.last() > Some(&name) {
// This assumes the user adds the feature name at the end of the list, as we're
// not looking ahead.
let correct_index = match prev_names.binary_search(&name) {
Ok(_) => {
// This only occurs when the feature name has already been declared.
tidy_error!(
bad,
"{}:{}: duplicate feature {}",
path.display(),
line_number,
name,
);
// skip any additional checks for this line
return None;
}
Err(index) => index,
};
in_feature_group = true;
prev_names = vec![];
continue;
} else if line.starts_with(FEATURE_GROUP_END_PREFIX) {
in_feature_group = false;
prev_names = vec![];
continue;
}

let correct_placement = if correct_index == 0 {
"at the beginning of the feature group".to_owned()
} else if correct_index == prev_names.len() {
// I don't believe this is reachable given the above assumption, but it
// doesn't hurt to be safe.
"at the end of the feature group".to_owned()
} else {
format!(
"between {} and {}",
prev_names[correct_index - 1],
prev_names[correct_index],
)
};
let mut parts = line.split(',');
let level = match parts.next().map(|l| l.trim().trim_start_matches('(')) {
Some("active") => Status::Unstable,
Some("incomplete") => Status::Unstable,
Some("removed") => Status::Removed,
Some("accepted") => Status::Stable,
_ => continue,
};
let name = parts.next().unwrap().trim();

let since_str = parts.next().unwrap().trim().trim_matches('"');
let since = match since_str.parse() {
Ok(since) => Some(since),
Err(err) => {
tidy_error!(
bad,
"{}:{}: failed to parse since: {} ({:?})",
path.display(),
line_number,
since_str,
err,
);
None
}
};
if in_feature_group {
if prev_names.last() > Some(&name) {
// This assumes the user adds the feature name at the end of the list, as we're
// not looking ahead.
let correct_index = match prev_names.binary_search(&name) {
Ok(_) => {
// This only occurs when the feature name has already been declared.
tidy_error!(
bad,
"{}:{}: duplicate feature {}",
path.display(),
line_number,
name,
);
// skip any additional checks for this line
continue;
}
Err(index) => index,
};

tidy_error!(
bad,
"{}:{}: feature {} is not sorted by feature name (should be {})",
path.display(),
line_number,
name,
correct_placement,
);
}
prev_names.push(name);
let correct_placement = if correct_index == 0 {
"at the beginning of the feature group".to_owned()
} else if correct_index == prev_names.len() {
// I don't believe this is reachable given the above assumption, but it
// doesn't hurt to be safe.
"at the end of the feature group".to_owned()
} else {
format!(
"between {} and {}",
prev_names[correct_index - 1],
prev_names[correct_index],
)
};

tidy_error!(
bad,
"{}:{}: feature {} is not sorted by feature name (should be {})",
path.display(),
line_number,
name,
correct_placement,
);
}
prev_names.push(name);
}

let issue_str = parts.next().unwrap().trim();
let tracking_issue = if issue_str.starts_with("None") {
if level == Status::Unstable && !next_feature_omits_tracking_issue {
tidy_error!(
bad,
"{}:{}: no tracking issue for feature {}",
path.display(),
line_number,
name,
);
}
None
} else {
let s = issue_str.split('(').nth(1).unwrap().split(')').next().unwrap();
Some(s.parse().unwrap())
};
Some((name.to_owned(), Feature { level, since, has_gate_test: false, tracking_issue }))
})
.collect()
let issue_str = parts.next().unwrap().trim();
let tracking_issue = if issue_str.starts_with("None") {
if level == Status::Unstable && !next_feature_omits_tracking_issue {
tidy_error!(
bad,
"{}:{}: no tracking issue for feature {}",
path.display(),
line_number,
name,
);
}
None
} else {
let s = issue_str.split('(').nth(1).unwrap().split(')').next().unwrap();
Some(s.parse().unwrap())
};
match features.entry(name.to_owned()) {
Entry::Occupied(e) => {
tidy_error!(
bad,
"{}:{} feature {name} already specified with status '{}'",
path.display(),
line_number,
e.get().level,
);
}
Entry::Vacant(e) => {
e.insert(Feature { level, since, has_gate_test: false, tracking_issue });
}
}
}
}

fn get_and_check_lib_features(
Expand Down

0 comments on commit 18b1342

Please sign in to comment.