Lint on invalid values passed to x.py --warnings #62406

Mark-Simulacrum · 2019-07-05T11:27:17Z

This also introduces support for --warnings allow and fixes --warnings
being overridden by the configuration file, config.toml.

Fixes #62402

r? @RalfJung

Centril · 2019-07-05T12:27:41Z

src/bootstrap/flags.rs

@@ -443,6 +443,19 @@ Arguments:
            }
        };

+        let warnings = match matches.opt_str("warnings").as_ref().map(|v| v.as_str()) {


Could you add this to its own function + a comment re. what is happening?

The code looks pretty obvious to me - what exactly would you want in the comment?

Well for one it would be good to know what the role of the warnings field is (it just says "true => deny" which is pretty unclear), why "deny" implies true, etc. But also it is hard to read code when you just have a big blob inside a function.

Also, why isn't the closure usage used here instead of using std::process::exit? (Moreover, std::process is in scope.)

Also, I feel obliged to echo @oli-obk here.

I've added some docs to the field (previously warnings, now deny_warnings).

I disagree that this would be good to pull out into a separate function; I think that wouldn't actually help. This is a single statement, not a group, and this function is the one that deals with processing command line arguments. It's also not a repetition of code, i.e., we're not going to be able to have other code which calls this function.

We're not using usage because I wanted to tailor the error message specifically to warnings, e.g., by including the permitted values. It's possibly that it could be made better, but I don't personally want to spend the time figuring out how to integrate with usage. This works, is simple, and IMO easier to interpret for end-users and future readers of this code.

I've added some docs to the field (previously warnings, now deny_warnings).

Thanks!

I disagree that this would be good to pull out into a separate function; I think that wouldn't actually help. This is a single statement, not a group, and this function is the one that deals with processing command line arguments. It's also not a repetition of code, i.e., we're not going to be able to have other code which calls this function.

I don't think that matters so much. By having large functions you force readers to pay attention to details which may be irrelevant to what the function does at large (in this case parsing all the arguments into flags instead of just dealing with --warnings) and it makes skimming the code harder. We don't have to have 5 line functions everywhere, but 395 lines is not readable.

I guess I don't care that much. I can do it if it'll unblock this PR. I've never read through this whole function though when trying to edit it, so I don't really see the point, and I continue to think it's actually harmful - makes understanding it harder rather than easier in straight-line code. I would have a different opinion if this function was a mess of if statements that were all nested, but it's not.

Centril · 2019-07-05T12:28:10Z

src/bootstrap/flags.rs

@@ -468,7 +481,7 @@ Arguments:
                .into_iter()
                .map(|p| p.into())
                .collect::<Vec<_>>(),
-            warnings: matches.opt_str("warnings").map(|v| v == "deny"),
+            warnings: warnings,


Suggested change

warnings: warnings,

warnings,

RalfJung · 2019-07-05T13:26:42Z

Are you sure you want me to review bootstrap stuff? I can try, but I have basically not have had any contact with that code.^^

Mark-Simulacrum · 2019-07-05T13:30:48Z

This is just touching argument parsing; pretty much anyone can review it. Up to you. If you don't want to I'd assign to @alexcrichton

RalfJung

Well I suppose I can try to just make guesses about the rest of the code. ;) Seems unlikely to have subtle interactions.

I just have a few questions and one nit, r=me otherwise.

RalfJung · 2019-07-05T13:40:47Z

src/bootstrap/config.rs

