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

iwanders · 2022-01-13T00:20:46Z

Hi, I ran into difficulties using the --cfg flag syntax, first hit when googling for the error was issue #66450. Reading that issue, it sounded like the best way to improve the experience was to improve the error message, this is low risk and doesn't introduce any additional argument parsing.

The issue mentions that it is entirely dependent on the shell, while this may be true, I think guiding the the user into the realization that the quotes may need to be escaped is helpful. The two suggested escapings both work in Bash and in the Windows command prompt.

fyi @ehuss

This provides the user with a helpful error message in case a key="value" message was specified but couldn't be parsed.

rust-highfive · 2022-01-13T00:20:48Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nagisa (or someone else) soon.

Please see the contribution instructions for more information.

iwanders · 2022-01-13T00:53:23Z

I naively checked the tests here, didn't see one there, grepped in the compiler directory and couldn't find a match, so did not notice the ones in src. Pushed a fix, ./x.py test -i src/test/ui/conditional-compilation passes locally.

nagisa

Hm, it would be good to have a test for --cfg key=value, we don't appear to have any tests for cases where the --cfg value is malformed in a very subtle way like what this PR is trying to address.

nagisa · 2022-01-15T18:46:39Z

compiler/rustc_interface/src/interface.rs

-                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\""#


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)

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.

seems reasonable to me.

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

iwanders · 2022-01-20T23:05:17Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

iwanders · 2022-01-20T23:07:41Z

Docs on rustbot state r? only works in the PR description, so I can't request a review from @estebank, hopefully this ping does the job :)

nagisa · 2022-01-20T23:09:06Z

@bors r+

bors · 2022-01-20T23:09:08Z

📌 Commit af87248 has been approved by nagisa

nagisa · 2022-01-20T23:09:44Z

(as a side note r? command does work in comments too)

iwanders · 2022-01-20T23:20:16Z

(as a side note r? command does work in comments too)

Cool, I've filed rust-lang/rustc-dev-guide#1289 to improve the docs.

…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#91965 (Add more granular `--exclude` in `x.py`) - rust-lang#92467 (Ensure that early-bound function lifetimes are always 'local') - rust-lang#92586 (Set the allocation MIN_ALIGN for espidf to 4.) - rust-lang#92835 (Improve error message for key="value" cfg arguments.) - rust-lang#92843 (Improve string concatenation suggestion) - rust-lang#92963 (Implement tuple array diagnostic) - rust-lang#93046 (Use let_else in even more places) - rust-lang#93109 (Improve `Arc` and `Rc` documentation) - rust-lang#93134 (delete `Stdin::split` forwarder) - rust-lang#93139 (rustdoc: fix overflow-wrap for table layouts) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Improve error message for cfg arguments.

90330b5

This provides the user with a helpful error message in case a key="value" message was specified but couldn't be parsed.

rust-highfive assigned nagisa Jan 13, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 13, 2022

This comment has been minimized.

Sign in to view

Update unit tests to accomodate for improved error.

db56c6d

nagisa reviewed Jan 15, 2022

View reviewed changes

nagisa 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 Jan 15, 2022

iwanders added 3 commits January 16, 2022 11:04

Add unit test for cfg-arg without quotes.

32be348

Limit scope of the hint about escaping.

1531d26

Update unit tests to match more limited hint about escaping.

af87248

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

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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2022

iwanders mentioned this pull request Jan 20, 2022

Clarify that r? works in comments. rust-lang/rustc-dev-guide#1289

Merged

matthiaskrgr mentioned this pull request Jan 21, 2022

Rollup of 10 pull requests #93173

Merged

bors merged commit e38cbc7 into rust-lang:master Jan 22, 2022

rustbot added this to the 1.60.0 milestone Jan 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iwanders commented Jan 13, 2022

rust-highfive commented Jan 13, 2022

This comment has been minimized.

iwanders commented Jan 13, 2022

nagisa left a comment

nagisa Jan 15, 2022 •

edited

Loading

iwanders Jan 16, 2022

nagisa Jan 16, 2022

iwanders commented Jan 20, 2022

iwanders commented Jan 20, 2022

nagisa commented Jan 20, 2022

bors commented Jan 20, 2022

nagisa commented Jan 20, 2022

iwanders commented Jan 20, 2022

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

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

Conversation

iwanders commented Jan 13, 2022

rust-highfive commented Jan 13, 2022

This comment has been minimized.

iwanders commented Jan 13, 2022

nagisa left a comment

Choose a reason for hiding this comment

nagisa Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

iwanders Jan 16, 2022

Choose a reason for hiding this comment

nagisa Jan 16, 2022

Choose a reason for hiding this comment

iwanders commented Jan 20, 2022

iwanders commented Jan 20, 2022

nagisa commented Jan 20, 2022

bors commented Jan 20, 2022

nagisa commented Jan 20, 2022

iwanders commented Jan 20, 2022

nagisa Jan 15, 2022 •

edited

Loading