Skip to content

Add --attribute-allowlist and --attribute-denylist options for icu4x-… #7166

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
wants to merge 1 commit into
base: main
Choose a base branch
from
+50 −0
Open

Add --attribute-allowlist and --attribute-denylist options for icu4x-… #7166

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
50 changes: 50 additions & 0 deletions provider/icu4x-datagen/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,20 @@ struct Cli {
#[arg(help = "Analyzes the binary and only includes markers that are used by the binary.")]
markers_for_bin: Option<PathBuf>,

#[arg(long, num_args = 2.., value_names = ["DOMAIN", "ATTRIBUTES"])]
#[arg(
help = "Filter attributes on markers for a domain. Accepts two or more arguments.\n\
Copy link
Member

@robertbastian robertbastian Oct 27, 2025

Choose a reason for hiding this comment

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

suggestion: instead of accepting a list of exact matches, it's probably more user-friendly (and more expressiv) to accept a regex. we already have the regex crate in the transitive dependencies anyway

we'd also not need both an allowlist and a denylist

Copy link
Member

@Manishearth Manishearth Oct 27, 2025

Choose a reason for hiding this comment

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

we'd also not need both an allowlist and a denylist

I'm not sure about this: inverting regexes is really annoying. having a way to both allowlist and denylist by-regex would be good, as long as we ensure they never clash

Copy link
Member

@robertbastian robertbastian Oct 27, 2025

Choose a reason for hiding this comment

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

We could have some special negate syntax as part of the values as well. In any case we need to figure out how an overlap of allow and deny should be handled when there are multiple filters for the same domain.

Copy link
Member

@Manishearth Manishearth Oct 27, 2025

Choose a reason for hiding this comment

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

That would be fine.

As for overlaps: I was thinking that we simply disallow them; in theory we could pick a priority and support it (or have it based on declaration order), but 99% of use cases will be covered by a single filter.

Having a single attribute with +/- syntax seems great.

Copy link
Member

@sffc sffc Oct 29, 2025

Choose a reason for hiding this comment

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

It's really nontrivial to design a good +/- syntax. It would be nice to avoid it if we have a better alternative. I've found from experience that the all/any/not thing used in cfg is the simplest design that covers basically all the use cases.

I wouldn't mind something like:

# Include one numbering system, latn
--marker-attributes-filter numbering_system=latn
--marker-attributes-filter numbering_system=/^latn$/

# Include latn and thai numbering systems
--marker-attribute-filter numbering_system=any(latn,thai)
--marker-attribute-filter numbering_system=/^(latn|thai)$/

# Include all numbering systems except for thai
--marker-attribute-filter numbering_system=not(thai)
--marker-attribute-filter numbering_system=/^(?!thai$).*/

I think string literals with all/any/not would cover 90% of use cases.

Copy link
Member

@sffc sffc Oct 29, 2025

Choose a reason for hiding this comment

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

I'm not excited about an allowlist/denylist filter argument pair. I think a single filter is a better path forward and we can iterate on the DSL in that context.

Copy link
Member

@Manishearth Manishearth Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm mostly worried about designing ourselves out of a convenient DSL by implementing it partially. Especially since parsing regexes is nontrivial.

Personally I weakly consider negative filtering to be a part of the MVP. I don't actually think there are other features we need in the DSL.

I think --filter="attr1=/foo/" is probably okay since it's easy to add attr1=-/foo/ later. As long as our DSL supports at most one filter per argument we're fine, we shouldn't be in a situation where we have to parse --filters="attr1=filter1 attr2=filter2" because then you need to learn where regexes end.

Copy link
Member

@sffc sffc Oct 29, 2025

Choose a reason for hiding this comment

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

I think the simplest and most foolproof implementation might be to allow negating the whole regex, and if the argument is specified multiple times, then perform the checks in series.

Example Args:

--marker-attributes-filter domain1=/^[abc].*$/
--marker-attributes-filter domain1=!/^a[de].*$/
--marker-attributes-filter domain1=/^aef.*$/
--marker-attributes-filter domain2=/^(abc|xyz)$/

This gets compiled into an ordered list of rules for domain1 and an ordered list of rules for domain2. If the first rule is positive, then the initial set is empty. If the first rule is negative, then the initial set is the whole set.

As an onramp, we can accept either a single positive regex or a single negative regex, and reject if the same domain was provided multiple times.

Copy link
Member

@sffc sffc Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To make it easier to type in Bash, probably omit the ^ and $ in the regex, and use - instead of ! for negation.

Copy link
Member

@Manishearth Manishearth Oct 29, 2025

Choose a reason for hiding this comment

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

I think that's a good starting point. I am worried about ^ and $ though, but I think if we always implicitly check for whole-string match users can always use * to do partial matches.

Copy link
Member

@robertbastian robertbastian Oct 27, 2025

Choose a reason for hiding this comment

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

issue: it should also be possible to add two filters for different domains

Copy link
Member

@Manishearth Manishearth Oct 27, 2025

Choose a reason for hiding this comment

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

Agreed.

The attributes are selected in the output."
)]
attribute_allowlist: Vec<String>,

#[arg(long, num_args = 2.., value_names = ["DOMAIN", "ATTRIBUTES"])]
#[arg(
help = "Filter out attributes on markers for a domain. Accepts two or more arguments.\n\
The attributes are excluded in the output."
)]
attribute_denylist: Vec<String>,

#[arg(long, short, num_args = 0..)]
#[cfg_attr(feature = "provider", arg(default_value = "recommended"))]
#[arg(
Expand Down Expand Up @@ -528,6 +542,42 @@ fn main() -> eyre::Result<()> {
driver.with_segmenter_models(cli.segmenter_models.clone())
};

if !cli.attribute_allowlist.is_empty() {
driver = driver.with_marker_attributes_filter(
&cli.attribute_allowlist[0].clone(),
move |attrs| {
let (_prefix, unit) = attrs
.as_str()
.split_once('-')
.unwrap_or(("", attrs.as_str()));

cli.attribute_allowlist
.clone()
.iter()
.skip(1)
.any(|attr| attr == unit)
},
);
};

if !cli.attribute_denylist.is_empty() {
driver = driver.with_marker_attributes_filter(
&cli.attribute_denylist[0].clone(),
move |attrs| {
let (_prefix, unit) = attrs
.as_str()
.split_once('-')
.unwrap_or(("", attrs.as_str()));

!cli.attribute_denylist
.clone()
.iter()
.skip(1)
.any(|attr| attr == unit)
},
);
};

let metadata: Result<ExportMetadata, DataError> = match cli.format {
#[cfg(not(feature = "fs_exporter"))]
Format::Fs => {
Expand Down