@@ -405,7 +405,7 @@ impl Config {
        config.incremental = flags.incremental;
        config.dry_run = flags.dry_run;
        config.keep_stage = flags.keep_stage;
-        if let Some(value) = flags.warnings {
+        if let Some(value) = flags.deny_warnings {
            config.deny_warnings = value;
        }


So argument parsing here is some kind of two-stage process? &[String] -> Flags -> Config? And this is specifically for what gets passed on the cmdline?

Yep, that's correct -- configuration loading is a bit non-ideal.

RalfJung · 2019-07-05T13:41:13Z

src/bootstrap/config.rs

@@ -571,7 +571,7 @@ impl Config {
            config.rustc_default_linker = rust.default_linker.clone();
            config.musl_root = rust.musl_root.clone().map(PathBuf::from);
            config.save_toolstates = rust.save_toolstates.clone().map(PathBuf::from);
-            set(&mut config.deny_warnings, rust.deny_warnings.or(flags.warnings));
+            set(&mut config.deny_warnings, flags.deny_warnings.or(rust.deny_warnings));


rust here refers to the config.toml?

Oh also, this inversion of priority won't disturb CI somehow? I don't know exactly how CI passes its parameters but I seem to recall it's not through config.toml.

Yes, that's true. CI doesn't pass the --warnings flag so it should be fine.

RalfJung · 2019-07-05T13:42:51Z

src/bootstrap/flags.rs

@@ -468,7 +472,7 @@ Arguments:
                .into_iter()
                .map(|p| p.into())
                .collect::<Vec<_>>(),
-            warnings: matches.opt_str("warnings").map(|v| v == "deny"),
+            deny_warnings,


IMO readability is slightly improved by inlining the let:
deny_warnings: parse_deny_warnings(&matches)
Not a blocker though.

This also introduces support for `--warnings allow` and fixes --warnings being overridden by the configuration file, config.toml.

Mark-Simulacrum · 2019-07-05T14:17:14Z

@bors r=RalfJung

bors · 2019-07-05T14:17:16Z

📌 Commit f01e5e6 has been approved by RalfJung

@RalfJung

…lfJung Lint on invalid values passed to x.py --warnings This also introduces support for `--warnings allow` and fixes --warnings being overridden by the configuration file, config.toml. Fixes rust-lang#62402 r? @RalfJung

@RalfJung

…lfJung Lint on invalid values passed to x.py --warnings This also introduces support for `--warnings allow` and fixes --warnings being overridden by the configuration file, config.toml. Fixes rust-lang#62402 r? @RalfJung

@RalfJung

…lfJung Lint on invalid values passed to x.py --warnings This also introduces support for `--warnings allow` and fixes --warnings being overridden by the configuration file, config.toml. Fixes rust-lang#62402 r? @RalfJung

@ghost

Rollup of 13 pull requests Successful merges: - #61545 (Implement another internal lints) - #62110 (Improve -Ztime-passes) - #62133 (Feature gate `rustc` attributes harder) - #62158 (Add MemoryExtra in InterpretCx constructor params) - #62168 (The (almost) culmination of HirIdification) - #62193 (Create async version of the dynamic-drop test) - #62369 (Remove `compile-pass` from compiletest) - #62380 (rustc_target: avoid negative register counts in the SysV x86_64 ABI.) - #62381 (Fix a typo in Write::write_vectored docs) - #62390 (Update README.md) - #62396 (remove Scalar::is_null_ptr) - #62406 (Lint on invalid values passed to x.py --warnings) - #62414 (Remove last use of mem::uninitialized in SGX) Failed merges: r? @ghost

rust-highfive assigned RalfJung Jul 5, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2019

Mark-Simulacrum mentioned this pull request Jul 5, 2019

x.py build --warnings deny does not fail despite warnings #62402

Closed

Centril reviewed Jul 5, 2019

View reviewed changes

Mark-Simulacrum force-pushed the warnings-lint branch 2 times, most recently from 60ee3d5 to 5f8c24c Compare July 5, 2019 12:56

Mark-Simulacrum force-pushed the warnings-lint branch from 5f8c24c to 7a1d635 Compare July 5, 2019 13:30

RalfJung reviewed Jul 5, 2019

View reviewed changes

Lint on invalid values passed to x.py --warnings

f01e5e6

This also introduces support for `--warnings allow` and fixes --warnings being overridden by the configuration file, config.toml.

Mark-Simulacrum force-pushed the warnings-lint branch from 7a1d635 to f01e5e6 Compare July 5, 2019 14:14

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 Jul 5, 2019

Centril mentioned this pull request Jul 5, 2019

Rollup of 14 pull requests #62413

Closed

Centril mentioned this pull request Jul 5, 2019

Rollup of 14 pull requests #62418

Closed

Centril mentioned this pull request Jul 5, 2019

Rollup of 13 pull requests #62419

Merged

bors merged commit f01e5e6 into rust-lang:master Jul 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint on invalid values passed to x.py --warnings #62406

Lint on invalid values passed to x.py --warnings #62406

Mark-Simulacrum commented Jul 5, 2019

Centril Jul 5, 2019

Mark-Simulacrum Jul 5, 2019

Centril Jul 5, 2019 •

edited

Loading

Mark-Simulacrum Jul 5, 2019

Centril Jul 5, 2019 •

edited

Loading

Mark-Simulacrum Jul 5, 2019

Centril Jul 5, 2019

RalfJung commented Jul 5, 2019

Mark-Simulacrum commented Jul 5, 2019

RalfJung left a comment •

edited

Loading

RalfJung Jul 5, 2019

Mark-Simulacrum Jul 5, 2019

RalfJung Jul 5, 2019

RalfJung Jul 5, 2019

Mark-Simulacrum Jul 5, 2019

RalfJung Jul 5, 2019

Mark-Simulacrum commented Jul 5, 2019

bors commented Jul 5, 2019

Lint on invalid values passed to x.py --warnings #62406

Lint on invalid values passed to x.py --warnings #62406

Conversation

Mark-Simulacrum commented Jul 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 5, 2019

Mark-Simulacrum commented Jul 5, 2019

RalfJung left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Jul 5, 2019

bors commented Jul 5, 2019

Centril Jul 5, 2019 •

edited

Loading

Centril Jul 5, 2019 •

edited

Loading

RalfJung left a comment •

edited

Loading