Avoid repeated interning of `env!("CFG_RELEASE")` #117188

dtolnay · 2023-10-25T21:15:18Z

Implements @cjgillot's suggestion from #117148 (comment).

rustbot · 2023-10-25T21:15:27Z

(rustbot has picked a reviewer for you, use r? to override)

cjgillot · 2023-10-26T07:01:13Z

compiler/rustc_macros/src/symbols.rs

-    let mut counter = 0u32;
-    let mut keys =
-        HashMap::<String, Span>::with_capacity(input.keywords.len() + input.symbols.len() + 10);
+    let mut entries = HashMap::<String, Preinterned>::with_capacity(


Should this use an IndexMap<String, Span>?

Ooh, good idea.

Here is what that looks like: 6eae659

indexmap::IndexMap is not used directly elsewhere in rustc, but I was not able to use rustc_data_structures::FxIndexMap here because rustc_data_structures depends on rustc_index which depends on rustc_macros which then cannot depend on rustc_data_structures. This could be fixed by splitting symbols! out of rustc_macros into a different crate, or splitting FxIndexMap out of rustc_data_structures, or just redefining another FxIndexMap within rustc_macros.

rust/compiler/rustc_data_structures/src/fx.rs

Line 7 in ccb160d

pub type FxIndexMap<K, V> = indexmap::IndexMap<K, V, BuildHasherDefault<FxHasher>>;

From looking at 6eae659, I think the original code is slightly preferable to me.

IndexMap makes illegal states unrepresentable because it's no longer possible for there to be Preinterned values with duplicate idx, or gaps in idx. However, needing to handle the conversion from usize indices to u32 in 4 places (instead of 1) eats into the benefit, and overall the code gets less comprehensible: for example compare prev.span_of_name vs *prev.get().

…of truth

---- symbols::tests::test_symbols stdout ---- thread 'symbols::tests::test_symbols' panicked at library/proc_macro/src/bridge/client.rs:311:17: procedural macro API is used outside of a procedural macro

dtolnay · 2023-10-26T09:03:22Z

Rebased to resolve conflict with Store #[stable] attribute's since value in structured form #117148

cjgillot · 2023-10-26T10:05:54Z

@bors r+

bors · 2023-10-26T10:05:56Z

📌 Commit c1552df has been approved by cjgillot

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#116968 (Invalid `?` suggestion on mismatched `Ok(T)`) - rust-lang#117032 (Enable cg_clif tests for riscv64gc) - rust-lang#117106 (When expecting closure argument but finding block provide suggestion) - rust-lang#117114 (Improve `stringify.rs` test) - rust-lang#117188 (Avoid repeated interning of `env!("CFG_RELEASE")`) - rust-lang#117243 (Explain implementation of mem::replace) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117188 - dtolnay:symbolenv, r=cjgillot Avoid repeated interning of `env!("CFG_RELEASE")` Implements `@cjgillot's` suggestion from rust-lang#117148 (comment).

nnethercote · 2023-11-08T22:41:32Z

compiler/rustc_macros/src/symbols.rs

        prefill_stream.extend(quote! {
            #n,
        });
    }

+    // Symbols whose value comes from an environment variable. It's allowed for
+    // these to have the same value as another symbol.


Why is this allowed? In practice it won't be a problem because no other pre-interned symbol will match the version number, but in principle if one did it could certainly lead to incorrect comparisons.

Could you share an example of how it might go wrong?

This implementation is intended to handle that situation correctly. I can't think of how having 2 Rust names for the same symbol would be bad.

Note that they will have the same SymbolIndex.

I see, I misread the comment. I thought it was possible for two identical strings to end up with different symbol values. But the prev.idx case below covers the case where an env var has the same value as another symbol. Two different names for the same string/symbol should indeed be fine. Sorry for the noise.

rustbot assigned wesleywiser Oct 25, 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. labels Oct 25, 2023

dtolnay mentioned this pull request Oct 25, 2023

Store #[stable] attribute's since value in structured form #117148

Merged

cjgillot reviewed Oct 26, 2023

View reviewed changes

dtolnay added 10 commits October 26, 2023 01:57

Touch up syn parsing in symbols macro

173dcb2

Represent symbol value as enum to prepare for supporting env vars

ba17934

Add a Parse impl for symbol Value

95742ff

Delete counter from symbols proc macro in favor of hashmap as source …

8dea49a

…of truth

Move symbols macro map into a struct

726a43c

Support environment variable for interned Symbol value

65f0253

Continue generating other symbols if an expr is not supported

1078250

Improve span of env-related errors

5563a9b

Pre-intern a symbol for env!("CFG_RELEASE")

ac4fa3f

Fix symbols::tests::test_symbols

c1552df

---- symbols::tests::test_symbols stdout ---- thread 'symbols::tests::test_symbols' panicked at library/proc_macro/src/bridge/client.rs:311:17: procedural macro API is used outside of a procedural macro

dtolnay force-pushed the symbolenv branch from 34e14ef to c1552df Compare October 26, 2023 09:03

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 26, 2023

matthiaskrgr mentioned this pull request Oct 26, 2023

Rollup of 6 pull requests #117249

Merged

bors merged commit 2656c98 into rust-lang:master Oct 27, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 27, 2023

dtolnay deleted the symbolenv branch October 27, 2023 00:06

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

Automated pull from upstream master ferrocene/ferrocene#74

Merged

nnethercote reviewed Nov 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid repeated interning of `env!("CFG_RELEASE")` #117188

Avoid repeated interning of `env!("CFG_RELEASE")` #117188

dtolnay commented Oct 25, 2023

rustbot commented Oct 25, 2023

cjgillot Oct 26, 2023

dtolnay Oct 26, 2023

dtolnay commented Oct 26, 2023

cjgillot commented Oct 26, 2023

bors commented Oct 26, 2023

nnethercote Nov 8, 2023

dtolnay Nov 24, 2023

nnethercote Nov 27, 2023

Avoid repeated interning of env!("CFG_RELEASE") #117188

Avoid repeated interning of env!("CFG_RELEASE") #117188

Conversation

dtolnay commented Oct 25, 2023

rustbot commented Oct 25, 2023

cjgillot Oct 26, 2023

Choose a reason for hiding this comment

dtolnay Oct 26, 2023

Choose a reason for hiding this comment

dtolnay commented Oct 26, 2023

cjgillot commented Oct 26, 2023

bors commented Oct 26, 2023

nnethercote Nov 8, 2023

Choose a reason for hiding this comment

dtolnay Nov 24, 2023

Choose a reason for hiding this comment

nnethercote Nov 27, 2023

Choose a reason for hiding this comment

Avoid repeated interning of `env!("CFG_RELEASE")` #117188

Avoid repeated interning of `env!("CFG_RELEASE")` #117188