Skip to content

Improve error message for key="value" cfg arguments. #92835

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 22, 2022
Merged

Improve error message for key="value" cfg arguments. #92835

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
90330b5
Improve error message for cfg arguments.
iwanders Jan 13, 2022
db56c6d
Update unit tests to accomodate for improved error.
iwanders Jan 13, 2022
32be348
Add unit test for cfg-arg without quotes.
iwanders Jan 16, 2022
1531d26
Limit scope of the hint about escaping.
iwanders Jan 16, 2022
af87248
Update unit tests to match more limited hint about escaping.
iwanders Jan 16, 2022
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
5 changes: 4 additions & 1 deletion compiler/rustc_interface/src/interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,10 @@ pub fn parse_cfgspecs(cfgspecs: Vec<String>) -> FxHashSet<(String, Option<String
Err(errs) => errs.into_iter().for_each(|mut err| err.cancel()),
}

error!(r#"expected `key` or `key="value"`"#);
error!(concat!(
r#"expected `key` or `key="value"`, ensure escaping is appropriate"#,
r#" for your shell, try 'key="value"' or key=\"value\""#
Copy link
Member

@nagisa nagisa Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When --cfg a(b=c) or similarly malformed argument is provided, such as in the test below, this added note seems really out of place.

Is there a way rustc could detect specifically for the situation where the escapes may be missing? I imagine any case where this situation occurs, rustc will see something like key=value.

Alternatively what about if rustc printed out the argument we received and could not parse? Something like

expected `key` or `key="value"`, got `key=value`

seem like might be just enough information for the user to figure out what may be going wrong (and avoids assuming any details about the environment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the value the parser obtained is already in the output:

$ rustc --cfg foo=val
error: invalid `--cfg` argument: `foo=val` (expected `key` or `key="value"`)

But, this unfortunately doesn't do much in guiding the user towards the escaping issue. If one is to read the message carefully, you can spot the difference of course. But if spotting the difference was easy, the issue wouldn't have been filed. Neither would I have gone through the trouble to incorporate its suggestion to more clearly nudge towards the proper escaping.

Stating what we received duplicates this information, it does more closely couple the quoted "value" next to whatever the user passed in though, so the difference may be easier to spot? It would look like this:

$ ./rustc --cfg foo=val
error: invalid `--cfg` argument: `foo=val` (expected `key` or `key="value"`, got `foo=val`)

I definitely agree with you that the current message would look odd in a lot of situations, like when a shell variable substitution goes wrong or something;

$ rustc --cfg '$VAR'
error: invalid `--cfg` argument: `$VAR` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

Like, in that case it's just the wrong advice, I didn't really think of that failure mode. Basically if any invalid character occurs in the string, we would get this 'hint' now, even though it may not be applicable, this is definitely not desirable.

So thinking about this a bit more, we can easily make the error message more specific by checking for = and if that is present check for =" and ending with ". That makes this hint much more specific, only hinting in cases where the user likely tried to do a key="value" cfg flag? We will still trigger on the a(b=c) situation, but I think that's reasonable? The ( can't be part of the key, the ) could be part of the value; ab="c)" is allowed.

I've implemented this limiting the scope of printing this hint using the simple rules proposed, see the updated unit tests. Only the a(b=c) and (newly added) key=value unit tests now print this hint, let me know your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

seems reasonable to me.

cc @estebank may want to take a look here as well.

));
})
.collect::<CrateConfig>();
cfg.into_iter().map(|(a, b)| (a.to_string(), b.map(|b| b.to_string()))).collect()
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// compile-flags: --cfg a(b=c)
// error-pattern: invalid `--cfg` argument: `a(b=c)` (expected `key` or `key="value"`)
// error-pattern: invalid `--cfg` argument: `a(b=c)` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")
fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-1.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
error: invalid `--cfg` argument: `a(b=c)` (expected `key` or `key="value"`)
error: invalid `--cfg` argument: `a(b=c)` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// compile-flags: --cfg a{b}
// error-pattern: invalid `--cfg` argument: `a{b}` (expected `key` or `key="value"`)
// error-pattern: invalid `--cfg` argument: `a{b}` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")
fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
error: invalid `--cfg` argument: `a{b}` (expected `key` or `key="value"`)
error: invalid `--cfg` argument: `a{b}` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-4.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// compile-flags: --cfg a(b)
// error-pattern: invalid `--cfg` argument: `a(b)` (expected `key` or `key="value"`)
// error-pattern: invalid `--cfg` argument: `a(b)` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")
fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-4.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
error: invalid `--cfg` argument: `a(b)` (expected `key` or `key="value"`)
error: invalid `--cfg` argument: `a(b)` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-6.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// compile-flags: --cfg a{
// error-pattern: invalid `--cfg` argument: `a{` (expected `key` or `key="value"`)
// error-pattern: invalid `--cfg` argument: `a{` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")
fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-6.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
error: invalid `--cfg` argument: `a{` (expected `key` or `key="value"`)
error: invalid `--cfg` argument: `a{` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-8.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// compile-flags: --cfg )
// error-pattern: invalid `--cfg` argument: `)` (expected `key` or `key="value"`)
// error-pattern: invalid `--cfg` argument: `)` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")
fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-arg-invalid-8.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
error: invalid `--cfg` argument: `)` (expected `key` or `key="value"`)
error: invalid `--cfg` argument: `)` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-empty-codemap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

// compile-flags: --cfg ""

// error-pattern: invalid `--cfg` argument: `""` (expected `key` or `key="value"`)
// error-pattern: invalid `--cfg` argument: `""` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")

pub fn main() {
}
2 changes: 1 addition & 1 deletion src/test/ui/conditional-compilation/cfg-empty-codemap.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
error: invalid `--cfg` argument: `""` (expected `key` or `key="value"`)
error: invalid `--cfg` argument: `""` (expected `key` or `key="value"`, ensure escaping is appropriate for your shell, try 'key="value"' or key=\"value\")