`rustc_interface` cleanups #117268

nnethercote · 2023-10-27T07:44:50Z

Particularly in and around --cfg and --check-cfg handling.

rustbot · 2023-10-27T07:45:01Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Urgau

Thank you. This brings some consistency between the two, which is nice.

Urgau · 2023-10-27T08:33:39Z

compiler/rustc_interface/src/interface.rs

+        let mut exhaustive_names = !specs.is_empty();
+        let mut exhaustive_values = !specs.is_empty();
+        let mut expecteds = FxHashMap::default();


This makes me a bit sad. I found operating on a CheckCfg struct to be clearer.
But I suppose it's for the greater good.

I think I would prefer keeping around the struct from the start, too. It may be good to add some clearer methods instead (e.g. deduplicating the old_syntax == None and immediate overwrite check, or adding methods for inserting new elements to the expecteds field).

I'll just remove the last commit, it's not that important.

Urgau · 2023-10-27T08:34:11Z

compiler/rustc_session/src/config.rs

-impl<T> CheckCfg<T> {
-    fn map_data<O: Eq + Hash>(self, f: impl Fn(T) -> O) -> CheckCfg<O> {
+impl CheckCfg<String> {
+    pub fn intern(self) -> CheckCfg<Symbol> {


Urgau · 2023-10-27T08:35:29Z

compiler/rustc_session/src/config.rs

-/// Converts the crate `--check-cfg` options from `String` to `Symbol`.
-/// `rustc_interface::interface::Config` accepts this in the compiler configuration,
-/// but the symbol interner is not yet set up then, so we must convert it later.


Do you think it is worth keeping this comment? by moving it to the intern function maybe.

There are other new comments that communicate the same ideas, so I think that's good enough.

nnethercote · 2023-10-27T10:41:25Z

Best reviewed one commit at a time.

bjorn3 · 2023-10-27T12:54:38Z

compiler/rustc_interface/src/lib.rs

 mod queries;
 pub mod util;

-pub use callbacks::setup_callbacks;


Looks like Geobacter uses this too: https://github.com/geobacter-rs/geobacter/blob/b569ef15dc7d49816e3ae5ea730d09fc94ee7e4b/runtime-core/src/codegen/worker/mod.rs#L86 It's been almost three years since they last updated their rustc fork though it seems.

I will just remove the relevant commit, it's not that important.

Are there any basic principles I can follow to understand what APIs might be used outside of the repository? I've had trouble with things in rustc_driver_impl and rustc_interface.

Are there any basic principles I can follow to understand what APIs might be used outside of the repository?

I did a github search for the function name and language:rust and the manually ignored all search results for rustc forks.

Right, so that's a 100% descriptive approach, but it seems like there's nothing prescriptive that can be relied on? "Just don't change anything that anyone is actually using, as determined by GitHub search" is not a good state of affairs. It's very easy to violate unintentionally, and gives no guidance on how to change things that are relied on.

Usually we can remove everything that is not used in rustc and doesn't have a comment explaining which external tool needs it.

My experience is that I do that, and then someone (often @bjorn3) complains, and so then I put it back and add that comment :/

#116827 is the most recent example.

In `test_edition_parsing`, change the `build_session_options_and_crate_config` call to `build_session_options`, because the config isn't used. That leaves a single call site for `build_session_options_and_crate_config`, so just inline and remove it.

Because its order doesn't matter. This is well demonstrated by `to_crate_config`, which creates a `CrateConfig` from an `FxHashSet`.

In `parse_cfg`, we now construct a `FxHashSet<String>` directly instead of constructing a `FxHashSet<Symbol>` and then immediately converting it to a `FxHashSet<String>`(!) (The type names made this behaviour non-obvious. The next commit will make the type names clearer.)

`parse_cfgspecs` and `parse_check_cfg` run very early, before the main interner is running. They each use a short-lived interner and convert all interned symbols to strings in their output data structures. Once the main interner starts up, these data structures get converted into new data structures that are identical except with the strings converted to symbols. All is not obvious from the current code, which is a mess, particularly with inconsistent naming that obscures the parallel string/symbol data structures. This commit clean things up a lot. - The existing `CheckCfg` type is generic, allowing both `CheckCfg<String>` and `CheckCfg<Symbol>` forms. This is really useful, but it defaults to `String`. The commit removes the default so we have to use `CheckCfg<String>` and `CheckCfg<Symbol>` explicitly, which makes things clearer. - Introduces `Cfg`, which is generic over `String` and `Symbol`, similar to `CheckCfg`. - Renames some things. - `parse_cfgspecs` -> `parse_cfg` - `CfgSpecs` -> `Cfg<String>`, plus it's used in more places, rather than the underlying `FxHashSet` type. - `CrateConfig` -> `Cfg<Symbol>`. - `CrateCheckConfig` -> `CheckCfg<Symbol>` - Adds some comments explaining the string-to-symbol conversions. - `to_crate_check_config`, which converts `CheckCfg<String>` to `CheckCfg<Symbol>`, is inlined and removed and combined with the overly-general `CheckCfg::map_data` to produce `CheckCfg::<String>::intern`. - `build_configuration` now does the `Cfg<String>`-to-`Cfg<Symbol>` conversion, so callers don't need to, which removes the need for `to_crate_config`. The diff for two of the fields in `Config` is a good example of the improved clarity: ``` - pub crate_cfg: FxHashSet<(String, Option<String>)>, - pub crate_check_cfg: CheckCfg, + pub crate_cfg: Cfg<String>, + pub crate_check_cfg: CheckCfg<String>, ``` Compare that with the diff for the corresponding fields in `ParseSess`, and the relationship to `Config` is much clearer than before: ``` - pub config: CrateConfig, - pub check_config: CrateCheckConfig, + pub config: Cfg<Symbol>, + pub check_config: CheckCfg<Symbol>, ```

Despite what I claimed in an earlier commit, the ordering does matter to some degree. Using `FxIndexSet` prevents changes to the error message order in `tests/ui/check-cfg/mix.rs`.

nnethercote · 2023-10-27T22:58:55Z

I removed the two controversial commits. I also added a new commit changing Cfg<T> back to an FxIndexSet, because I got an error message order difference after updating, which suggests that FxHashSet isn't good enough. I think it should be ready for re-review.

oli-obk · 2023-10-28T06:06:19Z

@bors r+

bors · 2023-10-28T06:06:21Z

📌 Commit 5438004 has been approved by oli-obk

It is now in the queue for this repository.

…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#116534 (Remove -Zdep-tasks.) - rust-lang#116739 (Make `E0277` use short paths) - rust-lang#116816 (Create `windows/api.rs` for safer FFI) - rust-lang#116945 (When encountering sealed traits, point types that implement it) - rust-lang#117025 (Cleanup and improve `--check-cfg` implementation) - rust-lang#117256 (Parse rustc version at compile time) - rust-lang#117268 (`rustc_interface` cleanups) - rust-lang#117277 (fix failure to detect a too-big-type after adding padding) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117268 - nnethercote:rustc_interface, r=oli-obk `rustc_interface` cleanups Particularly in and around `--cfg` and `--check-cfg` handling. r? `@oli-obk`

…r=oli-obk More `rustc_interface` cleanups In particular, following up rust-lang#117268 with more improvement to `--cfg`/`--check-cfg` handling. r? `@oli-obk`

…r=oli-obk More `rustc_interface` cleanups In particular, following up rust-lang#117268 with more improvement to `--cfg`/`--check-cfg` handling. r? ``@oli-obk``

Rollup merge of rust-lang#117376 - nnethercote:rustc_interface-more, r=oli-obk More `rustc_interface` cleanups In particular, following up rust-lang#117268 with more improvement to `--cfg`/`--check-cfg` handling. r? ``@oli-obk``

rustbot assigned oli-obk Oct 27, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 27, 2023

Urgau reviewed Oct 27, 2023

View reviewed changes

bjorn3 reviewed Oct 27, 2023

View reviewed changes

nnethercote added 7 commits October 28, 2023 09:03

Remove an unneeded dependency.

98c469c

Fix a comment.

3feec48

Streamline rustc_interface tests.

2142d01

In `test_edition_parsing`, change the `build_session_options_and_crate_config` call to `build_session_options`, because the config isn't used. That leaves a single call site for `build_session_options_and_crate_config`, so just inline and remove it.

Change CrateConfig from FxIndexSet to FxHashSet.

32986d8

Because its order doesn't matter. This is well demonstrated by `to_crate_config`, which creates a `CrateConfig` from an `FxHashSet`.

Change Cfg<T> to an FxIndexSet.

5438004

Despite what I claimed in an earlier commit, the ordering does matter to some degree. Using `FxIndexSet` prevents changes to the error message order in `tests/ui/check-cfg/mix.rs`.

nnethercote force-pushed the rustc_interface branch from 017671c to 5438004 Compare October 27, 2023 22:50

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 Oct 28, 2023

workingjubilee mentioned this pull request Oct 28, 2023

Rollup of 8 pull requests #117309

Merged

bors merged commit 48a3865 into rust-lang:master Oct 28, 2023

rustbot added this to the 1.75.0 milestone Oct 28, 2023

nnethercote deleted the rustc_interface branch October 29, 2023 20:59

nnethercote mentioned this pull request Oct 30, 2023

More rustc_interface cleanups #117376

Merged

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

nnethercote mentioned this pull request Nov 19, 2023

Remove support for compiler plugins. #116412

Merged

rustc_interface cleanups #117268

rustc_interface cleanups #117268

Uh oh!

Conversation

nnethercote commented Oct 27, 2023

Uh oh!

rustbot commented Oct 27, 2023

Uh oh!

Urgau left a comment

Choose a reason for hiding this comment

Uh oh!

Urgau Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Oct 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Oct 27, 2023

Uh oh!

oli-obk commented Oct 28, 2023

Uh oh!

bors commented Oct 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

`rustc_interface` cleanups #117268

`rustc_interface` cleanups #117268

Urgau Oct 27, 2023 •

edited

Loading