Skip to content

Commit

Permalink
Unrolled build for rust-lang#126682
Browse files Browse the repository at this point in the history
Rollup merge of rust-lang#126682 - Zalathar:coverage-attr, r=lcnr

coverage: Overhaul validation of the `#[coverage(..)]` attribute

This PR makes sweeping changes to how the (currently-unstable) coverage attribute is validated:
- Multiple coverage attributes on the same item/expression are now treated as an error.
- The attribute must always be `#[coverage(off)]` or `#[coverage(on)]`, and the error messages for this are more consistent.
  -  A trailing comma is still allowed after off/on, since that's part of the normal attribute syntax.
- Some places that silently ignored a coverage attribute now produce an error instead.
  - These cases were all clearly bugs.
- Some places that ignored a coverage attribute (with a warning) now produce an error instead.
  - These were originally added as lints, but I don't think it makes much sense to knowingly allow new attributes to be used in meaningless places.
  - Some of these errors might soon disappear, if it's easy to extend recursive coverage attributes to things like modules and impl blocks.

---

One of the goals of this PR is to lay a more solid foundation for making the coverage attribute recursive, so that it applies to all nested functions/closures instead of just the one it is directly attached to.

Fixes rust-lang#126658.

This PR incorporates rust-lang#126659, which adds more tests for validation of the coverage attribute.

`@rustbot` label +A-code-coverage
  • Loading branch information
rust-timer authored Jun 24, 2024
2 parents d8d5732 + 1852141 commit aafafa9
Show file tree
Hide file tree
Showing 18 changed files with 687 additions and 358 deletions.
2 changes: 0 additions & 2 deletions compiler/rustc_codegen_ssa/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ codegen_ssa_create_temp_dir = couldn't create a temp dir: {$error}
codegen_ssa_error_creating_remark_dir = failed to create remark directory: {$error}
codegen_ssa_expected_coverage_symbol = expected `coverage(off)` or `coverage(on)`
codegen_ssa_expected_used_symbol = expected `used`, `used(compiler)` or `used(linker)`
codegen_ssa_extern_funcs_not_found = some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
Expand Down
11 changes: 4 additions & 7 deletions compiler/rustc_codegen_ssa/src/codegen_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,7 @@ use rustc_span::{sym, Span};
use rustc_target::spec::{abi, SanitizerSet};

use crate::errors;
use crate::target_features::from_target_feature;
use crate::{
errors::{ExpectedCoverageSymbol, ExpectedUsedSymbol},
target_features::check_target_feature_trait_unsafe,
};
use crate::target_features::{check_target_feature_trait_unsafe, from_target_feature};

fn linkage_by_name(tcx: TyCtxt<'_>, def_id: LocalDefId, name: &str) -> Linkage {
use rustc_middle::mir::mono::Linkage::*;
Expand Down Expand Up @@ -139,7 +135,8 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
// coverage on a smaller scope within an excluded larger scope.
}
Some(_) | None => {
tcx.dcx().emit_err(ExpectedCoverageSymbol { span: attr.span });
tcx.dcx()
.span_delayed_bug(attr.span, "unexpected value of coverage attribute");
}
}
}
Expand Down Expand Up @@ -174,7 +171,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::USED;
}
Some(_) => {
tcx.dcx().emit_err(ExpectedUsedSymbol { span: attr.span });
tcx.dcx().emit_err(errors::ExpectedUsedSymbol { span: attr.span });
}
None => {
// Unfortunately, unconditionally using `llvm.used` causes
Expand Down
7 changes: 0 additions & 7 deletions compiler/rustc_codegen_ssa/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -564,13 +564,6 @@ pub struct UnknownArchiveKind<'a> {
pub kind: &'a str,
}

