compiletest: error when finding a trailing directive #123753

Urgau · 2024-04-10T21:18:01Z

This PR introduce a supplementary check that when checking for a compiletest directive, will also check that the next "word" after that directive is not also by itself a directive.

This is done to avoid situations like this //@ only-linux only-x86_64 where one might think that both directives are being taken into account while in fact the second in a comment, and so was ignored, until now.

Related to #123730
cc @scottmcm
r? @jieyouxu

rustbot · 2024-04-10T21:18:11Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

jieyouxu

Changes look mostly good to me, just some nitpicks and a question regarding if this would reject e.g. //@ revisions incremental (without the :).

src/tools/compiletest/src/header.rs

jieyouxu · 2024-04-10T21:57:26Z

src/tools/compiletest/src/header/tests.rs

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


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

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.

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:

rust/tests/ui/invalid-compile-flags/fuel.rs

Line 1 in b3bd705

//@ revisions: incremental threads

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

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?

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

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.

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

What

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

revisions is a known compiletest directive

when we parse name-value directives, the directive name is required to have a trailing colon in order for the parse to succeed:

rust/src/tools/compiletest/src/header.rs

Lines 1137 to 1146 in b14d8b2

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

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.

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.

jieyouxu · 2024-04-10T22:03:51Z

@rustbot author

jieyouxu · 2024-04-10T22:49:48Z

Changes look good to me and I think it's okay to reject //@ <known_directive_name> <another_known_directive_name> given that <another_known_directive_name> as a single word without - is rare (e.g. incremental or edition). It's very unlikely that a directive comment would start with a known directive name. Name-value directives (with :) are not affected by this check.

Feel free to r=me after CI is green.

@bors delegate+

bors · 2024-04-10T22:49:51Z

✌️ @Urgau, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

jieyouxu · 2024-04-10T23:31:41Z

@bors r+ rollup

bors · 2024-04-10T23:31:43Z

📌 Commit acd4e94 has been approved by jieyouxu

It is now in the queue for this repository.

…ve, r=jieyouxu compiletest: error when finding a trailing directive This PR introduce a supplementary check that when checking for a compiletest directive, will also check that the next "word" after that directive is not also by itself a directive. This is done to avoid situations like this `//@ only-linux only-x86_64` where one might think that both directives are being taken into account while in fact the second in a comment, and so was ignored, until now. Related to rust-lang#123730 cc `@scottmcm` r? `@jieyouxu`

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#123490 (Refactor `panic_unwind/seh.rs` pointer use) - rust-lang#123704 (Tweak value suggestions in `borrowck` and `hir_analysis`) - rust-lang#123753 (compiletest: error when finding a trailing directive) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 2 pull requests Successful merges: - rust-lang#123704 (Tweak value suggestions in `borrowck` and `hir_analysis`) - rust-lang#123753 (compiletest: error when finding a trailing directive) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123753 - Urgau:compiletest-trailing-directive, r=jieyouxu compiletest: error when finding a trailing directive This PR introduce a supplementary check that when checking for a compiletest directive, will also check that the next "word" after that directive is not also by itself a directive. This is done to avoid situations like this `//@ only-linux only-x86_64` where one might think that both directives are being taken into account while in fact the second in a comment, and so was ignored, until now. Related to rust-lang#123730 cc ``@scottmcm`` r? ``@jieyouxu``

rustbot assigned jieyouxu Apr 10, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 10, 2024

jieyouxu reviewed Apr 10, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2024

compiletest: error when finding a trailing directive

acd4e94

Urgau force-pushed the compiletest-trailing-directive branch from 0d9ee29 to acd4e94 Compare April 10, 2024 22:13

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 10, 2024

matthiaskrgr mentioned this pull request Apr 11, 2024

Rollup of 3 pull requests #123771

Closed

matthiaskrgr mentioned this pull request Apr 11, 2024

Rollup of 2 pull requests #123776

Merged

bors merged commit a6448d3 into rust-lang:master Apr 11, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Apr 11, 2024

Urgau deleted the compiletest-trailing-directive branch April 11, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiletest: error when finding a trailing directive #123753

compiletest: error when finding a trailing directive #123753

Urgau commented Apr 10, 2024

rustbot commented Apr 10, 2024

jieyouxu left a comment

jieyouxu Apr 10, 2024

jieyouxu Apr 10, 2024

Urgau Apr 10, 2024 •

edited

Loading

jieyouxu Apr 10, 2024

jieyouxu Apr 10, 2024 •

edited

Loading

Urgau Apr 10, 2024

jieyouxu Apr 10, 2024

jieyouxu Apr 10, 2024 •

edited

Loading

jieyouxu Apr 10, 2024 •

edited

Loading

jieyouxu Apr 10, 2024

jieyouxu commented Apr 10, 2024

jieyouxu commented Apr 10, 2024 •

edited

Loading

bors commented Apr 10, 2024

jieyouxu commented Apr 10, 2024

bors commented Apr 10, 2024

	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
	}
	}

compiletest: error when finding a trailing directive #123753

compiletest: error when finding a trailing directive #123753

Conversation

Urgau commented Apr 10, 2024

rustbot commented Apr 10, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

Urgau Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Urgau Apr 10, 2024

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Apr 10, 2024

Choose a reason for hiding this comment

jieyouxu commented Apr 10, 2024

jieyouxu commented Apr 10, 2024 • edited Loading

bors commented Apr 10, 2024

jieyouxu commented Apr 10, 2024

bors commented Apr 10, 2024

Urgau Apr 10, 2024 •

edited

Loading

jieyouxu Apr 10, 2024 •

edited

Loading

jieyouxu Apr 10, 2024 •

edited

Loading

jieyouxu Apr 10, 2024 •

edited

Loading

jieyouxu commented Apr 10, 2024 •

edited

Loading