lint: add bad opt access internal lint

[davidtwco]

Prompted by Zulip discussion.

Some command-line options accessible through sess.opts are best accessed through wrapper functions on Session, TyCtxt or otherwise, rather than through field access on the option struct in the Session.

Adds a new lint which triggers on those options that should be accessed through a wrapper function so that this is prohibited. Options are annotated with a new attribute rustc_lint_opt_deny_field_access which can specify the error message (i.e. "use this other function instead") to be emitted.

A simpler alternative would be to simply rename the options in the option type so that it is clear they should not be used, however this doesn't prevent uses, just discourages them. Another alternative would be to make the option fields private, and adding accessor functions on the option types, however the wrapper functions sometimes rely on additional state from Session or TyCtxt which wouldn't be available in an function on the option type, so the accessor would simply make the field available and its use would be discouraged too.

Leave a comment if there's an option I should add this to.

[rust-highfive]

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

[bjorn3]

Maybe also enable this lint for cli_forced_codegen_units, verify_llvm_ir, lto, thinlto and plt? Or even for all fields which have a wrapper, no matter how small?

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[RalfJung]

A simpler alternative would be to simply rename the options

An even simpler alternative would be to make these fields inaccessible using standard Rust visibility restrictions?

[bjorn3]

Sometimes they need to be accessed without an available Session. For example https://github.com/rust-lang/rust/pull/99710/files#diff-96da842aaf6582a52b9f23b4b353ef42214149dfb874819da8b616bacfb4b43eR564 for crate_types. Or every option for custom drivers that want to change options. (for example when cg_clif used a custom driver, it set panic=abort from inside the driver)

[davidtwco]

A simpler alternative would be to simply rename the options

An even simpler alternative would be to make these fields inaccessible using standard Rust visibility restrictions?

That's the second alternative I listed, it would absolutely work, and we could add our wrapper functions to Options/CodegenOptions/DebuggingOptions. But as soon as those wrapper functions need to make use of something from Session or TyCtxt, then we either need to pass those in (possible for Session, but probably not for TyCtxt), or the wrapper function just becomes a getter function that another wrapper on Session/TyCtxt uses - in which case it could be used accidentally just like a public field. A lint is a bit more machinery but it keeps the option accessible for the wrapper function, wherever that exists, but also prevents it from being used otherwise. I might be missing something though.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[lcnr]

a few nits, then r=me

haven't really checked the fields which have this lint, but even if one of the fields is incorrectly linted, that shouldn't hurt too much

[davidtwco]

@bors r=lcnr

[bors]

📌 Commit 7bab769 has been approved by lcnr

It is now in the queue for this repository.

[jyn514]

Another alternative would be to make the option fields private, and adding accessor functions on the option types, however the wrapper functions sometimes rely on additional state from Session or TyCtxt which wouldn't be available in an function on the option type, so the accessor would simply make the field available and its use would be discouraged too.

Can we instead move those fields to be defined in the same crate as Session? This seems like an improvement over the status quo, but at the cost of increased complexity ...

[davidtwco]

Another alternative would be to make the option fields private, and adding accessor functions on the option types, however the wrapper functions sometimes rely on additional state from Session or TyCtxt which wouldn't be available in an function on the option type, so the accessor would simply make the field available and its use would be discouraged too.

Can we instead move those fields to be defined in the same crate as Session? This seems like an improvement over the status quo, but at the cost of increased complexity ...

They already are, we could use pub(crate) (honestly, that skipped my mind entirely when writing this, for some reason, I only considered pub). If we want to do that instead then I don't mind r=me'ing a PR that removes this and does that instead.

[bjorn3]

pub(crate) wouldn't allow options to be changed in rustc_driver::Callbacks::config.

[Mark-Simulacrum]

It also doesn't prevent other accessors not using the accessor for this field - I think to do that we'd need wrapper structs in private modules or something, which feels ungreat. I think the lint is a pretty OK method of achieving our goals here.