#[derive(Diagnostic)]
#[diag(codegen_ssa_expected_coverage_symbol)]
pub struct ExpectedCoverageSymbol {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(codegen_ssa_expected_used_symbol)]
pub struct ExpectedUsedSymbol {
Expand Down
26 changes: 15 additions & 11 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ pub struct AttributeTemplate {
pub word: bool,
/// If `Some`, the attribute is allowed to take a list of items like `#[allow(..)]`.
pub list: Option<&'static str>,
/// If non-empty, the attribute is allowed to take a list containing exactly
/// one of the listed words, like `#[coverage(off)]`.
pub one_of: &'static [Symbol],
/// If `Some`, the attribute is allowed to be a name/value pair where the
/// value is a string, like `#[must_use = "reason"]`.
pub name_value_str: Option<&'static str>,
Expand Down Expand Up @@ -165,19 +168,20 @@ pub enum AttributeDuplicates {
/// E.g., `template!(Word, List: "description")` means that the attribute
/// supports forms `#[attr]` and `#[attr(description)]`.
macro_rules! template {
(Word) => { template!(@ true, None, None) };
(List: $descr: expr) => { template!(@ false, Some($descr), None) };
(NameValueStr: $descr: expr) => { template!(@ false, None, Some($descr)) };
(Word, List: $descr: expr) => { template!(@ true, Some($descr), None) };
(Word, NameValueStr: $descr: expr) => { template!(@ true, None, Some($descr)) };
(Word) => { template!(@ true, None, &[], None) };
(List: $descr: expr) => { template!(@ false, Some($descr), &[], None) };
(OneOf: $one_of: expr) => { template!(@ false, None, $one_of, None) };
(NameValueStr: $descr: expr) => { template!(@ false, None, &[], Some($descr)) };
(Word, List: $descr: expr) => { template!(@ true, Some($descr), &[], None) };
(Word, NameValueStr: $descr: expr) => { template!(@ true, None, &[], Some($descr)) };
(List: $descr1: expr, NameValueStr: $descr2: expr) => {
template!(@ false, Some($descr1), Some($descr2))
template!(@ false, Some($descr1), &[], Some($descr2))
};
(Word, List: $descr1: expr, NameValueStr: $descr2: expr) => {
template!(@ true, Some($descr1), Some($descr2))
template!(@ true, Some($descr1), &[], Some($descr2))
};
(@ $word: expr, $list: expr, $name_value_str: expr) => { AttributeTemplate {
word: $word, list: $list, name_value_str: $name_value_str
(@ $word: expr, $list: expr, $one_of: expr, $name_value_str: expr) => { AttributeTemplate {
word: $word, list: $list, one_of: $one_of, name_value_str: $name_value_str
} };
}

Expand Down Expand Up @@ -478,8 +482,8 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
EncodeCrossCrate::No, experimental!(no_sanitize)
),
gated!(
coverage, Normal, template!(Word, List: "on|off"),
WarnFollowing, EncodeCrossCrate::No,
coverage, Normal, template!(OneOf: &[sym::off, sym::on]),
ErrorPreceding, EncodeCrossCrate::No,
coverage_attribute, experimental!(coverage)
),

Expand Down
13 changes: 10 additions & 3 deletions compiler/rustc_parse/src/validate_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ use crate::{errors, parse_in};

use rustc_ast::token::Delimiter;
use rustc_ast::tokenstream::DelimSpan;
use rustc_ast::MetaItemKind;
use rustc_ast::{self as ast, AttrArgs, AttrArgsEq, Attribute, DelimArgs, MetaItem, Safety};
use rustc_ast::{
self as ast, AttrArgs, AttrArgsEq, Attribute, DelimArgs, MetaItem, MetaItemKind,
NestedMetaItem, Safety,
};
use rustc_errors::{Applicability, FatalError, PResult};
use rustc_feature::{
AttributeSafety, AttributeTemplate, BuiltinAttribute, Features, BUILTIN_ATTRIBUTE_MAP,
Expand Down Expand Up @@ -184,9 +186,13 @@ pub(super) fn check_cfg_attr_bad_delim(psess: &ParseSess, span: DelimSpan, delim

/// Checks that the given meta-item is compatible with this `AttributeTemplate`.
fn is_attr_template_compatible(template: &AttributeTemplate, meta: &ast::MetaItemKind) -> bool {
let is_one_allowed_subword = |items: &[NestedMetaItem]| match items {
[item] => item.is_word() && template.one_of.iter().any(|&word| item.has_name(word)),
_ => false,
};
match meta {
MetaItemKind::Word => template.word,
MetaItemKind::List(..) => template.list.is_some(),
MetaItemKind::List(items) => template.list.is_some() || is_one_allowed_subword(items),
MetaItemKind::NameValue(lit) if lit.kind.is_str() => template.name_value_str.is_some(),
MetaItemKind::NameValue(..) => false,
}
Expand Down Expand Up @@ -230,6 +236,7 @@ fn emit_malformed_attribute(
if let Some(descr) = template.list {
suggestions.push(format!("#{inner}[{name}({descr})]"));
}
suggestions.extend(template.one_of.iter().map(|&word| format!("#{inner}[{name}({word})]")));
if let Some(descr) = template.name_value_str {
suggestions.push(format!("#{inner}[{name} = \"{descr}\"]"));
}
Expand Down
15 changes: 3 additions & 12 deletions compiler/rustc_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,9 @@ passes_continue_labeled_block =
.label = labeled blocks cannot be `continue`'d
.block_label = labeled block the `continue` points to
passes_coverage_fn_defn =
`#[coverage]` may only be applied to function definitions
passes_coverage_ignored_function_prototype =
`#[coverage]` is ignored on function prototypes
passes_coverage_not_coverable =
`#[coverage]` must be applied to coverable code
.label = not coverable code
passes_coverage_propagate =
`#[coverage]` does not propagate into items and must be applied to the contained functions directly
passes_coverage_not_fn_or_closure =
attribute should be applied to a function definition or closure
.label = not a function or closure
passes_dead_codes =
{ $multiple ->
Expand Down
42 changes: 5 additions & 37 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
self.check_diagnostic_on_unimplemented(attr.span, hir_id, target)
}
[sym::inline] => self.check_inline(hir_id, attr, span, target),
[sym::coverage] => self.check_coverage(hir_id, attr, span, target),
[sym::coverage] => self.check_coverage(attr, span, target),
[sym::non_exhaustive] => self.check_non_exhaustive(hir_id, attr, span, target),
[sym::marker] => self.check_marker(hir_id, attr, span, target),
[sym::target_feature] => {
Expand Down Expand Up @@ -369,47 +369,15 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}
}

/// Checks if a `#[coverage]` is applied directly to a function
fn check_coverage(&self, hir_id: HirId, attr: &Attribute, span: Span, target: Target) -> bool {
/// Checks that `#[coverage(..)]` is applied to a function or closure.
fn check_coverage(&self, attr: &Attribute, span: Span, target: Target) -> bool {
match target {
// #[coverage] on function is fine
// #[coverage(..)] on function is fine
Target::Fn
| Target::Closure
| Target::Method(MethodKind::Trait { body: true } | MethodKind::Inherent) => true,

// function prototypes can't be covered
Target::Method(MethodKind::Trait { body: false }) | Target::ForeignFn => {
self.tcx.emit_node_span_lint(
UNUSED_ATTRIBUTES,
hir_id,
attr.span,
errors::IgnoredCoverageFnProto,
);
true
}

Target::Mod | Target::ForeignMod | Target::Impl | Target::Trait => {
self.tcx.emit_node_span_lint(
UNUSED_ATTRIBUTES,
hir_id,
attr.span,
errors::IgnoredCoveragePropagate,
);
true
}

Target::Expression | Target::Statement | Target::Arm => {
self.tcx.emit_node_span_lint(
UNUSED_ATTRIBUTES,
hir_id,
attr.span,
errors::IgnoredCoverageFnDefn,
);
true
}

_ => {
self.dcx().emit_err(errors::IgnoredCoverageNotCoverable {
self.dcx().emit_err(errors::CoverageNotFnOrClosure {
attr_span: attr.span,
defn_span: span,
});
Expand Down
16 changes: 2 additions & 14 deletions compiler/rustc_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,9 @@ pub struct InlineNotFnOrClosure {
pub defn_span: Span,
}

#[derive(LintDiagnostic)]
#[diag(passes_coverage_ignored_function_prototype)]
pub struct IgnoredCoverageFnProto;

#[derive(LintDiagnostic)]
#[diag(passes_coverage_propagate)]
pub struct IgnoredCoveragePropagate;

#[derive(LintDiagnostic)]
#[diag(passes_coverage_fn_defn)]
pub struct IgnoredCoverageFnDefn;

#[derive(Diagnostic)]
#[diag(passes_coverage_not_coverable, code = E0788)]
pub struct IgnoredCoverageNotCoverable {
#[diag(passes_coverage_not_fn_or_closure, code = E0788)]
pub struct CoverageNotFnOrClosure {
#[primary_span]
pub attr_span: Span,
#[label]
Expand Down
39 changes: 13 additions & 26 deletions tests/ui/coverage-attr/bad-syntax.rs
Original file line number Diff line number Diff line change
@@ -1,58 +1,45 @@
#![feature(coverage_attribute)]
//@ edition: 2021

// Tests the error messages produced (or not produced) by various unusual
// uses of the `#[coverage(..)]` attribute.

// FIXME(#126658): Multiple coverage attributes with the same value are useless,
// and should probably produce a diagnostic.
#[coverage(off)]
#[coverage(off)] //~ ERROR multiple `coverage` attributes
#[coverage(off)]
fn multiple_consistent() {}

// FIXME(#126658): When there are multiple inconsistent coverage attributes,
// it's unclear which one will prevail.
#[coverage(off)]
#[coverage(off)] //~ ERROR multiple `coverage` attributes
#[coverage(on)]
fn multiple_inconsistent() {}

#[coverage] //~ ERROR expected `coverage(off)` or `coverage(on)`
#[coverage] //~ ERROR malformed `coverage` attribute input
fn bare_word() {}

// FIXME(#126658): This shows as multiple different errors, one of which suggests
// writing bare `#[coverage]`, which is not allowed.
#[coverage = true]
//~^ ERROR expected `coverage(off)` or `coverage(on)`
//~| ERROR malformed `coverage` attribute input
//~| HELP the following are the possible correct uses
//~| SUGGESTION #[coverage(on|off)]
#[coverage = true] //~ ERROR malformed `coverage` attribute input
fn key_value() {}

#[coverage()] //~ ERROR expected `coverage(off)` or `coverage(on)`
#[coverage()] //~ ERROR malformed `coverage` attribute input
fn list_empty() {}

#[coverage(off, off)] //~ ERROR expected `coverage(off)` or `coverage(on)`
#[coverage(off, off)] //~ ERROR malformed `coverage` attribute input
fn list_consistent() {}

#[coverage(off, on)] //~ ERROR expected `coverage(off)` or `coverage(on)`
#[coverage(off, on)] //~ ERROR malformed `coverage` attribute input
fn list_inconsistent() {}

#[coverage(bogus)] //~ ERROR expected `coverage(off)` or `coverage(on)`
#[coverage(bogus)] //~ ERROR malformed `coverage` attribute input
fn bogus_word() {}

#[coverage(bogus, off)] //~ ERROR expected `coverage(off)` or `coverage(on)`
#[coverage(bogus, off)] //~ ERROR malformed `coverage` attribute input
fn bogus_word_before() {}

#[coverage(off, bogus)] //~ ERROR expected `coverage(off)` or `coverage(on)`
#[coverage(off, bogus)] //~ ERROR malformed `coverage` attribute input
fn bogus_word_after() {}

#[coverage(off,)]
#[coverage(off,)] // (OK!)
fn comma_after() {}

// FIXME(#126658): This shows as multiple different errors.
#[coverage(,off)]
//~^ ERROR expected identifier, found `,`
//~| HELP remove this comma
//~| ERROR expected `coverage(off)` or `coverage(on)`
#[coverage(,off)] //~ ERROR expected identifier, found `,`
fn comma_before() {}

fn main() {}
Loading

0 comments on commit aafafa9

Please sign in to comment.