Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiletest: error when finding a trailing directive #123753

Merged
merged 1 commit into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -935,16 +935,25 @@ struct HeaderLine<'ln> {
pub(crate) struct CheckDirectiveResult<'ln> {
is_known_directive: bool,
directive_name: &'ln str,
trailing_directive: Option<&'ln str>,
}

// Returns `(is_known_directive, directive_name)`.
pub(crate) fn check_directive(directive_ln: &str) -> CheckDirectiveResult<'_> {
let directive_name =
directive_ln.split_once([':', ' ']).map(|(pre, _)| pre).unwrap_or(directive_ln);
let (directive_name, post) = directive_ln.split_once([':', ' ']).unwrap_or((directive_ln, ""));

let trailing = post.trim().split_once(' ').map(|(pre, _)| pre).unwrap_or(post);
let trailing_directive = {
// 1. is the directive name followed by a space? (to exclude `:`)
matches!(directive_ln.get(directive_name.len()..), Some(s) if s.starts_with(" "))
// 2. is what is after that directive also a directive (ex: "only-x86 only-arm")
&& KNOWN_DIRECTIVE_NAMES.contains(&trailing)
}
.then_some(trailing);

CheckDirectiveResult {
is_known_directive: KNOWN_DIRECTIVE_NAMES.contains(&directive_name),
directive_name: directive_ln,
trailing_directive,
}
}

Expand Down Expand Up @@ -1014,7 +1023,8 @@ fn iter_header(
if testfile.extension().map(|e| e == "rs").unwrap_or(false) {
let directive_ln = non_revisioned_directive_line.trim();

let CheckDirectiveResult { is_known_directive, .. } = check_directive(directive_ln);
let CheckDirectiveResult { is_known_directive, trailing_directive, .. } =
check_directive(directive_ln);

if !is_known_directive {
*poisoned = true;
Expand All @@ -1028,6 +1038,21 @@ fn iter_header(

return;
}

if let Some(trailing_directive) = &trailing_directive {
*poisoned = true;

eprintln!(
"error: detected trailing compiletest test directive `{}` in {}:{}\n \
help: put the trailing directive in it's own line: `//@ {}`",
trailing_directive,
testfile.display(),
line_number,
trailing_directive,
);

return;
}
}

it(HeaderLine {
Expand All @@ -1051,7 +1076,8 @@ fn iter_header(

let rest = rest.trim_start();

let CheckDirectiveResult { is_known_directive, directive_name } = check_directive(rest);
let CheckDirectiveResult { is_known_directive, directive_name, .. } =
check_directive(rest);

if is_known_directive {
*poisoned = true;
Expand Down
21 changes: 21 additions & 0 deletions src/tools/compiletest/src/header/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -667,3 +667,24 @@ fn test_non_rs_unknown_directive_not_checked() {
);
assert!(!poisoned);
}

#[test]
fn test_trailing_directive() {
let mut poisoned = false;
run_path(&mut poisoned, Path::new("a.rs"), b"//@ only-x86 only-arm");
assert!(poisoned);
}

#[test]
fn test_trailing_directive_with_comment() {
let mut poisoned = false;
run_path(&mut poisoned, Path::new("a.rs"), b"//@ only-x86 only-arm with comment");
assert!(poisoned);
}

#[test]
fn test_not_trailing_directive() {
let mut poisoned = false;
run_path(&mut poisoned, Path::new("a.rs"), b"//@ revisions: incremental");
Copy link
Member

Choose a reason for hiding this comment

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

would this reject //@ revisions incremental? I want to say that the : was not enforced in compiletest directive grammar when parsing directives...

Copy link
Member

Choose a reason for hiding this comment

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

Also examples such as //@ revisions edition (mostly the single word no-dash directives), but probably fine since I don't think anyone would write that in isolation.

Copy link
Member Author

@Urgau Urgau Apr 10, 2024

Choose a reason for hiding this comment

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

edit: miss-read the question; yes it would be rejected!

No it would not be rejected, because it's currently used in tests/ui/invalid-compile-flags/fuel.rs:

//@ revisions: incremental threads

I also don't see a reason to reject it, since the : is a delimiter.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry let me clarify what I mean. I believe compiletest accepts both //@ revisions incremental and //@ revisions: incremental, would the logic in this PR (incorrectly) reject //@ revisions incremental?

Copy link
Member

@jieyouxu jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

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

Yeah, unfortunately,

//@ revisions incremental
fn main() {}

would get rejected

error: detected trailing compiletest test directive `incremental`

whereas

//@ revisions: incremental
fn main() {}

would get accepted. The thing here is that compiletest accepts both directive forms currently (unfortunate directive grammar strikes yet again)

EDIT: but it really should not silently accept it yet do nothing

Copy link
Member Author

Choose a reason for hiding this comment

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

No, no, it's me. I read the question the wrong way round.

I believe compiletest accepts both //@ revisions incremental and //@ revisions: incremental

I just checked, and it doesn't seems to be the case. There is no revisions (also no errors).

would the logic in this PR (incorrectly) reject //@ revisions incremental?

Yes, it would be rejected by this PR; but I would also argue that it's not incorrect since the syntax is a nop and should probably be an error.

Copy link
Member

Choose a reason for hiding this comment

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

I just checked, and it doesn't seems to be the case. There is no revisions (also no errors).

What

Copy link
Member

@jieyouxu jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

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

Ah darn, I think it's because of a combination of things:

  1. revisions is a known compiletest directive
  2. when we parse name-value directives, the directive name is required to have a trailing colon in order for the parse to succeed:
    pub fn parse_name_value_directive(&self, line: &str, directive: &str) -> Option<String> {
    let colon = directive.len();
    if line.starts_with(directive) && line.as_bytes().get(colon) == Some(&b':') {
    let value = line[(colon + 1)..].to_owned();
    debug!("{}: {}", directive, value);
    Some(expand_variables(value, self))
    } else {
    None
    }
    }

However, the way compiletest directive parsing is setup right now means that if known directive check allows a directive but the corresponding name-value directive parsing logic fails, we silently accept the directive but it has no effect.

This is awful LOL

Copy link
Member

@jieyouxu jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

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

Given that the colon is actually mandatory for name-value directives like revisions, I think it's okay to reject things like //@ only-arch known-directive because the trailing known-directive-looking-thing is very unlikely to be a directive commment.

Copy link
Member

Choose a reason for hiding this comment

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

I filed an issue #123760 to make sure we know that this is a bug, but it's out of the scope for this PR. I'm happy to accept this PR as is.

assert!(!poisoned);
}
Loading