From 3c4f24deaebf5759f4061de8f5ac24ecc599b93f Mon Sep 17 00:00:00 2001 From: Travis Long Date: Thu, 20 Aug 2026 12:17:43 -0500 Subject: [PATCH] Bug 2053531 - Add nimbus-fml lint Adds a lint command covering feature metadata, descriptions, naming and feature design, and a `no-lint` field to silence lints per feature or per file. `validate` no longer reports feature metadata warnings; that's the lint command's job now. --- CHANGELOG.md | 14 +- .../fixtures/fe/lints/included.fml.yaml | 19 + .../fixtures/fe/lints/including.fml.yaml | 22 + .../fixtures/fe/lints/needs-work.fml.yaml | 61 ++ .../fixtures/fe/lints/suppressions.fml.yaml | 25 + .../fixtures/fe/lints/well-formed.fml.yaml | 32 + .../src/backends/frontend_manifest.rs | 1 + .../nimbus-fml/src/command_line/cli.rs | 37 + .../nimbus-fml/src/command_line/commands.rs | 12 + .../nimbus-fml/src/command_line/mod.rs | 25 +- .../nimbus-fml/src/command_line/workflows.rs | 588 +++++++++++++-- components/support/nimbus-fml/src/frontend.rs | 11 + components/support/nimbus-fml/src/lib.rs | 1 + .../support/nimbus-fml/src/lints/design.rs | 516 +++++++++++++ .../nimbus-fml/src/lints/documentation.rs | 240 +++++++ .../support/nimbus-fml/src/lints/metadata.rs | 153 ++++ .../support/nimbus-fml/src/lints/mod.rs | 675 ++++++++++++++++++ .../support/nimbus-fml/src/lints/naming.rs | 451 ++++++++++++ components/support/nimbus-fml/src/main.rs | 1 + components/support/nimbus-fml/src/parser.rs | 10 + 20 files changed, 2823 insertions(+), 71 deletions(-) create mode 100644 components/support/nimbus-fml/fixtures/fe/lints/included.fml.yaml create mode 100644 components/support/nimbus-fml/fixtures/fe/lints/including.fml.yaml create mode 100644 components/support/nimbus-fml/fixtures/fe/lints/needs-work.fml.yaml create mode 100644 components/support/nimbus-fml/fixtures/fe/lints/suppressions.fml.yaml create mode 100644 components/support/nimbus-fml/fixtures/fe/lints/well-formed.fml.yaml create mode 100644 components/support/nimbus-fml/src/lints/design.rs create mode 100644 components/support/nimbus-fml/src/lints/documentation.rs create mode 100644 components/support/nimbus-fml/src/lints/metadata.rs create mode 100644 components/support/nimbus-fml/src/lints/mod.rs create mode 100644 components/support/nimbus-fml/src/lints/naming.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ce54a862685..0a37834603c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,5 @@ # v156.0 (In progress) -## ✨ What's Changed ✨ - -### Nimbus - -- A new API has been added to get the list of enrolled experiments and rollouts without instantiating a NimbusClient: `get_active_enrollments()`. ([#7560](https://github.com/mozilla/application-services/pull/7560)) - [Full Changelog](In progress) ## ✨ What's Changed ✨ @@ -14,6 +8,14 @@ - Add `LoginStore::list_candidates()` and `LoginStore::get_many()`, a pair of read APIs for consumers which filter logins on their unencrypted fields. `list_candidates()` returns a `LoginCandidate` per stored login - everything `Login` has except the secure fields (`username`/`password`), so searching by `origin`, `httpRealm` or `formActionOrigin` no longer forces a primary password prompt. `get_many()` then decrypts just the logins which matched. `list()` is unchanged, for callers who really do want every login in cleartext. +### Nimbus + +- A new API has been added to get the list of enrolled experiments and rollouts without instantiating a NimbusClient: `get_active_enrollments()`. ([#7560](https://github.com/mozilla/application-services/pull/7560)) + +### Nimbus FML + +- Add `nimbus-fml lint`, which checks a manifest against feature design lints covering metadata, descriptions, naming, and feature shape. Findings are warnings and don't affect code generation; `--error-on-warning` fails the run, for CI. A `no-lint` list on a feature or at the top of a manifest excuses it from the lints it names, so older versions of `nimbus-fml` will reject a manifest that uses one. `nimbus-fml validate` no longer reports feature metadata warnings; run `nimbus-fml lint` for those. ([Bug 2053531](https://bugzilla.mozilla.org/show_bug.cgi?id=2053531)) + # v155.0 (_2026-08-13_) [Full Changelog](https://github.com/mozilla/application-services/compare/v154.0...v155.0) diff --git a/components/support/nimbus-fml/fixtures/fe/lints/included.fml.yaml b/components/support/nimbus-fml/fixtures/fe/lints/included.fml.yaml new file mode 100644 index 00000000000..59e1db719ef --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/lints/included.fml.yaml @@ -0,0 +1,19 @@ +--- +# Included by including.fml.yaml; excuses what it defines from a lint. +about: + description: Fixture for the lint tests. +no-lint: + - MISSING_META_BUG +features: + included-feature: + description: A feature defined in a file that another file includes. + contacts: + - jdoe@example.com + documentation: + - name: User documentation + url: https://example.com/included-feature + variables: + enabled: + description: Whether the included feature does anything at all. + type: Boolean + default: false diff --git a/components/support/nimbus-fml/fixtures/fe/lints/including.fml.yaml b/components/support/nimbus-fml/fixtures/fe/lints/including.fml.yaml new file mode 100644 index 00000000000..29fc7285095 --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/lints/including.fml.yaml @@ -0,0 +1,22 @@ +--- +# Includes a file that carries its own top level `no-lint` list. +about: + description: Fixture for the lint tests. +channels: + - release +includes: + - included.fml.yaml +features: + including-feature: + description: A feature defined in the file that does the including. + contacts: + - jdoe@example.com + documentation: + - name: User documentation + url: https://example.com/including-feature + meta-bug: https://example.com/bugs + variables: + enabled: + description: Whether the including feature does anything at all. + type: Boolean + default: false diff --git a/components/support/nimbus-fml/fixtures/fe/lints/needs-work.fml.yaml b/components/support/nimbus-fml/fixtures/fe/lints/needs-work.fml.yaml new file mode 100644 index 00000000000..b9098471b64 --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/lints/needs-work.fml.yaml @@ -0,0 +1,61 @@ +--- +# Trips as many lints as fit into one file. +about: + description: Fixture for the lint tests. +channels: + - release +features: + myBadFeature: + description: Bad + variables: + hide-toolbar: + description: TODO + type: Boolean + default: false + myBadFeature-mode: + description: Which of the layouts the feature uses. + type: String + default: compact + section-list: + description: The sections shown to the user, in the order they appear. + type: List + default: [] + deep: + description: A value that has to be written out four levels deep. + type: Outer + default: {} +objects: + Outer: + description: The outermost object. + fields: + middle: + description: The object in the middle. + type: Middle + default: {} + Middle: + description: The object in the middle. + fields: + inner: + description: The innermost object. + type: Inner + default: {} + Inner: + description: The innermost object. + fields: + label: + description: The label shown to the user. + type: String + default: "" + unusedObject: + description: An object that no feature refers to. + fields: + label: + description: The label shown to the user. + type: String + default: "" +enums: + OnlyOne: + description: An enum that doesn't offer a choice. + variants: + onlyVariant: + description: The only variant there is. diff --git a/components/support/nimbus-fml/fixtures/fe/lints/suppressions.fml.yaml b/components/support/nimbus-fml/fixtures/fe/lints/suppressions.fml.yaml new file mode 100644 index 00000000000..86b5dbd41f6 --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/lints/suppressions.fml.yaml @@ -0,0 +1,25 @@ +--- +# Excuses itself from some lints, at both levels. +about: + description: Fixture for the lint tests. +channels: + - release +no-lint: + - MISSING_META_BUG + - NOT_A_REAL_FILE_LINT +features: + legacy-feature: + description: A feature that was designed before there were lints to nudge it. + no-lint: + - MISSING_ENABLED_VARIABLE + - NOT_A_REAL_LINT + contacts: + - jdoe@example.com + documentation: + - name: User documentation + url: https://example.com/legacy-feature + variables: + max-rows: + description: The largest number of rows the list is allowed to grow to. + type: Int + default: 3 diff --git a/components/support/nimbus-fml/fixtures/fe/lints/well-formed.fml.yaml b/components/support/nimbus-fml/fixtures/fe/lints/well-formed.fml.yaml new file mode 100644 index 00000000000..ca37fe615ef --- /dev/null +++ b/components/support/nimbus-fml/fixtures/fe/lints/well-formed.fml.yaml @@ -0,0 +1,32 @@ +--- +# Trips no lints. +about: + description: Fixture for the lint tests. +channels: + - release +features: + toolbar-redesign: + description: The redesigned toolbar shown at the bottom of the browser screen. + meta-bug: https://bugzilla.mozilla.org/show_bug.cgi?id=2053531 + contacts: + - jdoe@example.com + documentation: + - name: User documentation + url: https://example.com/toolbar-redesign + variables: + enabled: + description: Whether the redesigned toolbar is shown instead of the old one. + type: Boolean + default: false + button-style: + description: How the buttons in the toolbar are drawn. + type: ButtonStyle + default: outline +enums: + ButtonStyle: + description: The ways a toolbar button can be drawn. + variants: + outline: + description: The button is drawn as an outline only. + filled: + description: The button is drawn filled with the accent colour. diff --git a/components/support/nimbus-fml/src/backends/frontend_manifest.rs b/components/support/nimbus-fml/src/backends/frontend_manifest.rs index fcf08f4f4e4..7821ba23c03 100644 --- a/components/support/nimbus-fml/src/backends/frontend_manifest.rs +++ b/components/support/nimbus-fml/src/backends/frontend_manifest.rs @@ -27,6 +27,7 @@ impl From for ManifestFrontEnd { channels, includes: Default::default(), imports: Default::default(), + no_lint: Default::default(), features, legacy_types: None, types: Types { enums, objects }, diff --git a/components/support/nimbus-fml/src/command_line/cli.rs b/components/support/nimbus-fml/src/command_line/cli.rs index 04aeddf1ad0..5c3437ce511 100644 --- a/components/support/nimbus-fml/src/command_line/cli.rs +++ b/components/support/nimbus-fml/src/command_line/cli.rs @@ -33,6 +33,9 @@ pub enum Command { /// Validate an FML configuration and all of its channels. Validate(Validate), + /// Check an FML configuration against the Nimbus feature design lints. + Lint(Lint), + /// Print out all the channels to stdout, as JSON or one-per-line Channels(Channels), @@ -119,6 +122,40 @@ pub struct Validate { pub loader_info: LoaderInfo, } +#[derive(Args)] +pub struct Lint { + /// Sets the input file to use + #[arg(value_name = "INPUT", required_unless_present = "list")] + pub input: Option, + + #[command(flatten)] + pub loader_info: LoaderInfo, + + /// Switch a lint off for this run. May be repeated. + #[arg(long, value_name = "LINT")] + pub allow: Vec, + + /// Turn a lint into an error for this run. May be repeated. + #[arg(long, value_name = "LINT")] + pub deny: Vec, + + /// Exit with an error if there are any warnings. + #[arg(long)] + pub error_on_warning: bool, + + /// Also lint the features of imported manifests. + #[arg(long)] + pub include_imports: bool, + + /// If present, then print the findings as JSON. + #[arg(long)] + pub json: bool, + + /// Print the available lints and exit. + #[arg(long)] + pub list: bool, +} + #[derive(Args)] pub struct Channels { /// Sets the input file to use diff --git a/components/support/nimbus-fml/src/command_line/commands.rs b/components/support/nimbus-fml/src/command_line/commands.rs index 53378bf65ce..b93ab415e87 100644 --- a/components/support/nimbus-fml/src/command_line/commands.rs +++ b/components/support/nimbus-fml/src/command_line/commands.rs @@ -14,6 +14,8 @@ pub(crate) enum CliCmd { GenerateSingleFileManifest(GenerateSingleFileManifestCmd), FetchFile(LoaderConfig, String), Validate(ValidateCmd), + Lint(LintCmd), + ListLints, PrintChannels(PrintChannelsCmd), PrintInfo(PrintInfoCmd), } @@ -48,6 +50,16 @@ pub(crate) struct ValidateCmd { pub(crate) loader: LoaderConfig, } +pub(crate) struct LintCmd { + pub(crate) manifest: String, + pub(crate) loader: LoaderConfig, + pub(crate) allow: Vec, + pub(crate) deny: Vec, + pub(crate) error_on_warning: bool, + pub(crate) include_imports: bool, + pub(crate) as_json: bool, +} + pub(crate) struct PrintChannelsCmd { pub(crate) manifest: String, pub(crate) loader: LoaderConfig, diff --git a/components/support/nimbus-fml/src/command_line/mod.rs b/components/support/nimbus-fml/src/command_line/mod.rs index 1a13004a45c..53e51c9a634 100644 --- a/components/support/nimbus-fml/src/command_line/mod.rs +++ b/components/support/nimbus-fml/src/command_line/mod.rs @@ -12,7 +12,7 @@ use anyhow::Result; use clap::Parser; use commands::{ CliCmd, GenerateExperimenterManifestCmd, GenerateSingleFileManifestCmd, GenerateStructCmd, - PrintChannelsCmd, ValidateCmd, + LintCmd, PrintChannelsCmd, ValidateCmd, }; use std::{collections::BTreeMap, ffi::OsString, path::Path}; @@ -39,6 +39,8 @@ fn process_command(cmd: &CliCmd) -> Result<()> { } CliCmd::FetchFile(files, nm) => workflows::fetch_file(files, nm)?, CliCmd::Validate(params) => workflows::validate(params)?, + CliCmd::Lint(params) => workflows::lint(params)?, + CliCmd::ListLints => workflows::list_lints()?, CliCmd::PrintChannels(params) => workflows::print_channels(params)?, CliCmd::PrintInfo(params) => workflows::print_info(params)?, }; @@ -68,6 +70,7 @@ where cli::Command::Validate(cmd) => { CliCmd::Validate(create_validate_command_from_cli(&cmd, cwd)?) } + cli::Command::Lint(cmd) => create_lint_command_from_cli(&cmd, cwd)?, cli::Command::Channels(cmd) => { CliCmd::PrintChannels(create_print_channels_from_cli(&cmd, cwd)?) } @@ -176,6 +179,26 @@ fn create_validate_command_from_cli(cmd: &cli::Validate, cwd: &Path) -> Result Result { + if cmd.list { + return Ok(CliCmd::ListLints); + } + + // clap has already checked that there is an input file if we're not listing. + let manifest = cmd.input.clone().unwrap_or_default(); + let loader = create_loader(&manifest, &cmd.loader_info, cwd)?; + + Ok(CliCmd::Lint(LintCmd { + manifest, + loader, + allow: cmd.allow.clone(), + deny: cmd.deny.clone(), + error_on_warning: cmd.error_on_warning, + include_imports: cmd.include_imports, + as_json: cmd.json, + })) +} + fn create_print_channels_from_cli(cmd: &cli::Channels, cwd: &Path) -> Result { let manifest = cmd.input.clone(); let loader = create_loader(&cmd.input, &cmd.loader_info, cwd)?; diff --git a/components/support/nimbus-fml/src/command_line/workflows.rs b/components/support/nimbus-fml/src/command_line/workflows.rs index 759fdb99d46..b0b9503bcbb 100644 --- a/components/support/nimbus-fml/src/command_line/workflows.rs +++ b/components/support/nimbus-fml/src/command_line/workflows.rs @@ -6,12 +6,13 @@ use glob::MatchOptions; use std::collections::HashSet; use super::commands::{ - GenerateExperimenterManifestCmd, GenerateSingleFileManifestCmd, GenerateStructCmd, + GenerateExperimenterManifestCmd, GenerateSingleFileManifestCmd, GenerateStructCmd, LintCmd, PrintChannelsCmd, PrintInfoCmd, ValidateCmd, }; use crate::backends::info::ManifestInfo; use crate::error::FMLError::CliError; use crate::frontend::ManifestFrontEnd; +use crate::lints::{self, Finding, LintConfig, LintLevel, LintReport}; use crate::{ backends, error::{FMLError, Result}, @@ -19,13 +20,24 @@ use crate::{ parser::Parser, util::loaders::{FileLoader, FilePath, LoaderConfig}, }; -use std::io::Write; +use std::io::{IsTerminal, Write}; use std::path::Path; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; /// Use this when recursively looking for files. const MATCHING_FML_EXTENSION: &str = ".fml.yaml"; +/// `ColorChoice::Auto` only looks at `TERM`, not at whether stdout is a terminal, so +/// on its own it writes escape codes into redirected output and CI logs. +fn stdout_stream() -> StandardStream { + let choice = if std::io::stdout().is_terminal() { + ColorChoice::Auto + } else { + ColorChoice::Never + }; + StandardStream::stdout(choice) +} + pub(crate) fn generate_struct(cmd: &GenerateStructCmd) -> Result<()> { let files: FileLoader = TryFrom::try_from(&cmd.loader)?; @@ -171,7 +183,7 @@ pub(crate) fn fetch_file(files: &LoaderConfig, nm: &str) -> Result<()> { Ok(()) } -fn output_ok(stream: &mut StandardStream, title: &str) -> Result<()> { +fn output_ok(stream: &mut impl WriteColor, title: &str) -> Result<()> { write!(stream, "✅ ")?; stream.set_color(ColorSpec::new().set_fg(Some(Color::Green)))?; writeln!(stream, "{title}")?; @@ -180,7 +192,7 @@ fn output_ok(stream: &mut StandardStream, title: &str) -> Result<()> { Ok(()) } -fn output_note(stream: &mut StandardStream, title: &str) -> Result<()> { +fn output_note(stream: &mut impl WriteColor, title: &str) -> Result<()> { write!(stream, "ℹ️ ")?; stream.set_color(ColorSpec::new().set_fg(Some(Color::Yellow)))?; writeln!(stream, "{title}")?; @@ -189,16 +201,77 @@ fn output_note(stream: &mut StandardStream, title: &str) -> Result<()> { Ok(()) } -fn output_warn(stream: &mut StandardStream, title: &str, detail: &str) -> Result<()> { - write!(stream, "⚠️ ")?; - stream.set_color(ColorSpec::new().set_fg(Some(Color::Yellow)))?; - write!(stream, "{title}")?; +/// The width lint help text wraps to. +const HELP_WIDTH: usize = 88; + +/// The column the message starts in. +const LINT_NAME_WIDTH: usize = 26; + +fn wrapped(text: &str, initial_indent: &str, subsequent_indent: &str) -> String { + let options = textwrap::Options::new(HELP_WIDTH) + .initial_indent(initial_indent) + .subsequent_indent(subsequent_indent) + // Lint prose is full of kebab-case names; don't break them at the hyphens. + .word_splitter(textwrap::WordSplitter::NoHyphenation); + textwrap::fill(text, options) +} + +fn output_finding(stream: &mut impl WriteColor, finding: &Finding) -> Result<()> { + let (icon, color) = match finding.level { + LintLevel::Error => ("❎", Color::Red), + _ => ("⚠️", Color::Yellow), + }; + + write!(stream, " {icon} ")?; + stream.set_color(ColorSpec::new().set_fg(Some(color)))?; + write!(stream, "{: Result<()> { +/// Print the findings grouped under the feature, object or enum they're about, +/// followed by the guidance for each lint that fired. The guidance is per lint, not +/// per finding: eighty features missing a `meta-bug` need telling how once. +fn output_findings(stream: &mut impl WriteColor, report: &LintReport) -> Result<()> { + let mut subject: Option<(&Option, &String)> = None; + + for finding in &report.findings { + let this = (&finding.module, &finding.subject); + if subject != Some(this) { + if subject.is_some() { + writeln!(stream)?; + } + subject = Some(this); + + stream.set_color(ColorSpec::new().set_bold(true))?; + write!(stream, "{}", finding.subject)?; + stream.reset()?; + match &finding.module { + Some(module) => writeln!(stream, " (imported from {module})")?, + None => writeln!(stream)?, + } + } + output_finding(stream, finding)?; + } + + let lints = report.triggered_lints(); + if !lints.is_empty() { + writeln!(stream, "\nWhat to do about these:")?; + for lint in lints { + stream.set_color(ColorSpec::new().set_bold(true))?; + writeln!(stream, " {}", lint.name)?; + stream.reset()?; + writeln!(stream, "{}", wrapped(lint.help, " ", " "))?; + } + writeln!(stream)?; + } + + Ok(()) +} + +fn output_err(stream: &mut impl WriteColor, title: &str, detail: &str) -> Result<()> { writeln!(stream, "❎ ")?; stream.set_color(ColorSpec::new().set_fg(Some(Color::Red)))?; writeln!(stream, "{title}")?; @@ -209,7 +282,7 @@ fn output_err(stream: &mut StandardStream, title: &str, detail: &str) -> Result< } pub(crate) fn validate(cmd: &ValidateCmd) -> Result<()> { - let mut stdout = StandardStream::stdout(ColorChoice::Auto); + let mut stdout = stdout_stream(); let files: FileLoader = TryFrom::try_from(&cmd.loader)?; @@ -261,59 +334,6 @@ pub(crate) fn validate(cmd: &ValidateCmd) -> Result<()> { ), )?; - writeln!(stdout, "Validating feature metadata:")?; - let mut features_with_warnings = 0; - for (_, f) in intermediate_representation.iter_all_feature_defs() { - let fm = &f.metadata; - let mut missing = vec![]; - if fm.meta_bug.is_none() { - missing.push("'meta-bug'"); - } - if fm.documentation.is_empty() { - missing.push("'documentation'"); - } - if fm.contacts.is_empty() { - missing.push("'contacts'"); - } - if !missing.is_empty() { - output_warn( - &mut stdout, - &format!("'{}' missing metadata", &f.name), - &missing.join(", "), - )?; - features_with_warnings += 1; - } - } - - if features_with_warnings == 0 { - output_ok(&mut stdout, "All feature metadata ok\n")?; - } else { - let features = if features_with_warnings == 1 { - "feature" - } else { - "features" - }; - writeln!( - &mut stdout, - "Each feature should have entries for at least:" - )?; - writeln!(&mut stdout, " - meta-bug: a URL where to file bugs")?; - writeln!( - &mut stdout, - " - documentation: a list of one or more URLs documenting the feature" - )?; - writeln!(&mut stdout, " e.g. QA docs, user docs")?; - writeln!( - &mut stdout, - " - contacts: a list of one or more email addresses" - )?; - writeln!(&mut stdout, " (with Mozilla Jira accounts)")?; - writeln!( - &mut stdout, - "Metadata warnings detected in {features_with_warnings} {features}\n" - )?; - } - writeln!(&mut stdout, "Validating manifest for different channels:")?; let results = channels @@ -358,6 +378,168 @@ pub(crate) fn validate(cmd: &ValidateCmd) -> Result<()> { Ok(()) } +fn lint_report(cmd: &LintCmd) -> Result { + let files: FileLoader = TryFrom::try_from(&cmd.loader)?; + let file_path = files.file_path(&cmd.manifest)?; + + // One parser for both passes: `load_manifest` walks the whole include tree, and + // a second one would fetch and parse all of it again. + let parser: Parser = Parser::new(files, file_path.clone())?; + + // The top level `no-lint` block lives in the file, not the IR. + let mut loading = HashSet::new(); + let manifest_front_end = parser.load_manifest(&file_path, &mut loading)?; + + let config = LintConfig::new() + .including_imports(cmd.include_imports) + .with_file_suppressions(&manifest_front_end.no_lint) + .allowing(&cmd.allow)? + .denying(&cmd.deny)?; + + // Linting an invalid manifest would report nonsense. + let ir = parser + .get_intermediate_representation(None) + .and_then(|ir| { + ir.validate_manifest_with(cmd.loader.lax_gecko_pref_validation) + .map(|_| ir) + }) + .map_err(|e| { + CliError(format!( + "{e}\nA manifest has to be valid before it can be linted; run `nimbus-fml validate` for the details" + )) + })?; + + Ok(lints::lint_manifest(&ir, &config)) +} + +pub(crate) fn lint(cmd: &LintCmd) -> Result<()> { + let mut stdout = stdout_stream(); + + let report = lint_report(cmd)?; + + if cmd.as_json { + println!("{}", serde_json::to_string_pretty(&json_report(&report))?); + } else { + output_findings(&mut stdout, &report)?; + output_lint_summary(&mut stdout, &report)?; + } + + let errors = report.error_count(); + let warnings = report.warning_count(); + + if errors > 0 { + return Err(CliError(format!( + "Manifest has {} lint error{}", + errors, + if errors > 1 { "s" } else { "" } + ))); + } + + if cmd.error_on_warning && warnings > 0 { + return Err(CliError(format!( + "Manifest has {} lint warning{}", + warnings, + if warnings > 1 { "s" } else { "" } + ))); + } + + Ok(()) +} + +/// The shape `--json` emits: the counts a CI job needs, plus the findings. +fn json_report(report: &LintReport) -> serde_json::Value { + serde_json::json!({ + "errors": report.error_count(), + "warnings": report.warning_count(), + "suppressed": report.suppressed, + "subjects": report.subject_count(), + "findings": report.findings, + }) +} + +fn output_suppressed(stream: &mut impl WriteColor, report: &LintReport) -> Result<()> { + if report.suppressed == 0 { + return Ok(()); + } + output_note( + stream, + &format!( + "{} finding{} silenced by `no-lint`", + report.suppressed, + if report.suppressed > 1 { "s" } else { "" } + ), + ) +} + +fn output_lint_summary(stream: &mut impl WriteColor, report: &LintReport) -> Result<()> { + if report.is_empty() { + output_ok(stream, "No lint findings")?; + return output_suppressed(stream, report); + } + + let errors = report.error_count(); + let warnings = report.warning_count(); + let mut counts = Vec::new(); + if errors > 0 { + counts.push(format!( + "{errors} error{}", + if errors > 1 { "s" } else { "" } + )); + } + if warnings > 0 { + counts.push(format!( + "{warnings} warning{}", + if warnings > 1 { "s" } else { "" } + )); + } + + let subjects = report.subject_count(); + writeln!( + stream, + "Found {} in {subjects} place{}.", + counts.join(" and "), + if subjects > 1 { "s" } else { "" } + )?; + output_suppressed(stream, report)?; + writeln!( + stream, + "{}", + wrapped( + "A lint that doesn't apply can be switched off for a single feature with a `no-lint: [LINT_NAME]` list on that feature, for the whole file with a top level `no-lint:` list, or for this run with `--allow LINT_NAME`.", + "", + "", + ) + )?; + + Ok(()) +} + +pub(crate) fn list_lints() -> Result<()> { + const NAME_WIDTH: usize = 26; + const CATEGORY_WIDTH: usize = 15; + const LEVEL_WIDTH: usize = 10; + + let mut stdout = stdout_stream(); + + writeln!( + stdout, + "{: Result<()> { let files = TryFrom::try_from(&cmd.loader)?; let manifest = Parser::load_frontend(files, &cmd.manifest)?; @@ -660,6 +842,284 @@ mod test { Ok(()) } + fn lint_cmd(path: &str) -> LintCmd { + LintCmd { + manifest: join(pkg_dir(), path), + loader: Default::default(), + allow: Default::default(), + deny: Default::default(), + error_on_warning: false, + include_imports: false, + as_json: false, + } + } + + /// The lints a fixture trips, sorted and deduplicated. + fn lints_for(path: &str) -> Result> { + let report = lint_report(&lint_cmd(path))?; + let mut lints: Vec<_> = report.findings.iter().map(|f| f.lint).collect(); + lints.sort_unstable(); + lints.dedup(); + Ok(lints) + } + + #[test] + fn test_lint_command_says_nothing_about_a_well_formed_manifest() -> Result<()> { + let path = "fixtures/fe/lints/well-formed.fml.yaml"; + assert_eq!(lints_for(path)?, Vec::<&str>::new()); + + // Warnings are all the lints produce by default, so this succeeds. + lint(&lint_cmd(path))?; + Ok(()) + } + + #[test] + fn test_lint_command_finds_the_problems_in_a_manifest() -> Result<()> { + assert_eq!( + lints_for("fixtures/fe/lints/needs-work.fml.yaml")?, + vec![ + "COMMON_PREFIX", + "DEEP_NESTING", + "ENUM_VARIANT_CASING", + "FEATURE_NAME_CASING", + "MISSING_CONTACTS", + "MISSING_DOCUMENTATION", + "MISSING_ENABLED_VARIABLE", + "MISSING_META_BUG", + "NEGATED_BOOLEAN", + "STRINGLY_TYPED", + "TERSE_DESCRIPTION", + "TODO_IN_DESCRIPTION", + "TRIVIAL_ENUM", + "TYPE_IN_NAME", + "TYPE_NAME_CASING", + "UNUSED_TYPE", + "VARIABLE_NAME_CASING", + ] + ); + Ok(()) + } + + #[test] + fn test_lint_command_honours_no_lint() -> Result<()> { + // The file excuses itself from MISSING_META_BUG, its feature from + // MISSING_ENABLED_VARIABLE. + assert_eq!( + lints_for("fixtures/fe/lints/suppressions.fml.yaml")?, + // ... but both lists also name a lint that doesn't exist. + vec!["UNKNOWN_LINT"] + ); + Ok(()) + } + + #[test] + fn test_lint_command_reports_unknown_names_at_both_levels() -> Result<()> { + let report = lint_report(&lint_cmd("fixtures/fe/lints/suppressions.fml.yaml"))?; + let unknown: Vec<_> = report + .findings + .iter() + .filter(|f| f.lint == "UNKNOWN_LINT") + .map(|f| (f.subject.as_str(), f.message.as_str())) + .collect(); + + assert_eq!( + unknown, + vec![ + ( + "feature `legacy-feature`", + "`no-lint` names `NOT_A_REAL_LINT`, which isn't a lint" + ), + ( + "this manifest", + "`no-lint` names `NOT_A_REAL_FILE_LINT`, which isn't a lint" + ), + ] + ); + Ok(()) + } + + #[test] + fn test_lint_says_when_no_lint_silenced_a_finding() -> Result<()> { + // The included file silences MISSING_META_BUG, so lint would otherwise call + // the manifest clean without saying why. + let report = lint_report(&lint_cmd("fixtures/fe/lints/including.fml.yaml"))?; + assert!(report.is_empty()); + assert_eq!(report.suppressed, 1); + + let mut buffer = termcolor::Buffer::no_color(); + output_suppressed(&mut buffer, &report)?; + let output = String::from_utf8(buffer.into_inner()).expect("output is UTF-8"); + assert!( + output.contains("1 finding silenced by `no-lint`"), + "{output}" + ); + + Ok(()) + } + + #[test] + fn test_lint_command_honours_no_lint_in_an_included_file() -> Result<()> { + // The included file excuses what it defines from MISSING_META_BUG; the + // including file provides its own. + let path = "fixtures/fe/lints/including.fml.yaml"; + assert_eq!(lints_for(path)?, Vec::<&str>::new()); + assert_eq!(lint_report(&lint_cmd(path))?.suppressed, 1); + Ok(()) + } + + #[test] + fn test_lint_command_allow_and_deny() -> Result<()> { + let path = "fixtures/fe/lints/needs-work.fml.yaml"; + + let mut cmd = lint_cmd(path); + cmd.allow = vec!["TRIVIAL_ENUM".to_string()]; + assert!(!lint_report(&cmd)? + .findings + .iter() + .any(|f| f.lint == "TRIVIAL_ENUM")); + + // Warnings on their own are not a failure... + let mut cmd = lint_cmd(path); + lint(&cmd)?; + + // ... but they are when they're denied. + cmd.deny = vec!["TRIVIAL_ENUM".to_string()]; + assert!(lint(&cmd).is_err()); + + // ... or when the run says so. + let mut cmd = lint_cmd(path); + cmd.error_on_warning = true; + assert!(lint(&cmd).is_err()); + + Ok(()) + } + + /// Render a report the way `lint` does, minus the colour. + fn rendered(report: &LintReport) -> Result { + let mut buffer = termcolor::Buffer::no_color(); + output_findings(&mut buffer, report)?; + output_lint_summary(&mut buffer, report)?; + Ok(String::from_utf8(buffer.into_inner()).expect("output is UTF-8")) + } + + #[test] + fn test_every_finding_says_what_it_is_about() -> Result<()> { + // Findings are grouped by feature, so two of the same lint in one feature + // are only distinguishable by their messages. + let cmd = lint_cmd("fixtures/fe/lints/needs-work.fml.yaml"); + let report = lint_report(&cmd)?; + let output = rendered(&report)?; + + let findings: Vec<_> = output + .lines() + .filter(|l| l.trim_start().starts_with(['⚠', '❎'])) + .collect(); + assert_eq!(findings.len(), report.findings.len()); + + let distinct: HashSet<_> = findings.iter().collect(); + assert_eq!( + distinct.len(), + findings.len(), + "two findings render identically:\n{output}" + ); + + Ok(()) + } + + #[test] + fn test_help_is_printed_once_per_lint() -> Result<()> { + let cmd = lint_cmd("fixtures/fe/lints/needs-work.fml.yaml"); + let report = lint_report(&cmd)?; + // The help is wrapped, so match against it unwrapped. + let output = rendered(&report)? + .split_whitespace() + .collect::>() + .join(" "); + + // The fixture trips MISSING_META_BUG once and TERSE_DESCRIPTION twice. + assert_eq!(output.matches("Add a `meta-bug` URL").count(), 1); + assert_eq!( + output + .matches("use the description to say what changes when the value changes") + .count(), + 1 + ); + + Ok(()) + } + + #[test] + fn test_grouping_names_each_subject_once() -> Result<()> { + let cmd = lint_cmd("fixtures/fe/lints/needs-work.fml.yaml"); + let report = lint_report(&cmd)?; + let output = rendered(&report)?; + + for subject in ["feature `myBadFeature`", "object `unusedObject`"] { + assert_eq!( + output.matches(&format!("\n{subject}\n")).count() + + usize::from(output.starts_with(&format!("{subject}\n"))), + 1, + "{subject} should head exactly one group:\n{output}" + ); + } + + Ok(()) + } + + #[test] + fn test_lint_command_counts_suppressed_findings() -> Result<()> { + // The fixture silences one lint for the file and one for its feature. + let report = lint_report(&lint_cmd("fixtures/fe/lints/suppressions.fml.yaml"))?; + assert_eq!(report.suppressed, 2); + assert!(rendered(&report)?.contains("2 findings silenced by `no-lint`")); + + Ok(()) + } + + #[test] + fn test_lint_command_json_carries_the_counts() -> Result<()> { + let mut cmd = lint_cmd("fixtures/fe/lints/needs-work.fml.yaml"); + cmd.deny = vec!["TRIVIAL_ENUM".to_string()]; + let report = lint_report(&cmd)?; + + let json = json_report(&report); + assert_eq!(json["errors"], 1); + assert_eq!(json["warnings"], report.warning_count()); + assert_eq!(json["suppressed"], 0); + assert_eq!(json["subjects"], 3); + assert_eq!( + json["findings"].as_array().unwrap().len(), + report.findings.len() + ); + + Ok(()) + } + + #[test] + fn test_lint_command_rejects_unknown_lint_names() { + let mut cmd = lint_cmd("fixtures/fe/lints/well-formed.fml.yaml"); + cmd.allow = vec!["NOT_A_LINT".to_string()]; + + let error = lint(&cmd).expect_err("An unknown lint name should be an error"); + assert!(error.to_string().contains("NOT_A_LINT")); + } + + #[test] + fn test_lint_command_ignores_imported_features_by_default() -> Result<()> { + let path = "fixtures/fe/importing/simple/app.yaml"; + + let cmd = lint_cmd(path); + let report = lint_report(&cmd)?; + assert!(report.findings.iter().all(|f| f.module.is_none())); + + let mut cmd = lint_cmd(path); + cmd.include_imports = true; + let report = lint_report(&cmd)?; + assert!(report.findings.iter().any(|f| f.module.is_some())); + + Ok(()) + } + #[test] fn test_validate_command_fails_on_bad_default_value_for_one_channel() -> Result<()> { let path = "fixtures/fe/invalid/invalid_default_value_for_one_channel.fml.yaml"; diff --git a/components/support/nimbus-fml/src/frontend.rs b/components/support/nimbus-fml/src/frontend.rs index cdfd788446e..3ab1b877f0a 100644 --- a/components/support/nimbus-fml/src/frontend.rs +++ b/components/support/nimbus-fml/src/frontend.rs @@ -261,6 +261,10 @@ pub(crate) struct FeatureMetadata { #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub(crate) configurator: Option, + /// Lints this feature is excused from, by name. + #[serde(default)] + #[serde(skip_serializing_if = "Vec::is_empty")] + pub(crate) no_lint: Vec, } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -292,6 +296,12 @@ pub struct ManifestFrontEnd { #[serde(skip_serializing_if = "Vec::is_empty")] pub(crate) imports: Vec, + /// Lints everything in this file is excused from, by name. + #[serde(default)] + #[serde(rename = "no-lint", alias = "no_lint")] + #[serde(skip_serializing_if = "Vec::is_empty")] + pub(crate) no_lint: Vec, + #[serde(default)] #[serde(skip_serializing_if = "BTreeMap::is_empty")] pub(crate) features: BTreeMap, @@ -722,6 +732,7 @@ mod feature_metadata { events: vec![Url::from_str( "https://example.com/glean/dictionary/button-pressed" )?,], + no_lint: Default::default(), } ); diff --git a/components/support/nimbus-fml/src/lib.rs b/components/support/nimbus-fml/src/lib.rs index 309ee8f9565..0d89a797161 100644 --- a/components/support/nimbus-fml/src/lib.rs +++ b/components/support/nimbus-fml/src/lib.rs @@ -9,6 +9,7 @@ mod editing; pub mod error; pub(crate) mod frontend; pub mod intermediate_representation; +pub mod lints; pub mod parser; pub(crate) mod schema; pub mod util; diff --git a/components/support/nimbus-fml/src/lints/design.rs b/components/support/nimbus-fml/src/lints/design.rs new file mode 100644 index 00000000000..ec19986943c --- /dev/null +++ b/components/support/nimbus-fml/src/lints/design.rs @@ -0,0 +1,516 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +//! Lints about the shape of a feature. A feature can pass validation and still be +//! one that can't be switched off, can't be configured, or can't be filled in +//! without reading the source. + +use std::collections::{BTreeMap, HashSet}; + +use super::{enum_path, feature_path, object_path, variable_path, RawFinding}; +use crate::intermediate_representation::{ + EnumDef, FeatureDef, FeatureManifest, ObjectDef, PropDef, TypeRef, +}; + +define_lints! { + NO_VARIABLES: Design, Warning = + "Features should have something an experiment can change.", + "An experiment on a feature with no variables can only enrol users; it can't change what they see. Add at least a boolean `enabled` variable."; + MISSING_ENABLED_VARIABLE: Design, Warning = + "Features should be able to be switched off remotely.", + "Features should be able to be switched off without shipping new code. Add `enabled: { type: Boolean }`, default it to the behaviour that ships today, and check it before using the rest of the feature."; + TOO_MANY_VARIABLES: Design, Warning = + "Features with a lot of variables are hard to configure correctly.", + "Every variable is something an experiment owner has to understand. Consider splitting the feature up, or grouping related variables into objects."; + STRINGLY_TYPED: Design, Warning = + "Values with a fixed set of options should be enums, not strings.", + "Declare an enum and use that as the type: Experimenter can then offer the options and reject anything else, instead of passing a typo through to the app. A `Map` can't be checked at all; consider an object for the values, or a `StringAlias` for the keys."; + DEEP_NESTING: Design, Warning = + "Deeply nested configuration is hard to write by hand.", + "Experiment owners write these values out as JSON by hand. Consider flattening the value, or adding an `examples` block showing a complete one."; + TRIVIAL_ENUM: Design, Warning = + "An enum should offer a choice.", + "An enum that can only take one value can't be varied by an experiment. Add the other variants, or use a boolean."; + UNUSED_TYPE: Design, Warning = + "Objects and enums should be used by at least one feature.", + "Unused types are still generated into Kotlin and Swift, and still have to be maintained. Delete it, or use it."; +} + +const MAX_VARIABLES: usize = 25; + +/// A feature's variables are level 1, the fields of an object they hold level 2. +const MAX_NESTING_DEPTH: usize = 3; + +/// Names that suggest a value is one of a fixed set of options. +const ENUMERABLE_SUFFIXES: &[&str] = &[ + "alignment", + "behavior", + "behaviour", + "direction", + "kind", + "layout", + "mode", + "placement", + "position", + "state", + "strategy", + "style", + "theme", + "treatment", + "type", + "variant", +]; + +pub(crate) fn check_feature( + feature: &FeatureDef, + manifest: &FeatureManifest, + out: &mut Vec, +) { + if feature.props.is_empty() { + out.push(RawFinding::new( + &NO_VARIABLES, + feature_path(feature), + "This feature has no variables", + )); + // The rest is about variables this feature doesn't have. + return; + } + + if !feature.props.iter().any(is_enabled_variable) { + out.push(RawFinding::new( + &MISSING_ENABLED_VARIABLE, + feature_path(feature), + "This feature has no boolean `enabled` variable", + )); + } + + if feature.props.len() > MAX_VARIABLES { + out.push(RawFinding::new( + &TOO_MANY_VARIABLES, + feature_path(feature), + format!( + "This feature has {} variables (the limit is {MAX_VARIABLES})", + feature.props.len() + ), + )); + } + + for prop in &feature.props { + check_stringly_typed(feature, prop, out); + check_nesting(feature, prop, &manifest.obj_defs, out); + } +} + +pub(crate) fn check_enum(enum_def: &EnumDef, out: &mut Vec) { + if enum_def.variants.len() < 2 { + out.push(RawFinding::new( + &TRIVIAL_ENUM, + enum_path(enum_def), + format!( + "This enum has {} variant{}", + enum_def.variants.len(), + if enum_def.variants.len() == 1 { + "" + } else { + "s" + } + ), + )); + } +} + +pub(crate) fn check_manifest(manifest: &FeatureManifest, out: &mut Vec) { + // A manifest with no features is a library of types to include elsewhere. + if manifest.feature_defs.is_empty() { + return; + } + + let mut used = HashSet::new(); + for feature in manifest.iter_feature_defs() { + for prop in &feature.props { + mark_used(&prop.typ, &manifest.obj_defs, &mut used); + } + } + + for object in manifest.iter_object_defs() { + if !used.contains(&TypeRef::Object(object.name.clone())) { + out.push(RawFinding::new( + &UNUSED_TYPE, + object_path(object), + "No feature in this manifest uses this object", + )); + } + } + + for enum_def in manifest.iter_enum_defs() { + if !used.contains(&TypeRef::Enum(enum_def.name.clone())) { + out.push(RawFinding::new( + &UNUSED_TYPE, + enum_path(enum_def), + "No feature in this manifest uses this enum", + )); + } + } +} + +/// Record every type reachable from `typ`. Unlike `TypeQuery`, this tolerates an +/// undefined object rather than panicking. +fn mark_used(typ: &TypeRef, objects: &BTreeMap, used: &mut HashSet) { + if !used.insert(typ.clone()) { + return; + } + + match typ { + TypeRef::Option(inner) | TypeRef::List(inner) | TypeRef::StringMap(inner) => { + mark_used(inner, objects, used) + } + TypeRef::EnumMap(keys, values) => { + mark_used(keys, objects, used); + mark_used(values, objects, used); + } + TypeRef::Object(name) => { + if let Some(object) = objects.get(name) { + for prop in &object.props { + mark_used(&prop.typ, objects, used); + } + } + } + _ => {} + } +} + +fn is_enabled_variable(prop: &PropDef) -> bool { + is_boolean(&prop.typ) && (prop.name == "enabled" || prop.name.ends_with("-enabled")) +} + +fn is_boolean(typ: &TypeRef) -> bool { + match typ { + TypeRef::Boolean => true, + TypeRef::Option(inner) => is_boolean(inner), + _ => false, + } +} + +fn check_stringly_typed(feature: &FeatureDef, prop: &PropDef, out: &mut Vec) { + // A string-alias already says "one of a known set of strings". + if prop.string_alias.is_some() { + return; + } + + if let TypeRef::StringMap(values) = &prop.typ { + if matches!(**values, TypeRef::String) { + out.push(RawFinding::new( + &STRINGLY_TYPED, + variable_path(feature, prop), + format!( + "`{}` is a `Map`, so neither its keys nor its values can be checked", + prop.name + ), + )); + return; + } + } + + if !is_string(&prop.typ) { + return; + } + + let suffix = prop + .name + .rsplit('-') + .next() + .map(str::to_ascii_lowercase) + .unwrap_or_default(); + // Plurals count: `supported-modes` is as enumerable as `mode`. + let singular = suffix.strip_suffix('s').unwrap_or(&suffix); + + if ENUMERABLE_SUFFIXES.contains(&singular) { + out.push(RawFinding::new( + &STRINGLY_TYPED, + variable_path(feature, prop), + format!( + "`{}` is a `{}`, but its name suggests it is one of a fixed set of values", + prop.name, prop.typ + ), + )); + } +} + +fn is_string(typ: &TypeRef) -> bool { + match typ { + TypeRef::String => true, + TypeRef::Option(inner) | TypeRef::List(inner) => is_string(inner), + _ => false, + } +} + +fn check_nesting( + feature: &FeatureDef, + prop: &PropDef, + objects: &BTreeMap, + out: &mut Vec, +) { + let mut visiting = HashSet::new(); + let depth = 1 + type_depth(&prop.typ, objects, &mut visiting); + if depth > MAX_NESTING_DEPTH { + out.push(RawFinding::new( + &DEEP_NESTING, + variable_path(feature, prop), + format!( + "The value of `{}` is {depth} levels deep (the limit is {MAX_NESTING_DEPTH})", + prop.name + ), + )); + } +} + +/// How many levels of object are underneath this type. +fn type_depth( + typ: &TypeRef, + objects: &BTreeMap, + visiting: &mut HashSet, +) -> usize { + match typ { + TypeRef::Option(inner) | TypeRef::List(inner) | TypeRef::StringMap(inner) => { + type_depth(inner, objects, visiting) + } + TypeRef::EnumMap(_, values) => type_depth(values, objects, visiting), + TypeRef::Object(name) => { + if !visiting.insert(name.clone()) { + // Stop at a cycle; it is already deep enough to report. + return MAX_NESTING_DEPTH; + } + let depth = objects + .get(name) + .map(|o| { + o.props + .iter() + .map(|p| type_depth(&p.typ, objects, visiting)) + .max() + .unwrap_or_default() + }) + .unwrap_or_default(); + visiting.remove(name); + 1 + depth + } + _ => 0, + } +} + +#[cfg(test)] +mod unit_tests { + use super::*; + use serde_json::json; + + fn prop(name: &str, typ: &TypeRef) -> PropDef { + PropDef::with_doc(name, "A description of the variable.", typ, &json!(null)) + } + + fn feature(props: Vec) -> FeatureDef { + FeatureDef::new("my-feature", "A description.", props, false) + } + + fn lints(feature: &FeatureDef) -> Vec<&'static str> { + lints_with(feature, &Default::default()) + } + + fn lints_with(feature: &FeatureDef, manifest: &FeatureManifest) -> Vec<&'static str> { + let mut out = Vec::new(); + check_feature(feature, manifest, &mut out); + out.iter().map(|f| f.lint.name).collect() + } + + #[test] + fn test_no_variables() { + let findings = lints(&feature(Default::default())); + assert_eq!(findings, vec!["NO_VARIABLES"]); + } + + #[test] + fn test_missing_enabled_variable() { + assert!(lints(&feature(vec![prop("max-rows", &TypeRef::Int)])) + .contains(&"MISSING_ENABLED_VARIABLE")); + + for name in ["enabled", "sync-enabled"] { + assert!( + !lints(&feature(vec![prop(name, &TypeRef::Boolean)])) + .contains(&"MISSING_ENABLED_VARIABLE"), + "{name} should count as an enabled variable" + ); + } + + // `Option`, as gecko-pref backed variables use, counts too. + assert!(!lints(&feature(vec![prop( + "enabled", + &TypeRef::Option(Box::new(TypeRef::Boolean)) + )])) + .contains(&"MISSING_ENABLED_VARIABLE")); + + // A string called `enabled` doesn't. + assert!(lints(&feature(vec![prop("enabled", &TypeRef::String)])) + .contains(&"MISSING_ENABLED_VARIABLE")); + } + + #[test] + fn test_too_many_variables() { + let props = (0..=MAX_VARIABLES) + .map(|i| prop(&format!("variable-{i}"), &TypeRef::Int)) + .collect(); + assert!(lints(&feature(props)).contains(&"TOO_MANY_VARIABLES")); + } + + #[test] + fn test_stringly_typed() { + assert!( + lints(&feature(vec![prop("button-style", &TypeRef::String)])) + .contains(&"STRINGLY_TYPED") + ); + assert!(lints(&feature(vec![prop( + "supported-modes", + &TypeRef::List(Box::new(TypeRef::String)) + )])) + .contains(&"STRINGLY_TYPED")); + assert!(lints(&feature(vec![prop( + "overrides", + &TypeRef::StringMap(Box::new(TypeRef::String)) + )])) + .contains(&"STRINGLY_TYPED")); + + // An enum is what the lint is asking for. + assert!(!lints(&feature(vec![prop( + "button-style", + &TypeRef::Enum("ButtonStyle".to_string()) + )])) + .contains(&"STRINGLY_TYPED")); + + // So is a string-alias. + let aliased = PropDef::with_string_alias( + "player-type", + &TypeRef::String, + &json!(null), + &TypeRef::StringAlias("PlayerType".to_string()), + ); + assert!(!lints(&feature(vec![aliased])).contains(&"STRINGLY_TYPED")); + + // A name that doesn't suggest fixed options is fine as a string. + assert!( + !lints(&feature(vec![prop("button-label", &TypeRef::String)])) + .contains(&"STRINGLY_TYPED") + ); + } + + #[test] + fn test_deep_nesting() { + let mut manifest = FeatureManifest { + obj_defs: ObjectDef::into_map(&[ + ObjectDef::new( + "Outer", + &[prop("middle", &TypeRef::Object("Middle".into()))], + ), + ObjectDef::new("Middle", &[prop("inner", &TypeRef::Object("Inner".into()))]), + ObjectDef::new("Inner", &[prop("label", &TypeRef::String)]), + ]), + ..Default::default() + }; + + // feature › Outer › Middle › Inner is 4 levels. + let outer = feature(vec![prop("outer", &TypeRef::Object("Outer".into()))]); + assert!(lints_with(&outer, &manifest).contains(&"DEEP_NESTING")); + + // feature › Middle › Inner is 3. + let middle = feature(vec![prop("middle", &TypeRef::Object("Middle".into()))]); + assert!(!lints_with(&middle, &manifest).contains(&"DEEP_NESTING")); + + // A list of objects is as deep as the objects in it. + let list_of_outers = feature(vec![prop( + "outers", + &TypeRef::List(Box::new(TypeRef::Object("Outer".into()))), + )]); + assert!(lints_with(&list_of_outers, &manifest).contains(&"DEEP_NESTING")); + + // A cycle terminates. + manifest.obj_defs.insert( + "Inner".to_string(), + ObjectDef::new("Inner", &[prop("outer", &TypeRef::Object("Outer".into()))]), + ); + assert!(lints_with(&outer, &manifest).contains(&"DEEP_NESTING")); + } + + #[test] + fn test_trivial_enum() { + let mut out = Vec::new(); + check_enum(&EnumDef::new("OnlyOne", &["only"]), &mut out); + assert_eq!( + out.iter().map(|f| f.lint.name).collect::>(), + vec!["TRIVIAL_ENUM"] + ); + + let mut out = Vec::new(); + check_enum(&EnumDef::new("TwoOfThem", &["this", "that"]), &mut out); + assert!(out.is_empty()); + } + + #[test] + fn test_unused_type() { + let used = TypeRef::Object("Used".into()); + let mut manifest = FeatureManifest { + obj_defs: ObjectDef::into_map(&[ + ObjectDef::new("Used", &[prop("style", &TypeRef::Enum("Style".into()))]), + ObjectDef::new("Unused", &[prop("label", &TypeRef::String)]), + ]), + enum_defs: EnumDef::into_map(&[ + EnumDef::new("Style", &["this", "that"]), + EnumDef::new("UnusedStyle", &["this", "that"]), + ]), + ..Default::default() + }; + manifest.add_feature(feature(vec![prop("used", &used)])); + + let mut out = Vec::new(); + check_manifest(&manifest, &mut out); + let paths: Vec<_> = out.iter().map(|f| f.location.subject.as_str()).collect(); + assert_eq!(paths, vec!["object `Unused`", "enum `UnusedStyle`"]); + } + + #[test] + fn test_unused_types_survive_a_dangling_object_reference() { + // `Missing` is never declared. Validation catches that; linting shouldn't + // panic on it. + let mut manifest = FeatureManifest { + obj_defs: ObjectDef::into_map(&[ObjectDef::new( + "Unused", + &[prop("label", &TypeRef::String)], + )]), + ..Default::default() + }; + manifest.add_feature(feature(vec![prop( + "dangling", + &TypeRef::Object("Missing".into()), + )])); + + let mut out = Vec::new(); + check_manifest(&manifest, &mut out); + assert_eq!( + out.iter() + .map(|f| f.location.subject.as_str()) + .collect::>(), + vec!["object `Unused`"] + ); + } + + #[test] + fn test_types_are_used_if_a_manifest_has_no_features() { + let manifest = FeatureManifest { + obj_defs: ObjectDef::into_map(&[ObjectDef::new( + "Exported", + &[prop("label", &TypeRef::String)], + )]), + ..Default::default() + }; + + let mut out = Vec::new(); + check_manifest(&manifest, &mut out); + assert!(out.is_empty()); + } +} diff --git a/components/support/nimbus-fml/src/lints/documentation.rs b/components/support/nimbus-fml/src/lints/documentation.rs new file mode 100644 index 00000000000..9d8248b2335 --- /dev/null +++ b/components/support/nimbus-fml/src/lints/documentation.rs @@ -0,0 +1,240 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +//! Lints about descriptions, which are what an experiment owner reads in +//! Experimenter when deciding what to put in a branch. + +use super::{ + enum_path, enum_variant_path, feature_path, object_field_path, object_path, variable_path, + Location, RawFinding, +}; +use crate::intermediate_representation::{EnumDef, FeatureDef, ObjectDef}; + +define_lints! { + MISSING_DESCRIPTION: Documentation, Warning = + "Everything in a manifest should have a description.", + "Descriptions are shown to experiment owners in Experimenter: say what the thing is for, and what changes when it changes."; + TERSE_DESCRIPTION: Documentation, Warning = + "Descriptions should say more than the name already does.", + "The name is already visible next to the description; use the description to say what changes when the value changes."; + TODO_IN_DESCRIPTION: Documentation, Warning = + "Descriptions shouldn't be left as placeholders.", + "Placeholder descriptions ship to Experimenter as-is."; +} + +/// Shorter than this is almost certainly a restatement of the name. +const MINIMUM_WORDS: usize = 3; + +const PLACEHOLDERS: &[&str] = &["todo", "fixme", "tbd", "xxx", "wip"]; + +pub(crate) fn check_feature(feature: &FeatureDef, out: &mut Vec) { + check_description( + "feature", + &feature.name, + &feature.metadata.description, + feature_path(feature), + out, + ); + for prop in &feature.props { + check_description( + "variable", + &prop.name, + &prop.doc, + variable_path(feature, prop), + out, + ); + } +} + +pub(crate) fn check_object(object: &ObjectDef, out: &mut Vec) { + check_description( + "object", + &object.name, + &object.doc, + object_path(object), + out, + ); + for prop in &object.props { + check_description( + "field", + &prop.name, + &prop.doc, + object_field_path(object, prop), + out, + ); + } +} + +pub(crate) fn check_enum(enum_def: &EnumDef, out: &mut Vec) { + check_description( + "enum", + &enum_def.name, + &enum_def.doc, + enum_path(enum_def), + out, + ); + for variant in &enum_def.variants { + check_description( + "variant", + &variant.name, + &variant.doc, + enum_variant_path(enum_def, &variant.name), + out, + ); + } +} + +fn check_description( + what: &str, + name: &str, + description: &str, + path: Location, + out: &mut Vec, +) { + let description = description.trim(); + + // Findings are grouped under their feature, object or enum, so only members + // have to name themselves. + let subject = if path.is_member() { + format!("`{name}`") + } else { + format!("this {what}") + }; + + if description.is_empty() { + out.push(RawFinding::new( + &MISSING_DESCRIPTION, + path, + format!("{subject} has no description"), + )); + return; + } + + if let Some(placeholder) = placeholder_in(description) { + out.push(RawFinding::new( + &TODO_IN_DESCRIPTION, + path.clone(), + format!("The description of {subject} is still marked `{placeholder}`"), + )); + } + + let words = description.split_whitespace().count(); + if words < MINIMUM_WORDS { + out.push(RawFinding::new( + &TERSE_DESCRIPTION, + path, + format!( + "The description of {subject} is only {words} word{}: `{description}`", + if words == 1 { "" } else { "s" } + ), + )); + } else if restates_name(name, description) { + out.push(RawFinding::new( + &TERSE_DESCRIPTION, + path, + format!("The description of {subject} just restates its name"), + )); + } +} + +fn placeholder_in(description: &str) -> Option<&'static str> { + let words: Vec = description + .split(|c: char| !c.is_alphanumeric()) + .map(str::to_ascii_lowercase) + .collect(); + PLACEHOLDERS + .iter() + .find(|p| words.iter().any(|w| w == *p)) + .copied() +} + +/// Is the description the name with the punctuation, and maybe an article, removed? +fn restates_name(name: &str, description: &str) -> bool { + normalize(name) == normalize(description) +} + +fn normalize(value: &str) -> String { + let words: Vec = value + .split(|c: char| !c.is_alphanumeric()) + .filter(|s| !s.is_empty()) + .map(str::to_ascii_lowercase) + .collect(); + + let words = match words.split_first() { + Some((first, rest)) if ["a", "an", "the"].contains(&first.as_str()) => rest, + _ => &words, + }; + + words.join(" ") +} + +#[cfg(test)] +mod unit_tests { + use super::*; + use crate::intermediate_representation::FeatureDef; + + fn test_location() -> Location { + feature_path(&FeatureDef::new("a-feature", "", Default::default(), false)) + } + + fn lints(name: &str, description: &str) -> Vec<&'static str> { + let mut out = Vec::new(); + check_description("variable", name, description, test_location(), &mut out); + out.iter().map(|f| f.lint.name).collect() + } + + #[test] + fn test_missing_description() { + assert_eq!(lints("my-variable", ""), vec!["MISSING_DESCRIPTION"]); + assert_eq!(lints("my-variable", " "), vec!["MISSING_DESCRIPTION"]); + } + + #[test] + fn test_good_description() { + assert!(lints( + "max-rows", + "The largest number of rows the widget can grow to." + ) + .is_empty()); + } + + #[test] + fn test_terse_description() { + assert_eq!(lints("enabled", "Enabled"), vec!["TERSE_DESCRIPTION"]); + assert_eq!( + lints("section-order", "The section order"), + vec!["TERSE_DESCRIPTION"] + ); + } + + #[test] + fn test_description_restating_the_name() { + assert_eq!( + lints("max-visible-rows", "Max visible rows"), + vec!["TERSE_DESCRIPTION"] + ); + assert_eq!( + lints("max-visible-rows", "max_visible_rows."), + vec!["TERSE_DESCRIPTION"] + ); + assert!(lints("max-visible-rows", "How many rows are visible at once.").is_empty()); + } + + #[test] + fn test_placeholder_description() { + assert_eq!( + lints("my-variable", "TODO: write this description"), + vec!["TODO_IN_DESCRIPTION"] + ); + assert_eq!( + lints( + "my-variable", + "The colour of the button (FIXME: which one?)" + ), + vec!["TODO_IN_DESCRIPTION"] + ); + // `todos` is a word, not a placeholder. + assert!(lints("my-variable", "The list of todos shown in the widget.").is_empty()); + } +} diff --git a/components/support/nimbus-fml/src/lints/metadata.rs b/components/support/nimbus-fml/src/lints/metadata.rs new file mode 100644 index 00000000000..d9c5e94faee --- /dev/null +++ b/components/support/nimbus-fml/src/lints/metadata.rs @@ -0,0 +1,153 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +//! Lints about a feature's metadata: who owns it, where it's documented, and where +//! to file bugs against it. + +use super::{feature_path, RawFinding}; +use crate::intermediate_representation::FeatureDef; + +define_lints! { + MISSING_META_BUG: Metadata, Warning = + "Features should say where bugs against them are filed.", + "Add a `meta-bug` URL, so that QA and experiment owners know where to file issues with the feature."; + MISSING_DOCUMENTATION: Metadata, Warning = + "Features should link to at least one document describing them.", + "Add a `documentation` list of named URLs, e.g. user docs, QA docs or the feature's design document."; + MISSING_CONTACTS: Metadata, Warning = + "Features should name at least one person to ask about them.", + "Add a `contacts` list of one or more email addresses (with Mozilla Jira accounts), so that questions about the feature reach someone who can answer them."; + INVALID_CONTACT: Metadata, Warning = + "Contacts should be email addresses.", + "Contacts are used to route QA questions, so they need to be addresses that can be written to."; +} + +pub(crate) fn check_feature(feature: &FeatureDef, out: &mut Vec) { + let metadata = &feature.metadata; + let path = feature_path(feature); + + if metadata.meta_bug.is_none() { + out.push(RawFinding::new( + &MISSING_META_BUG, + path.clone(), + "No `meta-bug`", + )); + } + + if metadata.documentation.is_empty() { + out.push(RawFinding::new( + &MISSING_DOCUMENTATION, + path.clone(), + "No `documentation`", + )); + } + + if metadata.contacts.is_empty() { + out.push(RawFinding::new( + &MISSING_CONTACTS, + path.clone(), + "No `contacts`", + )); + } + + for contact in &metadata.contacts { + if !is_email_address(contact) { + out.push(RawFinding::new( + &INVALID_CONTACT, + path.clone(), + format!("`{contact}` doesn't look like an email address"), + )); + } + } +} + +/// Attempts to filter team names and handles, it doesn't actually validate +/// addresses. +fn is_email_address(contact: &str) -> bool { + if contact.trim() != contact || contact.chars().any(char::is_whitespace) { + return false; + } + let mut parts = contact.split('@'); + match (parts.next(), parts.next(), parts.next()) { + (Some(local), Some(domain), None) => { + !local.is_empty() + && domain.contains('.') + && !domain.starts_with('.') + && !domain.ends_with('.') + } + _ => false, + } +} + +#[cfg(test)] +mod unit_tests { + use super::*; + use crate::frontend::DocumentationLink; + use std::str::FromStr; + use url::Url; + + fn feature() -> FeatureDef { + FeatureDef::new("my-feature", "A description", Default::default(), false) + } + + fn lints(feature: &FeatureDef) -> Vec<&'static str> { + let mut out = Vec::new(); + check_feature(feature, &mut out); + out.iter().map(|f| f.lint.name).collect() + } + + #[test] + fn test_empty_metadata() { + assert_eq!( + lints(&feature()), + vec![ + "MISSING_META_BUG", + "MISSING_DOCUMENTATION", + "MISSING_CONTACTS" + ] + ); + } + + #[test] + fn test_complete_metadata() -> crate::error::Result<()> { + let mut feature = feature(); + feature.metadata.meta_bug = Some(Url::from_str("https://example.com/EXP-23")?); + feature.metadata.contacts = vec!["jdoe@example.com".to_string()]; + feature.metadata.documentation = vec![DocumentationLink { + name: "User documentation".to_string(), + url: Url::from_str("https://example.info/my-feature")?, + }]; + + assert!(lints(&feature).is_empty()); + Ok(()) + } + + #[test] + fn test_contacts_that_arent_addresses() { + let mut feature = feature(); + feature.metadata.contacts = vec!["the nimbus team".to_string()]; + + assert!(lints(&feature).contains(&"INVALID_CONTACT")); + } + + #[test] + fn test_is_email_address() { + for ok in ["jdoe@example.com", "j.doe+nimbus@example.co.uk"] { + assert!(is_email_address(ok), "{ok} should be an address"); + } + for not_ok in [ + "jdoe", + "the nimbus team", + "jdoe@example", + "@example.com", + "jdoe@@example.com", + " jdoe@example.com", + ] { + assert!( + !is_email_address(not_ok), + "{not_ok} shouldn't be an address" + ); + } + } +} diff --git a/components/support/nimbus-fml/src/lints/mod.rs b/components/support/nimbus-fml/src/lints/mod.rs new file mode 100644 index 00000000000..ff168c56f99 --- /dev/null +++ b/components/support/nimbus-fml/src/lints/mod.rs @@ -0,0 +1,675 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +//! Custom lints for Nimbus feature manifests. +//! +//! Where validation asks whether a manifest can generate working code, these ask +//! whether the feature is one an experiment owner can work with. They never stop +//! code generation, and they're reported by `nimbus-fml lint` alone. +//! +//! A lint can be silenced by a `no-lint` list on a feature, a top level `no-lint` +//! list, or `--allow`/`--deny`. + +/// Declares a [`LintInfo`] static per lint plus a `LINTS` slice of them, which is +/// what [`ALL_LINTS`] is assembled from, so declaring a lint registers it. +/// +/// ```ignore +/// define_lints! { +/// MISSING_META_BUG: Metadata, Warning = +/// "Features should say where to file bugs.", +/// "Add a `meta-bug` URL."; +/// } +/// ``` +/// +/// Must stay above the `mod` declarations below, which is what puts it in scope for +/// them. +macro_rules! define_lints { + ($($name:ident: $category:ident, $level:ident = $description:literal, $help:literal;)*) => { + $( + pub static $name: $crate::lints::LintInfo = $crate::lints::LintInfo { + name: stringify!($name), + description: $description, + help: $help, + category: $crate::lints::LintCategory::$category, + default_level: $crate::lints::LintLevel::$level, + }; + )* + + /// Every lint declared in this module. + pub static LINTS: &[&$crate::lints::LintInfo] = &[$(&$name),*]; + }; +} + +mod design; +mod documentation; +mod metadata; +mod naming; + +use std::{ + cmp::Reverse, + collections::{BTreeMap, BTreeSet, HashSet}, +}; + +use serde::Serialize; + +use crate::{ + error::{FMLError, Result}, + intermediate_representation::{EnumDef, FeatureDef, FeatureManifest, ObjectDef, PropDef}, +}; + +define_lints! { + UNKNOWN_LINT: Lints, Warning = + "A `no-lint` entry names a lint that doesn't exist.", + "Run `nimbus-fml lint --list` to see the available lints."; +} + +/// What a lint is about, as reported by `nimbus-fml lint --list`. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] +#[serde(rename_all = "kebab-case")] +pub enum LintCategory { + Metadata, + Documentation, + Naming, + Design, + /// Lints about the lints themselves. + Lints, +} + +impl LintCategory { + pub fn as_str(&self) -> &'static str { + match self { + Self::Metadata => "metadata", + Self::Documentation => "documentation", + Self::Naming => "naming", + Self::Design => "design", + Self::Lints => "lints", + } + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Serialize)] +#[serde(rename_all = "lowercase")] +pub enum LintLevel { + /// Switched off; findings are discarded. + Allow, + /// Reported, but doesn't fail the run. + Warning, + /// Reported, and fails the run. + Error, +} + +impl LintLevel { + pub fn as_str(&self) -> &'static str { + match self { + Self::Allow => "allow", + Self::Warning => "warning", + Self::Error => "error", + } + } +} + +#[derive(Debug)] +pub struct LintInfo { + pub name: &'static str, + pub description: &'static str, + /// What to do about it. Shown once per report, not once per finding. + pub help: &'static str, + pub category: LintCategory, + pub default_level: LintLevel, +} + +lazy_static::lazy_static! { + /// Every lint, in `--list` order. + pub static ref ALL_LINTS: Vec<&'static LintInfo> = metadata::LINTS + .iter() + .chain(documentation::LINTS) + .chain(naming::LINTS) + .chain(design::LINTS) + .chain(LINTS) + .copied() + .collect(); +} + +pub fn find_lint(name: &str) -> Option<&'static LintInfo> { + ALL_LINTS.iter().find(|l| l.name == name).copied() +} + +/// Which lints run, and how loudly. +#[derive(Debug, Clone, Default)] +pub struct LintConfig { + levels: BTreeMap<&'static str, LintLevel>, + file_suppressions: BTreeSet, + include_imports: bool, +} + +impl LintConfig { + pub fn new() -> Self { + Default::default() + } + + pub fn allowing(self, names: &[String]) -> Result { + self.with_level(names, LintLevel::Allow) + } + + pub fn denying(self, names: &[String]) -> Result { + self.with_level(names, LintLevel::Error) + } + + fn with_level(mut self, names: &[String], level: LintLevel) -> Result { + for name in names { + let lint = find_lint(name).ok_or_else(|| { + FMLError::CliError(format!( + "`{name}` isn't a lint. Run `nimbus-fml lint --list` to see the available lints" + )) + })?; + self.levels.insert(lint.name, level); + } + Ok(self) + } + + /// Lints named by a top level `no-lint` block. + pub fn with_file_suppressions(mut self, names: &[String]) -> Self { + self.file_suppressions = names.iter().cloned().collect(); + self + } + + pub fn including_imports(mut self, include_imports: bool) -> Self { + self.include_imports = include_imports; + self + } + + fn level_for(&self, lint: &'static LintInfo) -> LintLevel { + *self.levels.get(lint.name).unwrap_or(&lint.default_level) + } +} + +/// Where in the manifest a finding is. The `subject` is what findings are grouped +/// under when reported, so it is the feature, object or enum, never a member of one. +#[derive(Debug, Clone)] +pub(crate) struct Location { + subject: String, + member: Option, +} + +impl Location { + fn subject(subject: String) -> Self { + Self { + subject, + member: None, + } + } + + fn member(subject: String, member: String) -> Self { + Self { + subject, + member: Some(member), + } + } + + pub(crate) fn is_member(&self) -> bool { + self.member.is_some() + } +} + +/// A finding before the runner has decided whether to report it. +#[derive(Debug, Clone)] +pub(crate) struct RawFinding { + lint: &'static LintInfo, + location: Location, + message: String, +} + +impl RawFinding { + pub(crate) fn new( + lint: &'static LintInfo, + location: Location, + message: impl Into, + ) -> RawFinding { + Self { + lint, + location, + message: message.into(), + } + } +} + +#[derive(Debug, Clone, Serialize)] +pub struct Finding { + pub lint: &'static str, + pub level: LintLevel, + /// Set when the finding came from an imported manifest. + #[serde(skip_serializing_if = "Option::is_none")] + pub module: Option, + /// e.g. ``feature `homescreen` ``. + pub subject: String, + /// e.g. ``variable `enabled` ``. + #[serde(skip_serializing_if = "Option::is_none")] + pub member: Option, + pub message: String, +} + +#[derive(Debug, Clone, Default)] +pub struct LintReport { + pub findings: Vec, + /// How many findings a `no-lint` block silenced that would otherwise have been + /// reported, so that a manifest can't opt out of everything and look clean. + pub suppressed: usize, +} + +impl LintReport { + pub fn is_empty(&self) -> bool { + self.findings.is_empty() + } + + pub fn error_count(&self) -> usize { + self.count_of(LintLevel::Error) + } + + pub fn warning_count(&self) -> usize { + self.count_of(LintLevel::Warning) + } + + fn count_of(&self, level: LintLevel) -> usize { + self.findings.iter().filter(|f| f.level == level).count() + } + + /// How many features, objects and enums have findings. + pub fn subject_count(&self) -> usize { + self.findings + .iter() + .map(|f| (&f.module, &f.subject)) + .collect::>() + .len() + } + + /// The lints that fired, in registration order. + pub fn triggered_lints(&self) -> Vec<&'static LintInfo> { + let fired: HashSet<_> = self.findings.iter().map(|f| f.lint).collect(); + ALL_LINTS + .iter() + .filter(|l| fired.contains(l.name)) + .copied() + .collect() + } +} + +/// Run the lints against a manifest that has already been through +/// [`FeatureManifest::validate_manifest`]. +pub fn lint_manifest(fm: &FeatureManifest, config: &LintConfig) -> LintReport { + let mut report = LintReport::default(); + + // The top level `no-lint` list belongs to the file rather than to any one + // feature, so it is checked once instead of per module. + let mut raw = Vec::new(); + check_no_lint_names( + config.file_suppressions.iter().map(String::as_str), + manifest_path(), + &mut raw, + ); + collect(raw, config, &HashSet::new(), &None, &mut report); + + lint_module(fm, config, None, &mut report); + + if config.include_imports { + for (id, child) in &fm.all_imports { + lint_module(child, config, Some(id.to_string()), &mut report); + } + } + + // Group by location, worst first, so a feature's findings are reported together. + report.findings.sort_by(|a, b| { + (&a.module, &a.subject, Reverse(a.level), a.lint, &a.member).cmp(&( + &b.module, + &b.subject, + Reverse(b.level), + b.lint, + &b.member, + )) + }); + + report +} + +fn lint_module( + fm: &FeatureManifest, + config: &LintConfig, + module: Option, + out: &mut LintReport, +) { + for feature in fm.iter_feature_defs() { + let mut raw = Vec::new(); + metadata::check_feature(feature, &mut raw); + documentation::check_feature(feature, &mut raw); + naming::check_feature(feature, &mut raw); + design::check_feature(feature, fm, &mut raw); + check_no_lint_names( + feature.metadata.no_lint.iter().map(String::as_str), + feature_path(feature), + &mut raw, + ); + + let suppressions: HashSet<&str> = feature + .metadata + .no_lint + .iter() + .map(String::as_str) + .collect(); + collect(raw, config, &suppressions, &module, out); + } + + let no_suppressions = HashSet::new(); + + for object in fm.iter_object_defs() { + let mut raw = Vec::new(); + documentation::check_object(object, &mut raw); + naming::check_object(object, &mut raw); + collect(raw, config, &no_suppressions, &module, out); + } + + for enum_def in fm.iter_enum_defs() { + let mut raw = Vec::new(); + documentation::check_enum(enum_def, &mut raw); + naming::check_enum(enum_def, &mut raw); + design::check_enum(enum_def, &mut raw); + collect(raw, config, &no_suppressions, &module, out); + } + + let mut raw = Vec::new(); + design::check_manifest(fm, &mut raw); + collect(raw, config, &no_suppressions, &module, out); +} + +fn collect( + raw: Vec, + config: &LintConfig, + suppressions: &HashSet<&str>, + module: &Option, + out: &mut LintReport, +) { + for finding in raw { + let name = finding.lint.name; + + let level = config.level_for(finding.lint); + if level == LintLevel::Allow { + continue; + } + + // Counted so the summary can report it, but only once the lint is known to + // be one that would otherwise have been shown. + if suppressions.contains(name) || config.file_suppressions.contains(name) { + out.suppressed += 1; + continue; + } + + out.findings.push(Finding { + lint: name, + level, + module: module.clone(), + subject: finding.location.subject, + member: finding.location.member, + message: finding.message, + }); + } +} + +fn check_no_lint_names<'a>( + names: impl IntoIterator, + location: Location, + out: &mut Vec, +) { + for name in names { + if find_lint(name).is_none() { + out.push(RawFinding::new( + &UNKNOWN_LINT, + location.clone(), + format!("`no-lint` names `{name}`, which isn't a lint"), + )); + } + } +} + +pub(crate) fn manifest_path() -> Location { + Location::subject("this manifest".to_string()) +} + +pub(crate) fn feature_path(feature: &FeatureDef) -> Location { + Location::subject(format!("feature `{}`", feature.name)) +} + +pub(crate) fn variable_path(feature: &FeatureDef, prop: &PropDef) -> Location { + Location::member( + format!("feature `{}`", feature.name), + format!("variable `{}`", prop.name), + ) +} + +pub(crate) fn object_path(object: &ObjectDef) -> Location { + Location::subject(format!("object `{}`", object.name)) +} + +pub(crate) fn object_field_path(object: &ObjectDef, prop: &PropDef) -> Location { + Location::member( + format!("object `{}`", object.name), + format!("field `{}`", prop.name), + ) +} + +pub(crate) fn enum_path(enum_def: &EnumDef) -> Location { + Location::subject(format!("enum `{}`", enum_def.name)) +} + +pub(crate) fn enum_variant_path(enum_def: &EnumDef, variant: &str) -> Location { + Location::member( + format!("enum `{}`", enum_def.name), + format!("variant `{variant}`"), + ) +} + +#[cfg(test)] +mod unit_tests { + use super::*; + use crate::{ + error::Result, + intermediate_representation::{FeatureDef, PropDef, TypeRef}, + }; + use serde_json::json; + + #[test] + fn test_lint_names_are_unique_and_well_formed() { + let mut seen = HashSet::new(); + for lint in ALL_LINTS.iter() { + assert!( + seen.insert(lint.name), + "{} is registered more than once", + lint.name + ); + assert!( + lint.name + .chars() + .all(|c| c.is_ascii_uppercase() || c.is_ascii_digit() || c == '_'), + "{} isn't SCREAMING_SNAKE_CASE", + lint.name + ); + assert!( + lint.description.ends_with('.'), + "{}'s description should be a sentence", + lint.name + ); + } + } + + /// No metadata, no description, no variables. + fn empty_feature(name: &str) -> FeatureDef { + FeatureDef::new(name, "", Default::default(), false) + } + + #[test] + fn test_every_lint_is_registered() { + for lint in [ + &metadata::MISSING_META_BUG, + &documentation::MISSING_DESCRIPTION, + &naming::FEATURE_NAME_CASING, + &design::UNUSED_TYPE, + &UNKNOWN_LINT, + ] { + assert_eq!(find_lint(lint.name).map(|l| l.name), Some(lint.name)); + } + } + + #[test] + fn test_suppressed_findings_are_counted() { + let mut suppressed = empty_feature("suppressed"); + suppressed.metadata.no_lint = vec!["NO_VARIABLES".to_string()]; + + let mut fm = FeatureManifest::default(); + fm.add_feature(suppressed); + + let report = lint_manifest(&fm, &LintConfig::new()); + assert_eq!(report.suppressed, 1); + + // `--allow` isn't counted. + let config = LintConfig::new() + .allowing(&["NO_VARIABLES".to_string()]) + .unwrap(); + let mut fm = FeatureManifest::default(); + fm.add_feature(empty_feature("plain")); + assert_eq!(lint_manifest(&fm, &config).suppressed, 0); + } + + #[test] + fn test_findings_are_reported_once_per_lint() { + let mut fm = FeatureManifest::default(); + fm.add_feature(empty_feature("my-feature")); + + let report = lint_manifest(&fm, &LintConfig::new()); + let mut lints: Vec<_> = report.findings.iter().map(|f| f.lint).collect(); + lints.sort_unstable(); + let deduped = lints.iter().collect::>(); + assert_eq!(lints.len(), deduped.len()); + + assert!(lints.contains(&"NO_VARIABLES")); + assert!(lints.contains(&"MISSING_DESCRIPTION")); + assert!(lints.contains(&"MISSING_CONTACTS")); + } + + #[test] + fn test_allow_switches_a_lint_off() -> Result<()> { + let mut fm = FeatureManifest::default(); + fm.add_feature(empty_feature("my-feature")); + + let config = LintConfig::new().allowing(&["NO_VARIABLES".to_string()])?; + let report = lint_manifest(&fm, &config); + assert!(!report.findings.iter().any(|f| f.lint == "NO_VARIABLES")); + + Ok(()) + } + + #[test] + fn test_deny_makes_a_lint_an_error() -> Result<()> { + let mut fm = FeatureManifest::default(); + fm.add_feature(empty_feature("my-feature")); + + let config = LintConfig::new().denying(&["NO_VARIABLES".to_string()])?; + let report = lint_manifest(&fm, &config); + let finding = report + .findings + .iter() + .find(|f| f.lint == "NO_VARIABLES") + .expect("NO_VARIABLES should be reported"); + assert_eq!(finding.level, LintLevel::Error); + assert_eq!(report.error_count(), 1); + + Ok(()) + } + + #[test] + fn test_unknown_lint_names_are_rejected_on_the_command_line() { + let err = LintConfig::new() + .allowing(&["NOT_A_LINT".to_string()]) + .expect_err("An unknown lint name should be an error"); + assert!(err.to_string().contains("NOT_A_LINT")); + } + + #[test] + fn test_file_suppressions() { + let mut fm = FeatureManifest::default(); + fm.add_feature(empty_feature("my-feature")); + + let config = LintConfig::new().with_file_suppressions(&["NO_VARIABLES".to_string()]); + let report = lint_manifest(&fm, &config); + assert!(!report.findings.iter().any(|f| f.lint == "NO_VARIABLES")); + } + + #[test] + fn test_feature_suppressions() { + let mut suppressed = empty_feature("suppressed"); + suppressed.metadata.no_lint = vec!["NO_VARIABLES".to_string()]; + + let mut fm = FeatureManifest::default(); + fm.add_feature(suppressed); + fm.add_feature(empty_feature("not-suppressed")); + + let report = lint_manifest(&fm, &LintConfig::new()); + let features: Vec<_> = report + .findings + .iter() + .filter(|f| f.lint == "NO_VARIABLES") + .map(|f| f.subject.as_str()) + .collect(); + assert_eq!(features, vec!["feature `not-suppressed`"]); + } + + #[test] + fn test_unknown_lint_names_in_the_manifest_are_reported() { + let mut feature = empty_feature("my-feature"); + feature.metadata.no_lint = vec!["NOT_A_LINT".to_string()]; + + let mut fm = FeatureManifest::default(); + fm.add_feature(feature); + + let report = lint_manifest(&fm, &LintConfig::new()); + assert!(report + .findings + .iter() + .any(|f| f.lint == "UNKNOWN_LINT" && f.message.contains("NOT_A_LINT"))); + } + + #[test] + fn test_a_well_formed_feature_is_quiet() { + let feature = FeatureDef::new( + "my-feature", + "Controls the shape and behaviour of the widget on the home screen.", + vec![ + PropDef::with_doc( + "enabled", + "Whether the widget is shown on the home screen at all.", + &TypeRef::Boolean, + &json!(false), + ), + PropDef::with_doc( + "max-rows", + "The largest number of rows the widget is allowed to grow to.", + &TypeRef::Int, + &json!(3), + ), + ], + false, + ); + let mut fm = FeatureManifest::default(); + fm.add_feature(feature); + + // Metadata lints are expected; nothing else should fire. + let config = LintConfig::new().allowing(&[ + "MISSING_META_BUG".to_string(), + "MISSING_DOCUMENTATION".to_string(), + "MISSING_CONTACTS".to_string(), + ]); + let report = lint_manifest(&fm, &config.unwrap()); + assert!( + report.is_empty(), + "unexpected findings: {:?}", + report.findings + ); + } +} diff --git a/components/support/nimbus-fml/src/lints/naming.rs b/components/support/nimbus-fml/src/lints/naming.rs new file mode 100644 index 00000000000..093b7ec6457 --- /dev/null +++ b/components/support/nimbus-fml/src/lints/naming.rs @@ -0,0 +1,451 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +//! Lints about names. Feature ids and variable names are typed by hand into +//! Experimenter branches, so they should be predictable. + +use lazy_static::lazy_static; +use regex::Regex; + +use super::{ + enum_path, enum_variant_path, feature_path, object_field_path, object_path, variable_path, + Location, RawFinding, +}; +use crate::intermediate_representation::{EnumDef, FeatureDef, ObjectDef, PropDef, TypeRef}; + +define_lints! { + FEATURE_NAME_CASING: Naming, Warning = + "Feature ids should be kebab-case.", + "Feature ids are typed by hand into Experimenter."; + VARIABLE_NAME_CASING: Naming, Warning = + "Variable and field names should be kebab-case.", + "Variables are written out as JSON keys in Experimenter."; + TYPE_NAME_CASING: Naming, Warning = + "Objects and enums should be UpperCamelCase.", + "Objects and enums become classes and types in the generated Kotlin and Swift."; + ENUM_VARIANT_CASING: Naming, Warning = + "Enum variants should be kebab-case.", + "Variants are written out as JSON strings in Experimenter."; + COMMON_PREFIX: Naming, Warning = + "Variables shouldn't repeat the name of the feature they belong to.", + "Variables are always read together with the feature they belong to, so a shared prefix only makes them longer. Drop it, or group the variables into an object if the prefix is really a thing in its own right."; + TYPE_IN_NAME: Naming, Warning = + "Variable names shouldn't repeat the name of their type.", + "The type is shown next to the name everywhere the name is; repeating it only makes the name longer."; + NEGATED_BOOLEAN: Naming, Warning = + "Booleans should be named for what is true, not what is false.", + "Everyone setting this in an experiment has to work out what `false` means. Name the variable for the behaviour that is switched on, and flip the default."; +} + +lazy_static! { + static ref KEBAB_CASE: Regex = Regex::new(r"^[a-z][a-z0-9]*(-[a-z0-9]+)*$").unwrap(); + static ref UPPER_CAMEL_CASE: Regex = Regex::new(r"^[A-Z][A-Za-z0-9]*$").unwrap(); +} + +/// Words that read as a predicate rather than a namespace, so sharing one across a +/// feature's variables is a convention, not something to group into an object. +const PREDICATE_WORDS: &[&str] = &["allow", "can", "has", "is", "should", "show", "use"]; + +/// Words that make `false` mean the thing happens. +const NEGATIVE_WORDS: &[&str] = &[ + "disable", + "disabled", + "disallow", + "disallowed", + "dont", + "hidden", + "hide", + "never", + "no", + "not", + "prevent", + "suppress", +]; + +pub(crate) fn check_feature(feature: &FeatureDef, out: &mut Vec) { + if !KEBAB_CASE.is_match(&feature.name) { + out.push(RawFinding::new( + &FEATURE_NAME_CASING, + feature_path(feature), + format!( + "`{}` isn't kebab-case{}", + feature.name, + rename_to(&feature.name) + ), + )); + } + + for prop in &feature.props { + check_variable_name(&prop.name, variable_path(feature, prop), out); + check_type_in_name(prop, variable_path(feature, prop), out); + check_negated_boolean(prop, variable_path(feature, prop), out); + } + + check_common_prefix(feature, out); +} + +pub(crate) fn check_object(object: &ObjectDef, out: &mut Vec) { + if !UPPER_CAMEL_CASE.is_match(&object.name) { + out.push(RawFinding::new( + &TYPE_NAME_CASING, + object_path(object), + format!("`{}` isn't UpperCamelCase", object.name), + )); + } + + for prop in &object.props { + check_variable_name(&prop.name, object_field_path(object, prop), out); + check_type_in_name(prop, object_field_path(object, prop), out); + check_negated_boolean(prop, object_field_path(object, prop), out); + } +} + +pub(crate) fn check_enum(enum_def: &EnumDef, out: &mut Vec) { + if !UPPER_CAMEL_CASE.is_match(&enum_def.name) { + out.push(RawFinding::new( + &TYPE_NAME_CASING, + enum_path(enum_def), + format!("`{}` isn't UpperCamelCase", enum_def.name), + )); + } + + for variant in &enum_def.variants { + if !KEBAB_CASE.is_match(&variant.name) { + out.push(RawFinding::new( + &ENUM_VARIANT_CASING, + enum_variant_path(enum_def, &variant.name), + format!( + "`{}` isn't kebab-case{}", + variant.name, + rename_to(&variant.name) + ), + )); + } + } +} + +fn check_variable_name(name: &str, path: Location, out: &mut Vec) { + if !KEBAB_CASE.is_match(name) { + out.push(RawFinding::new( + &VARIABLE_NAME_CASING, + path, + format!("`{name}` isn't kebab-case{}", rename_to(name)), + )); + } +} + +/// `sections-list: List
` says "list" twice. +fn check_type_in_name(prop: &PropDef, path: Location, out: &mut Vec) { + let Some((_, last)) = prop.name.rsplit_once('-') else { + return; + }; + let last = last.to_ascii_lowercase(); + + // As far as the name goes, `Option` is still a boolean. + let typ = match &prop.typ { + TypeRef::Option(inner) => inner.as_ref(), + typ => typ, + }; + + let redundant = match typ { + TypeRef::Boolean => ["bool", "boolean", "flag"].contains(&last.as_str()), + TypeRef::Int => ["int", "integer", "num", "number"].contains(&last.as_str()), + TypeRef::String => ["str", "string"].contains(&last.as_str()), + TypeRef::List(_) => ["list", "array"].contains(&last.as_str()), + TypeRef::StringMap(_) | TypeRef::EnumMap(..) => { + ["map", "dict", "dictionary"].contains(&last.as_str()) + } + TypeRef::Enum(_) => last == "enum", + TypeRef::Object(_) => ["obj", "object", "json"].contains(&last.as_str()), + _ => false, + }; + + if redundant { + out.push(RawFinding::new( + &TYPE_IN_NAME, + path, + format!( + "`{}` ends in `-{last}`, but its type is already `{}`", + prop.name, prop.typ + ), + )); + } +} + +/// `disable-sync: false` takes a moment to read; `sync-enabled: true` doesn't. +fn check_negated_boolean(prop: &PropDef, path: Location, out: &mut Vec) { + if !is_boolean(&prop.typ) { + return; + } + + let negative = prop + .name + .split('-') + .find(|word| NEGATIVE_WORDS.contains(&word.to_ascii_lowercase().as_str())); + + if let Some(word) = negative { + out.push(RawFinding::new( + &NEGATED_BOOLEAN, + path, + format!("`{}` is a boolean named with `{word}`", prop.name), + )); + } +} + +/// A feature called `homescreen` doesn't need variables called `homescreen-*`. +fn check_common_prefix(feature: &FeatureDef, out: &mut Vec) { + let feature_prefix = format!("{}-", feature.name); + let mut prefixed = Vec::new(); + + for prop in &feature.props { + if prop.name.starts_with(&feature_prefix) { + prefixed.push(prop); + } + } + + for prop in &prefixed { + out.push(RawFinding::new( + &COMMON_PREFIX, + variable_path(feature, prop), + format!( + "`{}` repeats the name of the feature it belongs to; rename it to `{}`", + prop.name, + &prop.name[feature_prefix.len()..] + ), + )); + } + + // A prefix shared by every variable is noise even when it isn't the feature name. + if prefixed.is_empty() && feature.props.len() > 1 { + if let Some(shared) = shared_prefix(&feature.props) { + out.push(RawFinding::new( + &COMMON_PREFIX, + feature_path(feature), + format!( + "All {} variables of this feature start with `{shared}-`", + feature.props.len() + ), + )); + } + } +} + +fn shared_prefix(props: &[PropDef]) -> Option { + let first = props.first()?.name.split('-').next()?.to_string(); + if PREDICATE_WORDS.contains(&first.as_str()) { + return None; + } + props + .iter() + .all(|p| { + p.name + .split_once('-') + .map(|(head, _)| head == first) + .unwrap_or_default() + }) + .then_some(first) +} + +fn is_boolean(typ: &TypeRef) -> bool { + match typ { + TypeRef::Boolean => true, + TypeRef::Option(inner) => is_boolean(inner), + _ => false, + } +} + +/// The kebab-case of `name`, unless that is what `name` already is. Names the regex +/// rejects but `heck` can't improve, like `9lives`, have nothing to suggest. +fn rename_to(name: &str) -> String { + use heck::ToKebabCase; + let kebab = name.to_kebab_case(); + if kebab == name { + String::new() + } else { + format!("; rename it to `{kebab}`") + } +} + +#[cfg(test)] +mod unit_tests { + use super::*; + use serde_json::json; + + fn prop(name: &str, typ: &TypeRef) -> PropDef { + PropDef::with_doc(name, "A description of the variable.", typ, &json!(null)) + } + + fn feature(name: &str, props: Vec) -> FeatureDef { + FeatureDef::new(name, "A description of the feature.", props, false) + } + + fn lints(feature: &FeatureDef) -> Vec<&'static str> { + let mut out = Vec::new(); + check_feature(feature, &mut out); + out.iter().map(|f| f.lint.name).collect() + } + + #[test] + fn test_casing() { + assert!(lints(&feature( + "my-feature", + vec![prop("my-variable", &TypeRef::Int)] + )) + .is_empty()); + + assert_eq!( + lints(&feature("myFeature", Default::default())), + vec!["FEATURE_NAME_CASING"] + ); + assert_eq!( + lints(&feature("my_feature", Default::default())), + vec!["FEATURE_NAME_CASING"] + ); + assert_eq!( + lints(&feature( + "my-feature", + vec![prop("myVariable", &TypeRef::Int)] + )), + vec!["VARIABLE_NAME_CASING"] + ); + } + + #[test] + fn test_casing_without_a_suggestion() { + // `heck` can't improve a name the regex rejects for starting with a digit. + let findings = lints(&feature("9lives", Default::default())); + assert_eq!(findings, vec!["FEATURE_NAME_CASING"]); + + let mut out = Vec::new(); + check_feature(&feature("9lives", Default::default()), &mut out); + assert_eq!(out[0].message, "`9lives` isn't kebab-case"); + } + + #[test] + fn test_type_name_casing() { + let mut out = Vec::new(); + check_object(&ObjectDef::new("my-object", &[]), &mut out); + check_enum(&EnumDef::new("myEnum", &["ok"]), &mut out); + let names: Vec<_> = out.iter().map(|f| f.lint.name).collect(); + assert_eq!(names, vec!["TYPE_NAME_CASING", "TYPE_NAME_CASING"]); + + let mut out = Vec::new(); + check_enum(&EnumDef::new("MyEnum", &["notKebab"]), &mut out); + let names: Vec<_> = out.iter().map(|f| f.lint.name).collect(); + assert_eq!(names, vec!["ENUM_VARIANT_CASING"]); + } + + #[test] + fn test_type_in_name() { + for (name, typ) in [ + ("enabled-bool", TypeRef::Boolean), + ("sections-list", TypeRef::List(Box::new(TypeRef::String))), + ("max-rows-int", TypeRef::Int), + ] { + assert!( + lints(&feature("my-feature", vec![prop(name, &typ)])).contains(&"TYPE_IN_NAME"), + "{name} should be flagged" + ); + } + + // Still a boolean when it's optional. + assert!(lints(&feature( + "my-feature", + vec![prop( + "enabled-bool", + &TypeRef::Option(Box::new(TypeRef::Boolean)) + )] + )) + .contains(&"TYPE_IN_NAME")); + + // The suffix is only redundant if it really is the type. + assert!(!lints(&feature( + "my-feature", + vec![prop("shopping-list", &TypeRef::String)] + )) + .contains(&"TYPE_IN_NAME")); + } + + #[test] + fn test_negated_boolean() { + for name in [ + "disable-sync", + "hide-toolbar", + "sync-disabled", + "no-onboarding", + ] { + assert!( + lints(&feature("my-feature", vec![prop(name, &TypeRef::Boolean)])) + .contains(&"NEGATED_BOOLEAN"), + "{name} should be flagged" + ); + } + + assert!(!lints(&feature( + "my-feature", + vec![prop("sync-enabled", &TypeRef::Boolean)] + )) + .contains(&"NEGATED_BOOLEAN")); + + // Only booleans read as double negatives. + assert!(!lints(&feature( + "my-feature", + vec![prop("hide-after", &TypeRef::Int)] + )) + .contains(&"NEGATED_BOOLEAN")); + } + + #[test] + fn test_common_prefix() { + let feature = feature( + "homescreen", + vec![ + prop("homescreen-enabled", &TypeRef::Boolean), + prop("homescreen-sections", &TypeRef::String), + ], + ); + let mut out = Vec::new(); + check_feature(&feature, &mut out); + let findings: Vec<_> = out + .iter() + .filter(|f| f.lint.name == "COMMON_PREFIX") + .collect(); + assert_eq!(findings.len(), 2); + assert!(findings[0].message.contains("rename it to `enabled`")); + } + + #[test] + fn test_shared_predicate_prefix_is_not_a_namespace() { + // Focus names every variable of its `onboarding` feature `is-*`; that is a + // boolean convention, not a prefix to strip. + let feature = feature( + "onboarding", + vec![ + prop("is-enabled", &TypeRef::Boolean), + prop("is-cfr-enabled", &TypeRef::Boolean), + ], + ); + assert!(!lints(&feature).contains(&"COMMON_PREFIX")); + } + + #[test] + fn test_shared_prefix_that_isnt_the_feature_name() { + let feature = feature( + "homescreen", + vec![ + prop("section-order", &TypeRef::String), + prop("section-titles", &TypeRef::String), + ], + ); + assert!(lints(&feature).contains(&"COMMON_PREFIX")); + + // One variable isn't a pattern. + let feature = feature_with_one("section-order"); + assert!(!lints(&feature).contains(&"COMMON_PREFIX")); + } + + fn feature_with_one(name: &str) -> FeatureDef { + feature("homescreen", vec![prop(name, &TypeRef::String)]) + } +} diff --git a/components/support/nimbus-fml/src/main.rs b/components/support/nimbus-fml/src/main.rs index 170e6689764..5b69dc056e7 100644 --- a/components/support/nimbus-fml/src/main.rs +++ b/components/support/nimbus-fml/src/main.rs @@ -11,6 +11,7 @@ mod error; mod fixtures; mod frontend; mod intermediate_representation; +mod lints; mod parser; mod schema; mod util; diff --git a/components/support/nimbus-fml/src/parser.rs b/components/support/nimbus-fml/src/parser.rs index cdf692b8661..e95c76bb315 100644 --- a/components/support/nimbus-fml/src/parser.rs +++ b/components/support/nimbus-fml/src/parser.rs @@ -175,11 +175,21 @@ impl Parser { let imports = self.merge_import_block_list(&parent.imports, &child.imports)?; + // The child's features are about to become the parent's, so its suppressions + // have to come with them. + let mut no_lint = parent.no_lint.clone(); + for name in &child.no_lint { + if !no_lint.contains(name) { + no_lint.push(name.clone()); + } + } + let merged = ManifestFrontEnd { features, types: Types { enums, objects }, legacy_types: None, imports, + no_lint, ..parent };